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

sys/net/gnrc_netif: Make use of confirm send #18139

Merged
merged 2 commits into from
Sep 17, 2022
Merged

sys/net/gnrc_netif: Make use of confirm send #18139

merged 2 commits into from
Sep 17, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented May 25, 2022 •
edited
Loading

Contribution description

  • Introduce modules netdev_legacy_api and netdev_new_api to allow optimizing for the "with confirm_send()" and "without confirm_send()" variant of the netdev API
    • Each netdev driver has to depend on exactly one of those
    • If both are used (because two or more netdevs are in use and one is migrated and the other not), the API version is checked at runtime
  • With netdev_new_api translate NETDEV_EVENT_TX_COMPLETE into an sys/event event, so that this can be signaled from ISR. This safes a roundtrip to the ISR handler if the type of the event is known at ISR time (e.g. multiple signal pins or peripheral netdev)
  • If the new API is used, wait for the NETDEV_EVENT_TX_COMPLETE event before signalling upper layers completion with gnrc_tx_sync.

Testing procedure

This needs to be tested in conjunction with a driver actually being ported to the new API. However, we cannot port drivers to the new API until both GNRC and lwIP correctly operate with the new API.

#18428 ports the STM32 Ethernet driver to the new API, so that this PR can be tested there.

Issues/PRs references

Similar to #15821 but uses the now existing sys/event infrastructure rather than thread_flags

fixes #7474

@maribu maribu added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 25, 2022
@github-actions github-actions bot added Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support Area: sys Area: System Platform: ARM Platform: This PR/issue effects ARM-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms labels May 25, 2022
@maribu maribu force-pushed the sys/net/gnrc/netif/confirm_send branch from bc18120 to 47f807f Compare July 22, 2022 08:10
@benpicco
Copy link
Contributor

Does the WIP label still apply?

@maribu
Copy link
Member Author

maribu commented Jul 22, 2022

This could be merged if I drop the Ethernet change, as that change will break lwIP. I am currently working on the same thing for lwIP, after which driver could safely start making use of confirm_send.

@maribu maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 9, 2022
This was referenced Aug 9, 2022
Merged
Merged
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 18, 2022
@benpicco
Copy link
Contributor

This needs a rebase

@maribu maribu force-pushed the sys/net/gnrc/netif/confirm_send branch from 21d2f01 to eb21b20 Compare August 22, 2022 09:02
@github-actions github-actions bot removed Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: build system Area: Build system Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: LoRa Area: LoRa radio support labels Aug 22, 2022
@maribu
Copy link
Member Author

maribu commented Sep 14, 2022

Ping :)

benpicco
benpicco reviewed Sep 14, 2022
View reviewed changes
sys/net/gnrc/netif/ethernet/gnrc_netif_ethernet.c Show resolved Hide resolved
sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved
sys/net/gnrc/netif/gnrc_netif.c Show resolved Hide resolved
sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved
@maribu maribu force-pushed the sys/net/gnrc/netif/confirm_send branch from eb21b20 to 21c0969 Compare September 15, 2022 11:02
@benpicco
Copy link
Contributor

Please squash!

Marian Buschsieweke added 2 commits September 15, 2022 13:23
sys/net/gnrc/netif: provide support for confirm_send()
1cb6f9e 
This adds support for netdevs implementing the new API that provides
`netdev_driver_t::confirm_send()`. This allows implementing netdevs
in an event based non-blocking fashion, making live of driver
developers a bit easier. In addition, `gnrc_tx_sync` will now throttle
users of `sock_udp_send()` so that they can only send datagrams as
fast as the network stack and hardware is able to send out.

Finally, this lays the groundwork to fetch TX statistics (such as
TX timestamps, reception of layer 2 ACKs/NACKs, etc.) from the network
devices.
sys/net/gnrc/netif: adapt to new API
581f35e 
For Ethernet, raw netifs, and IEEE 802.15.4 netifs only release outgoing
frame with legacy drivers, as gnrc_netif does so with new non-blocking
API.
@maribu maribu force-pushed the sys/net/gnrc/netif/confirm_send branch from 460a279 to 581f35e Compare September 15, 2022 11:23
@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 15, 2022
@maribu
Copy link
Member Author

maribu commented Sep 16, 2022

All green. If anyone wants to have another look before I hit merge, it is now or never ;)

@maribu maribu mentioned this pull request Sep 16, 2022
Closed
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 17, 2022
@maribu maribu enabled auto-merge September 17, 2022 13:15
@maribu maribu merged commit 8457f09 into RIOT-OS:master Sep 17, 2022
@maribu
Copy link
Member Author

maribu commented Sep 17, 2022

Thx everyone :)

@maribu maribu deleted the sys/net/gnrc/netif/confirm_send branch September 17, 2022 18:23
@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
Reviewers

@benpicco benpicco benpicco approved these changes

@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

@cgundogan cgundogan Awaiting requested review from cgundogan

@aabadie aabadie Awaiting requested review from aabadie

@jia200x jia200x Awaiting requested review from jia200x

@bergzand bergzand Awaiting requested review from bergzand

@jue89 jue89 Awaiting requested review from jue89

@daniel-k daniel-k Awaiting requested review from daniel-k

@smlng smlng Awaiting requested review from smlng

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@vincent-d vincent-d Awaiting requested review from vincent-d

@dylad dylad Awaiting requested review from dylad

@keestux keestux Awaiting requested review from keestux

@gschorcht gschorcht Awaiting requested review from gschorcht

Assignees
No one assigned
Labels
Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2022.10
Development

Successfully merging this pull request may close these issues.

6lo gnrc fragmentation expects driver to block on TX
2 participants
@maribu @benpicco