Skip to content

sys/net/gnrc/netif: allow checking if a netdev is legacy or new API #18426

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

Merged
merged 1 commit into from
Aug 17, 2022

Conversation

maribu
Copy link
Member

@maribu maribu commented Aug 9, 2022

Contribution description

A if netdev_driver_t::confirm_send() is provided, it provides the new netdev API. However, detecting the API at runtime and handling both API styles comes at a cost. This can be optimized in case only new or only old style netdevs are in use.

To do so, this adds the pseudo modules netdev_legacy_api and netdev_new_api. As right now no netdev actually implements the new API, all netdevs pull in netdev_legacy_api. If netdev_legacy_api is in used but netdev_new_api is not, we can safely assume at compile time that only legacy netdevs are in use. Similar, if only netdev_new_api is used, only support for the new API is needed. Only when both are in use, run time checks are needed.

This provides two helper function to check for a netif if the corresponding netdev implements the old or the new API. (With one being the inverse of the other.) They are suitable for constant folding when only new or only legacy devices are in use. Consequently, dead branches should be eliminated by the optimizer.

Testing procedure

The provided functions are not actually used, so there is not really anything to test. It was only split out as the lwIP and the GNRC adaption to make use of netdev_driver_t::confirm_send() both need this and this should be trivial to review.

Issues/PRs references

None

@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: network Area: Networking 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 Aug 9, 2022
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Aug 10, 2022
@benpicco
Copy link
Contributor

CI is not happy and a rebase is in order

@maribu maribu force-pushed the sys/net/gnrc/netif/api_check branch from 5a0145c to 6bba244 Compare August 11, 2022 11:57
@maribu maribu force-pushed the sys/net/gnrc/netif/api_check branch 2 times, most recently from c212344 to 48a0841 Compare August 11, 2022 20:25
@github-actions github-actions bot removed the Area: tests Area: tests and testing framework label Aug 11, 2022
@maribu
Copy link
Member Author

maribu commented Aug 11, 2022

It seems to not be easy to enforce that either netdev_legacy_api or netdev_new_api is in use, as e.g. examples/gcoap can be used with boards that do not have any netdev (and hence never pull in either). I adjusted the check to assume the legacy API in absence of either module. That should also users of out-of-tree netdevs happy, for now.

@maribu maribu force-pushed the sys/net/gnrc/netif/api_check branch 2 times, most recently from 5c7ff85 to 2104e4c Compare August 16, 2022 13:59
@maribu maribu force-pushed the sys/net/gnrc/netif/api_check branch from 2104e4c to cc4c34e Compare August 16, 2022 14:49
@maribu
Copy link
Member Author

maribu commented Aug 17, 2022

All green now :)

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.

We can skip the compile test if you fix he doc error

A if `netdev_driver_t::confirm_send()` is provided, it provides the
new netdev API. However, detecting the API at runtime and handling
both API styles comes at a cost. This can be optimized in case only
new or only old style netdevs are in use.

To do so, this adds the pseudo modules `netdev_legacy_api` and
`netdev_new_api`. As right now no netdev actually implements the new
API, all netdevs pull in `netdev_legacy_api`. If `netdev_legacy_api` is
in used but `netdev_new_api` is not, we can safely assume at compile
time that only legacy netdevs are in use. Similar, if only
`netdev_new_api` is used, only support for the new API is needed. Only
when both are in use, run time checks are needed.

This provides two helper function to check for a netif if the
corresponding netdev implements the old or the new API. (With one
being the inverse of the other.) They are suitable for constant folding
when only new or only legacy devices are in use. Consequently, dead
branches should be eliminated by the optimizer.
@maribu maribu added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Aug 17, 2022
@maribu maribu force-pushed the sys/net/gnrc/netif/api_check branch from 376c643 to 276ad57 Compare August 17, 2022 10:56
@maribu
Copy link
Member Author

maribu commented Aug 17, 2022

Once again, all green :)

@MrKevinWeiss
Copy link
Contributor

It looks like many new changes were introduced since it was last compiled, are you sure we should skip the compile check?

@maribu
Copy link
Member Author

maribu commented Aug 17, 2022

The last run was 3 hours ago and I see no merges within the last three hours to master.

@benpicco
Copy link
Contributor

benpicco commented Aug 17, 2022

A full CI run is 3h now and will randomly fail, currently it's best to avoid them if possible.

@benpicco benpicco merged commit 2321841 into RIOT-OS:master Aug 17, 2022
@benpicco
Copy link
Contributor

benpicco commented Aug 17, 2022

Urgh looks like @MrKevinWeiss was right, sorry about that 😨

@maribu
Copy link
Member Author

maribu commented Aug 18, 2022

There were 5 hours between the last full CI run and the merge. The second CI run with build tests skipped came 4 hours after the full CI run and different in a typo fix of documentation.

Unless we allow only fast forward merges like in release branches, or use GitLab style merge trains, or use bors, things like this will happen. The issue is not that the build tests were skipped. If there would be no typo and no rebuild with skipped build tests, a 5 hours old Murdock result would have been considered good enough and this would have been merged just the same.

@benpicco
Copy link
Contributor

I'm a bit surprised this wasn't caught by CI in the last build, nothing about that code has changed.

@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@maribu maribu deleted the sys/net/gnrc/netif/api_check branch January 21, 2023 21:44
elenaf9 added a commit to elenaf9/RIOT that referenced this pull request Jul 23, 2025
Nimble still implements the legacy netdev API.
The `netdev_legacy_api` must be explicitly used, otherwise other network
drivers that use the new API will overwrite it also for nimble.

See RIOT-OS#18426.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: LoRa Area: LoRa radio support 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 CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Platform: ARM Platform: This PR/issue effects ARM-based platforms 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