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: fix adaption to API change of netdev_new_api #21091

Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 16, 2024

Contribution description

Since #21012 a netdev in the new API can return > 0 directly in netdev_driver_t::send() to indicate the driver is naturally synchronous and has already completed the transmission.

The adaption of lwIP to that API change contained a bug: It handled the case after the thread is already blocked waiting for the signal that is never going to arrive. This is now fixed.

Testing procedure

In master

$ make BOARD=native64 -C tests/pkg/lwip -j && tests/pkg/lwip/bin/native64/tests_lwip.elf tap1
[...]
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2025.01-devel-318-g4044c8)
RIOT lwip test application
> ifconfig
ifconfig
Iface ET0 HWaddr: 26:de:d0:85:bd:b2 Link: up State: up
        Link type: wired
        inet6 addr: fe80:0:0:0:24de:d0ff:fe85:bdb2 scope: link state: tentative (1 probes send)
$  make BOARD=native64 -C examples/gnrc_networking flash term -j
[...]
2024-12-16 14:58:22,260 # RIOT native interrupts/signals initialized.
2024-12-16 14:58:22,261 # RIOT native board initialized.
2024-12-16 14:58:22,261 # RIOT native hardware initialization complete.
2024-12-16 14:58:22,261 # 
2024-12-16 14:58:22,261 # main(): This is RIOT! (Version: 2025.01-devel-318-g4044c8)
2024-12-16 14:58:22,261 # RIOT network stack example application
2024-12-16 14:58:22,261 # All up, running the shell now
> ping fe80:0:0:0:24de:d0ff:fe85:bdb2
2024-12-16 14:58:23,914 # ping fe80:0:0:0:24de:d0ff:fe85:bdb2
2024-12-16 14:58:26,915 # 
2024-12-16 14:58:26,916 # --- fe80:0:0:0:24de:d0ff:fe85:bdb2 PING statistics ---
2024-12-16 14:58:26,916 # 3 packets transmitted, 0 packets received, 100% packet loss

This PR

$ make BOARD=native64 -C tests/pkg/lwip -j && tests/pkg/lwip/bin/native64/tests_lwip.elf tap1
[...]
RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2025.01-devel-319-g5ea74-pkg/lwip/adapt-to-netdev_new_new_api)
RIOT lwip test application
> ifconfig
ifconfig
Iface ET0 HWaddr: 26:de:d0:85:bd:b2 Link: up State: up
        Link type: wired
        inet6 addr: fe80:0:0:0:24de:d0ff:fe85:bdb2 scope: link state: valid preferred
$ make BOARD=native64 -C examples/gnrc_networking flash term -j
[...]
2024-12-16 14:59:14,001 # RIOT native board initialized.
2024-12-16 14:59:14,001 # RIOT native hardware initialization complete.
2024-12-16 14:59:14,001 # 
2024-12-16 14:59:14,002 # main(): This is RIOT! (Version: 2025.01-devel-319-g5ea74-pkg/lwip/adapt-to-netdev_new_new_api)
2024-12-16 14:59:14,002 # RIOT network stack example application
2024-12-16 14:59:14,002 # All up, running the shell now
> ping fe80:0:0:0:24de:d0ff:fe85:bdb2
2024-12-16 14:59:18,610 # ping fe80:0:0:0:24de:d0ff:fe85:bdb2
2024-12-16 14:59:18,611 # 12 bytes from fe80::24de:d0ff:fe85:bdb2%6: icmp_seq=0 ttl=255 time=0.550 ms
2024-12-16 14:59:19,612 # 12 bytes from fe80::24de:d0ff:fe85:bdb2%6: icmp_seq=1 ttl=255 time=0.605 ms
2024-12-16 14:59:20,612 # 12 bytes from fe80::24de:d0ff:fe85:bdb2%6: icmp_seq=2 ttl=255 time=0.522 ms
2024-12-16 14:59:20,612 # 
2024-12-16 14:59:20,613 # --- fe80:0:0:0:24de:d0ff:fe85:bdb2 PING statistics ---
2024-12-16 14:59:20,613 # 3 packets transmitted, 3 packets received, 0% packet loss
2024-12-16 14:59:20,613 # round-trip min/avg/max = 0.522/0.559/0.605 ms

Issues/PRs references

#21012

@maribu maribu requested a review from miri64 as a code owner December 16, 2024 14:00
@maribu maribu requested a review from benpicco December 16, 2024 14:00
@github-actions github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports labels Dec 16, 2024
@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 16, 2024
@maribu maribu force-pushed the pkg/lwip/adapt-to-netdev_new_new_api branch 2 times, most recently from 0fb00bd to 04589cd Compare December 16, 2024 14:09
@maribu
Copy link
Member Author

maribu commented Dec 16, 2024

I tested again after the last force push, still works as before.

I originally assumed that this was just not adapted to the new API; it was, but just a few lines where it should have come.

@maribu maribu changed the title pkg/lwip: adapt to API change of netdev_new_api pkg/lwip: fix adaption to API change of netdev_new_api Dec 16, 2024
Since RIOT-OS#21012 a netdev in the new API
can return > 0 directly in netdev_driver_t::send() to indicate the
driver is naturally synchronous and has already completed the
transmission.

The adaption of lwIP to that API change contained a bug: It handled the
case after the thread is already blocked waiting for the signal that
is never going to arrive. This is now fixed.
@maribu maribu force-pushed the pkg/lwip/adapt-to-netdev_new_new_api branch from 04589cd to 424eae0 Compare December 16, 2024 14:11
@maribu
Copy link
Member Author

maribu commented Dec 16, 2024

And another force-push: Rewording the commit title. (No retesting required, as no code changed.)

@riot-ci
Copy link

riot-ci commented Dec 16, 2024

Murdock results

✔️ PASSED

424eae0 pkg/lwip: fix adaption to API change of netdev_new_api

Success Failures Total Runtime
10249 0 10249 15m:55s

Artifacts

@benpicco
Copy link
Contributor

benpicco commented Dec 16, 2024

You can also drop the if (res < 0) { case on line 322, this can never be reached.

@maribu
Copy link
Member Author

maribu commented Dec 16, 2024

It could be set to an error other than -EAGAIN by confirm_send() in

while (-EAGAIN == (res = netdev->driver->confirm_send(netdev, NULL))) {

@benpicco benpicco enabled auto-merge December 16, 2024 14:37
@benpicco benpicco added this pull request to the merge queue Dec 16, 2024
Merged via the queue into RIOT-OS:master with commit a40852d Dec 16, 2024
26 checks passed
@maribu maribu deleted the pkg/lwip/adapt-to-netdev_new_new_api branch December 16, 2024 19:22
@maribu
Copy link
Member Author

maribu commented Dec 16, 2024

Thx 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants