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

tree-wide: Introduce netif feature and use it #20682

Merged
merged 2 commits into from
May 22, 2024
Merged

tree-wide: Introduce netif feature and use it #20682

merged 2 commits into from
May 22, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented May 21, 2024

Contribution description

This gets rid of a long list of boards with network interfaces and instead let's boards (or MCUs with peripheral network interfaces) provide the netif feature.

The apps that before used the long list are not depending on the feature instead (in case of the default example, this is an optional dependency).

Testing procedure

Ideally, no change in binaries. There may be some boards that actually do have a network interface but were not listed in Makefile.boards.netif that now qualify as having a network interface, if the feature was provided on MCU level (only for peripheral netifs).

Issues/PRs references

Partially fixes #20680

@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 May 21, 2024
@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: build system Area: Build system Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: boards Area: Board ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications labels May 21, 2024
@riot-ci
Copy link

riot-ci commented May 21, 2024 •
edited
Loading

Murdock results

✔️ PASSED

77dfc7e boards/esp32*: move saul_gpio dep to board level

Success Failures Total Runtime
10102 0 10104 13m:33s

Artifacts

mguetschow
mguetschow requested changes May 22, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks for that! I haven't compared the removed list of boards vs. added FEATURES_PROVIDED in detail, but I'd trust you to have done that thoroughly enough - in the end it is only relevant for apps/tests using the default board features.

Just a minor addition below.

boards/common/nrf52/Makefile.features Show resolved Hide resolved
@bmewen
Copy link
Contributor

bmewen commented May 22, 2024 •
edited
Loading

Sounds good to me, I tested it with a dwm1001, an arduino nano 33 ble (and the sense one) and didn't notice any unexpected behavior on the default example.
The board list also seems to be compliant with what was existing.

My only question is about the following line :

RIOT/examples/default/Makefile

Line 49 in 0641ab2

ifneq (,$(filter $(BOARD),$(BOARD_PROVIDES_NETIF)))

Since you added a feature it should be the same as this one I think :

RIOT/examples/default/Makefile.board.dep

Line 1 in 0641ab2

ifneq (,$(filter netif,$(FEATURES_USED)))

@maribu @mguetschow
tree-wide: Introduce netif feature and use it
97a6543 
This gets rid of a long list of boards with network interfaces and
instead let's boards (or MCUs with peripheral network interfaces)
provide the netif feature.

The apps that before used the long list are not depending on the
feature instead (in case of the default example, this is an
optional dependency).

Co-authored-by: mguetschow <mikolai.guetschow@tu-dresden.de>
Co-authored-by: mewen.berthelot <mewen.berthelot@orange.com>
@maribu maribu force-pushed the buildsystem/netif-feature branch from 5264a06 to 97a6543 Compare May 22, 2024 08:41
@maribu
Copy link
Member Author

maribu commented May 22, 2024

Sounds good to me, I tested it with a dwm1001, an arduino nano 33 ble (and the sense one) and didn't notice any unexpected behavior on the default example. The board list also seems to be compliant with what was existing.

My only question is about the following line :

RIOT/examples/default/Makefile

Line 49 in 0641ab2

ifneq (,$(filter $(BOARD),$(BOARD_PROVIDES_NETIF)))

Since you added a feature it should be the same as this one I think :

RIOT/examples/default/Makefile.board.dep

Line 1 in 0641ab2

ifneq (,$(filter netif,$(FEATURES_USED)))

Indeed! Good catch :)

@maribu maribu marked this pull request as ready for review May 22, 2024 08:41
@maribu maribu added this pull request to the merge queue May 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 22, 2024
@maribu
boards/esp32*: move saul_gpio dep to board level
77dfc7e 
The ESP32 Ethernet Kit has only a single GPIO pin exposed to SAUL via
`saul_gpio`, but that GPIO doubles as PHY clock input when `esp32_eth`
is used. Hence, the `saul_gpio` dep should be optional and only kick
in when `esp32_eth` is not used.
@maribu
Copy link
Member Author

maribu commented May 22, 2024

Looks like the esp32-ethernet-kit* failed to build with esp32_eth and saul_default in use, but this was never triggered in the CI in master because they were not in BOARD_PROVIDES_NETIF.

The second commit fixes that issue.

mguetschow
mguetschow approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Good catch, thanks @maribu!

@mguetschow mguetschow enabled auto-merge May 22, 2024 12:25
@mguetschow mguetschow added this pull request to the merge queue May 22, 2024
Merged via the queue into RIOT-OS:master with commit fc78e1f May 22, 2024
26 checks passed
@mguetschow mguetschow added this to the Release 2024.07 milestone Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mguetschow mguetschow mguetschow approved these changes

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

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

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

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

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

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

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

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications 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 Platform: AVR Platform: This PR/issue effects AVR-based platforms Platform: ESP Platform: This PR/issue effects ESP-based platforms Platform: native Platform: This PR/issue effects the native platform Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2024.07
Development

Successfully merging this pull request may close these issues.

test files required to compile
4 participants
@maribu @riot-ci @bmewen @mguetschow