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.

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

pkg/lwip: Set netdev callback before driver init #18548

Merged
merged 2 commits into from
Sep 14, 2022

Conversation

yarrick
Copy link
Contributor

@yarrick yarrick commented Sep 3, 2022

Contribution description

lwIP did not set the netdev callback early enough, so events triggered during or right after initialization were lost.

After the IDF upgrade my board could not receive packets via Ethernet anymore since the cable connect event id was still stored as the pending event. This had not been processed by the ISR due to the missing callback, and the stored event id stopped other events from triggering.

This explains why the connect/rx event clash seen in "esp32/eth: Don't overwrite queued event with RX packet" (95196fb) only happened with lwIP. The fix for that is no longer needed and therefore reverted.

Testing procedure

tests/lwip still works and interfaces can get IPv4 or IPv6 address dynamically (meaning RX & TX works).

Issues/PRs references

Broken for me since #17601. Reverting #16084

Otherwise if the cable is connected at boot the immediate NETDEV_EVENT_ISR
event signaling connection will not trigger the isr, and the netdev will not
clear its pending event.

This explains why the connect/rx event clash seen in "esp32/eth: Don't
overwrite queued event with RX packet" (95196fb) only happened
with lwIP.

Now on my ESP32 board with Ethernet the issue was the opposite (since IDF
upgrade), the stuck connected event blocked receive from working. After
this change 95196fb can be reverted since even early events are
consumed properly.
This reverts commit 95196fb.

Not needed since the initial event is now processed properly.
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Area: network Area: Networking Area: pkg Area: External package ports Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Sep 3, 2022
@gschorcht
Copy link
Contributor

I didn't test esp_eth together with lwip in PR #17601. esp_eth worked without any problem wit hexamples/gnrc_networking.

Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Code looks good. I don't have the hardware with me, I can test it next week.

@benpicco benpicco requested a review from jia200x September 3, 2022 17:43
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 3, 2022
@gschorcht
Copy link
Contributor

I could reproduce the behavior without this PR. It works with this PR.

@benpicco benpicco merged commit 276195e into RIOT-OS:master Sep 14, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants