Skip to content

netdev: make netdev_% variants a submodule#13565

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
jia200x:pr/netdev_submodule
Sep 30, 2020
Merged

netdev: make netdev_% variants a submodule#13565
benpicco merged 2 commits intoRIOT-OS:masterfrom
jia200x:pr/netdev_submodule

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Mar 5, 2020

Contribution description

This PR makes netdev a module and add netdev_% variants as submodules.
This should help to add new netdev components (netdev_lora?) and helpers associated to netdev (and not a variant)

Testing procedure

Try compiling gnrc_networking for native and any IEEE802.15.4 board like samr21-xpro

Issues/PRs references

#13562

@jia200x jia200x requested review from benpicco and miri64 March 5, 2020 17:40
@jia200x jia200x added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: network Area: Networking labels Mar 5, 2020
@jia200x jia200x mentioned this pull request Mar 6, 2020
Closed
@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 Mar 6, 2020
benpicco
benpicco approved these changes Mar 6, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Nice cleanup, no functional changes.

@jia200x jia200x requested a review from kaspar030 March 6, 2020 13:08
kaspar030
kaspar030 approved these changes Mar 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

LGTM.

@jia200x jia200x force-pushed the pr/netdev_submodule branch from b8ae78f to 1652bd3 Compare March 6, 2020 13:12
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Mar 6, 2020

There was a whitespace. I fixed it and amended directly.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Mar 6, 2020

something broke ;-)

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Mar 9, 2020

I moved netdev_tap to the netdev folder. It should work now.

@jia200x jia200x force-pushed the pr/netdev_submodule branch from 9400ce3 to da1c9f1 Compare March 9, 2020 10:07
miri64
miri64 requested changes Mar 9, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

I moved netdev_tap to the netdev folder. It should work now.

Waitwaitwaitwait. That doesn't make sense at all! netdev_tap is not a submodule of netdev, it is a device driver. And it only works for native, so it should stay there.

miri64
miri64 previously requested changes Mar 9, 2020
View reviewed changes
drivers/Makefile.dep
USEMODULE += netif
endif

ifneq (,$(filter netdev_%,$(USEMODULE)))
Copy link
Copy Markdown
Member

@miri64 miri64 Mar 9, 2020

Choose a reason for hiding this comment

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

This line and ...

Copy link
Copy Markdown
Member Author

@jia200x jia200x Mar 9, 2020

Choose a reason for hiding this comment

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

I would keep this line anyway. In worst case it doesn't compile any module, but I plan to add some common helpers for netdev that might be used by netdev_tap too.

makefiles/pseudomodules.inc.mk Outdated
PSEUDOMODULES += mpu_stack_guard
PSEUDOMODULES += nanocoap_%
PSEUDOMODULES += netdev_default
PSEUDOMODULES += netdev_%
Copy link
Copy Markdown
Member

@miri64 miri64 Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

... this line are the actual problem. They are just declaring everything starting with netdev_ a submodule of netdev. However, netdev_tap is not. It's just a netdev simulator using the hosts TAP interface (where the name is basically coming from). I would prefer, to either name the submodules explicitly, or do it in a similar way that I already tried in #10970 (this tackles a complete different problem that is disjunct of this).

@jia200x jia200x force-pushed the pr/netdev_submodule branch from da1c9f1 to 1549fe7 Compare March 9, 2020 10:46
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Mar 9, 2020

I just added it to the "NO_PSEUDOMODULES" list and amended directly. I removed the netdev_tap commit.

@miri64 miri64 dismissed their stale review March 9, 2020 10:49

👍

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented May 14, 2020

It seems that there's consensus on this one. Just need a rebase!

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 22, 2020

ping @jia200x, please rebase!

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Sep 28, 2020

ping @jia200x, please rebase.

Just thinking of the netdev_tap module name, would it makes sense to call it native_tap instead ? This way it would be pretty clear that it's a "tap" driver and that it's only for native.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Sep 29, 2020 •
edited
Loading

Just thinking of the netdev_tap module name, would it makes sense to call it native_tap instead ? This way it would be pretty clear that it's a "tap" driver and that it's only for native.

Yes! Definitely. The netdev_tap is simply an implementation of a native driver that runs TAP, on top of netdev. I would be in favor of this.

@jia200x jia200x force-pushed the pr/netdev_submodule branch from 1549fe7 to 413ffbb Compare September 29, 2020 10:16
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Sep 29, 2020

I changed the logic a little bit. I'm not touching netdev_tap now (although I agree with @aabadie that we should change the name).

@leandrolanzieri leandrolanzieri mentioned this pull request Sep 30, 2020
Merged
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Sep 30, 2020

I piggybacked the fix here

benpicco
benpicco reviewed Sep 30, 2020
View reviewed changes
@jia200x jia200x force-pushed the pr/netdev_submodule branch from c50a081 to ad61f28 Compare September 30, 2020 14:29
@jia200x jia200x force-pushed the pr/netdev_submodule branch from ad61f28 to 8289612 Compare September 30, 2020 14:33
@jia200x jia200x added Area: network Area: Networking and removed Area: network Area: Networking labels Sep 30, 2020
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Sep 30, 2020

I refreshed the labels. It should be green soon :)

@benpicco benpicco merged commit f2a1043 into RIOT-OS:master Sep 30, 2020
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Oct 1, 2020

thanks for the review!

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

@kaspar030 kaspar030 kaspar030 approved these changes

@miri64 miri64 miri64 left review comments

@aabadie aabadie Awaiting requested review from aabadie

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jia200x @miri64 @aabadie @benpicco @kaspar030