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

make: introduce netdev_default pseudomodule #5134

Merged
merged 1 commit into from
Mar 24, 2016
Merged

make: introduce netdev_default pseudomodule #5134

merged 1 commit into from
Mar 24, 2016

Conversation

OlegHahm
Copy link
Member

Additionally the dependencies for GNRC specific modules are centralized in Makefile.dep.

Depends on #4646.

@OlegHahm OlegHahm added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: build system Area: Build system State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 22, 2016
@OlegHahm OlegHahm added this to the Release 2016.04 milestone Mar 22, 2016
@OlegHahm OlegHahm mentioned this pull request Mar 22, 2016
Merged
3 tasks
@miri64
Copy link
Member

miri64 commented Mar 22, 2016

Why the dependency to #4646?

@OlegHahm
Copy link
Member Author

Because otherwise I would have to update it afterwards. ;-)

@OlegHahm
Copy link
Member Author

btw. I'm not so sure if netif_default is a good name or somehow confusing.

@miri64
Copy link
Member

miri64 commented Mar 22, 2016

Maybe netdev2_default?

@kaspar030
Copy link
Contributor

@OlegHahm Could you summarize the module's intended use?

@OlegHahm
Copy link
Member Author

The idea is similar to gnrc_netif_default, just for the more generic (no pun intended) case where one wants to use the default network interface of a board without GNRC.

@OlegHahm
Copy link
Member Author

Maybe netdev2_default?

Well, it's not necessarily bound to netdev2, either. (Actually, the Phytec transceiver is not yet netdev2.) In theory we could name it netdev_default, since netdev2 is to be renamed soon anyway.

OlegHahm
OlegHahm reviewed Mar 22, 2016
View reviewed changes
Makefile.dep
@@ -12,6 +12,31 @@ ifneq (,$(filter nhdp,$(USEMODULE)))
USEMODULE += oonf_rfc5444
endif

ifneq (,$(filter gnrc_netif_default,$(USEMODULE)))
USEMODULE += gnrc_netif
USEMODULE += netif_default
Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably superfluous since the Makefile.dep of the board is evaluated before the general Makefile.dep here.

Copy link
Member

Choose a reason for hiding this comment

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

I would vote for the Makefile.dep to pull in the device on netif_default, not gnrc_netif_default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you rephrase?

Copy link
Member

Choose a reason for hiding this comment

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

E.g.

ifneq (,$(filter netif_default,$(USEMODULE)))
    USEMODULE += at86rf231
endif

instead of

E.g.

ifneq (,$(filter gnrc_netif_default,$(USEMODULE)))
    USEMODULE += at86rf231
endif

Copy link
Member

Choose a reason for hiding this comment

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

(or instead of)

ifneq (,$(filter netif_default gnrc_netif_default,$(USEMODULE)))
    USEMODULE += at86rf231
endif

as your fixes do, for that matter.

Copy link
Member

Choose a reason for hiding this comment

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

Yes! This is why it should not be removed, but the dependency of gnrc_netif_default in boards/*/Makefile.dep should be! :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

But if include gnrc_netif_default in my application's Makefile, the board wouldn't add the correct driver to the modules then.

Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would it know that it should?

Copy link
Member

Choose a reason for hiding this comment

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

Clarified offline. Now I see that it only is possible the way it is currently implemented.

@miri64
Copy link
Member

miri64 commented Mar 23, 2016

Maybe netdev2_default?

Well, it's not necessarily bound to netdev2, either. (Actually, the Phytec transceiver is not yet netdev2.) In theory we could name it netdev_default, since netdev2 is to be renamed soon anyway.

@miri64 miri64 closed this Mar 23, 2016
@miri64 miri64 reopened this Mar 23, 2016
@miri64
Copy link
Member

miri64 commented Mar 23, 2016

(the mobile interface of GitHub is shitty btw. One wrong tab with the thumb and the PR is closed -.-)

@miri64
Copy link
Member

miri64 commented Mar 23, 2016

Maybe netdev2_default?

Well, it's not necessarily bound to netdev2, either. (Actually, the Phytec transceiver is not yet netdev2.) In theory we could name it netdev_default, since netdev2 is to be renamed soon anyway.

This should have been edited above, but somehow the mobile interface did not let it through either... So for clarity again:

To me, netdev is a unifying term for both gnrc_netdev and netdev2, while netif is something that should sit on top of a netdev and may have some MAC implementation in between. So calling the pseudo-module netif_default is definitely wrong in my opinion, because it only pulls in the netdev => I would vote for calling it netdev_default right away.

@kaspar030
Copy link
Contributor

I would vote for calling it netdev_default right away.

For different reasons, but +1!

@OlegHahm OlegHahm added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 24, 2016
@OlegHahm OlegHahm changed the title make: introduce netif_default pseudomodule make: introduce netdev_default pseudomodule Mar 24, 2016
@OlegHahm
Copy link
Member Author

Rebased to master after #4646 got merged and renamed to *netdev_default.

@miri64
Copy link
Member

miri64 commented Mar 24, 2016

Tests seem to be working still, let's see what Murdock is saying. Please squash.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 24, 2016
@OlegHahm
make: introduce netif_default pseudomodule
10f5f1a 
Additionally the dependencies for GNRC specific modules are centralized in Makefile.dep.
@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 24, 2016
@OlegHahm
Copy link
Member Author

Squashed

@miri64 miri64 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 Mar 24, 2016
@miri64
Copy link
Member

miri64 commented Mar 24, 2016

You removed the NEEDS SQUASHING label to late for Murdock ;-)

@miri64
Copy link
Member

miri64 commented Mar 24, 2016

And go.

@miri64 miri64 merged commit 9dcb5cc into RIOT-OS:master Mar 24, 2016
@miri64 miri64 mentioned this pull request Mar 24, 2016
Closed
10 tasks
@OlegHahm OlegHahm deleted the netif_default branch March 24, 2016 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

Labels
Area: build system Area: Build system 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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2016.04
Development

Successfully merging this pull request may close these issues.
3 participants
@OlegHahm @miri64 @kaspar030