Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tests: rework eth drivers #17813

Merged
merged 13 commits into from
Mar 28, 2022
Merged

tests: rework eth drivers #17813

merged 13 commits into from
Mar 28, 2022

Conversation

leandrolanzieri
Copy link
Contributor

Contribution description

Split from #17739, but with a difference: instead of using symlinks the common functionality to interact with Ethernet netdev drivers is implemented in its own test_utils module.

Testing procedure

  • Run the tests, you should be able to send an receive Ethernet frames
  • Green CI

Issues/PRs references

#17739

@leandrolanzieri leandrolanzieri requested a review from maribu March 16, 2022 16:12
@leandrolanzieri leandrolanzieri mentioned this pull request Mar 16, 2022
Merged
@github-actions github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework labels Mar 16, 2022
kaspar030
kaspar030 reviewed Mar 16, 2022
View reviewed changes
.murdock Outdated Show resolved Hide resolved
@leandrolanzieri leandrolanzieri added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 18, 2022
@leandrolanzieri
Copy link
Contributor Author

CI is passing on this one

@leandrolanzieri leandrolanzieri added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Mar 21, 2022
@github-actions github-actions bot added the Area: network Area: Networking label Mar 21, 2022
benpicco
benpicco reviewed Mar 21, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this on my same54-xpro but I got

main(): This is RIOT! (Version: 2022.04-devel-864-g8bfb6-HEAD)
Test application for SAM0 ethernet peripheral
Device 0 registered (type: 12)
Initialization successful - starting the shell now
> sys/event/event.c:38 => 0x795
*** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

Inside isr 84
Stack pointer corrupted, reset to top of stack
FSR/FAR:
 CFSR: 0x00000000
 HFSR: 0x80000000
 DFSR: 0x00000002
 AFSR: 0x00000000
Misc
EXC_RET: 0xfffffff1

when plugged into the local network

sys/include/test_utils/netdev_eth_minimal.h Outdated Show resolved Hide resolved
@leandrolanzieri
Copy link
Contributor Author

I tried this on my same54-xpro but I got

main(): This is RIOT! (Version: 2022.04-devel-864-g8bfb6-HEAD)
Test application for SAM0 ethernet peripheral
Device 0 registered (type: 12)
Initialization successful - starting the shell now
> sys/event/event.c:38 => 0x795
*** RIOT kernel panic:
FAILED ASSERTION.

*** halted.

Inside isr 84
Stack pointer corrupted, reset to top of stack
FSR/FAR:
 CFSR: 0x00000000
 HFSR: 0x80000000
 DFSR: 0x00000002
 AFSR: 0x00000000
Misc
EXC_RET: 0xfffffff1

when plugged into the local network

I think I figured it out. I don't have your hardware, but in my nucleo boards this was working, because netdev_register is called from the netdev->init functions instead of the driver setup, so the context is set to NULL after I already set it to the event.

@leandrolanzieri
Copy link
Contributor Author

@benpicco do you mind giving it another try?

@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Mar 22, 2022
@leandrolanzieri
Copy link
Contributor Author

There is still a crash when a bogus (negative error?) length is received and od_hex_dump attempts to print more memory than is physically available:

Thank you! Added your suggestion in 5b0b74c

@benpicco
Copy link
Contributor

Please squash

fjmolinas
fjmolinas reviewed Mar 24, 2022
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some static checks are failing

sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
maribu
maribu reviewed Mar 24, 2022
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me. Some style nitpicks inline. (Feel free to use better wording for the Doxygen stuff.)

drivers/include/w5100.h Outdated Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal.c Outdated Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
sys/test_utils/netdev_eth_minimal/netdev_eth_minimal_internal.h Show resolved Hide resolved
tests/driver_enc28j60/main.c Show resolved Hide resolved
tests/driver_esp_eth/Makefile.ci Outdated Show resolved Hide resolved
tests/driver_stm32_eth/main.c Outdated Show resolved Hide resolved
@leandrolanzieri leandrolanzieri force-pushed the pr/tests/eth_drivers_rework branch from ec83eb9 to c53fe1f Compare March 24, 2022 08:32
@github-actions github-actions bot removed the Area: CI Area: Continuous Integration of RIOT components label Mar 24, 2022
@leandrolanzieri
Copy link
Contributor Author

I think I have addressed all comments. I squashed and removed the commit that limited the CI

@leandrolanzieri leandrolanzieri force-pushed the pr/tests/eth_drivers_rework branch from c53fe1f to 3790843 Compare March 24, 2022 08:45
maribu
maribu reviewed Mar 24, 2022
View reviewed changes
tests/driver_enc28j60/Makefile.ci
Comment on lines +8 to +10
esp8266-esp-12x \
esp8266-olimex-mod \
esp8266-sparkfun-thing \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's odd. Maybe the linking issue is not due to memory limitation, but has other causes? If I recall correctly, even the old generation ESPs are pretty well equipped with RAM and flash.

@benpicco
Copy link
Contributor

btw: doesn't tests/driver_atwinc15x0 also fall into this category?

@maribu
Copy link
Member

maribu commented Mar 24, 2022

btw: doesn't tests/driver_atwinc15x0 also fall into this category?

That is IEEE 802.11, not IEEE 802.3.

@benpicco
Copy link
Contributor

btw: doesn't tests/driver_atwinc15x0 also fall into this category?

That is IEEE 802.11, not IEEE 802.3.

Yea but at the interface level we don't differentiate between WiFi and Ethernet yet

benpicco
benpicco approved these changes Mar 28, 2022
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me then.

@benpicco benpicco merged commit 1133d04 into RIOT-OS:master Mar 28, 2022
@leandrolanzieri leandrolanzieri deleted the pr/tests/eth_drivers_rework branch March 28, 2022 17:16
@leandrolanzieri
Copy link
Contributor Author

Thanks everyone for the review!

@OlegHahm OlegHahm added this to the Release 2022.04 milestone Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas left review comments

@kaspar030 kaspar030 kaspar030 left review comments

@maribu maribu maribu left review comments

@benpicco benpicco benpicco approved these changes

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@cgundogan cgundogan Awaiting requested review from cgundogan

@smlng smlng Awaiting requested review from smlng

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Milestone
Release 2022.04
Development

Successfully merging this pull request may close these issues.
6 participants
@leandrolanzieri @benpicco @maribu @kaspar030 @fjmolinas @OlegHahm