Skip to content

auto_init: upgrade bootstrap of Ethernet devices for gnrc_netif2#7410

Merged
cgundogan merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_netif2/feat/ethernet
Oct 10, 2017
Merged

auto_init: upgrade bootstrap of Ethernet devices for gnrc_netif2#7410
cgundogan merged 1 commit intoRIOT-OS:masterfrom
miri64:gnrc_netif2/feat/ethernet

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Jul 25, 2017 •
edited
Loading

Bootstraps all ethernet devices for gnrc_netif2 (non-intrusively)

Depends on #7370.

This PR is part of the network layer remodelling effort:
PR dependencies

@miri64 miri64 added GNRC Area: network Area: Networking State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 25, 2017
@miri64 miri64 requested a review from kaspar030 July 25, 2017 20:50
miri64 added a commit to miri64/RIOT that referenced this pull request Jul 27, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into HEAD
c312e15
@miri64 miri64 mentioned this pull request Jul 27, 2017
Merged
@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from e6d19a3 to f59e643 Compare July 30, 2017 18:42
miri64 added a commit to miri64/RIOT that referenced this pull request Jul 31, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…
d4058f2 
…#7424
@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from f59e643 to 236627b Compare August 1, 2017 14:12
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 1, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…
70ed025 
…#7424
@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from 236627b to 382ffd7 Compare August 21, 2017 10:52
miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017
@miri64
Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…
3318d8c 
… into RIOT-OS#7410
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Aug 21, 2017

Rebased to current master and current dependencies

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS#…
231d4dd 
…7424
miri64 added a commit to miri64/RIOT that referenced this pull request Oct 6, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…
4ced871 
…#7424
@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from d08e411 to c7fff90 Compare October 10, 2017 08:50
miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017
@miri64
Merge branch 'gnrc_netif2/feat/new-netif-api-for-gnrc' (RIOT-OS#7370)…
7d897c4 
… into RIOT-OS#7410
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Rebased to current master and dependencies.

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…
0e268f2 
…#7424
@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from c7fff90 to e98df3c Compare October 10, 2017 09:46
@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 State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 10, 2017
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Rebased to current master, no longer waiting for other PR

@miri64 miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2017
bergzand
bergzand reviewed Oct 10, 2017
View reviewed changes
sys/auto_init/netif/auto_init_netdev_tap.c Outdated

/**
* @ingroup auto_init_gnrc_netif
* @ingroup auto_init_netif
Copy link
Copy Markdown
Member

@bergzand bergzand Oct 10, 2017

Choose a reason for hiding this comment

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

Is there a reason why the @ingroup is only changed here?

Copy link
Copy Markdown
Member Author

@miri64 miri64 Oct 10, 2017

Choose a reason for hiding this comment

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

Not sure...both groups don't exist... will revert this change and we figure out something else in a separate PR.

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 10, 2017
@miri64
Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…
096d719 
…#7424
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Oct 10, 2017 •
edited
Loading

I can't spot any other obvious errors and the test looks working. (Edit: no test here, confusing this one with #7409)

The defines of the w5100 are not scoped like the other drivers, but that's probably better solved in a different PR :)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

@kaspar030 made some comments about my usage of LOG_ERROR(). I will remove them here and in the other PRs.

@miri64 miri64 mentioned this pull request Oct 10, 2017
Merged
This was referenced Oct 10, 2017
Merged
Merged
Merged
Merged
smlng
smlng reviewed Oct 10, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

Compile tested with adapted tests/driver_enc28j60, seems to work but no real testing. Pre-Ack

@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from de9bba4 to 7ebe533 Compare October 10, 2017 19:39
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Rebased to current master

@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Oct 10, 2017

Squash, please.

@miri64 miri64 force-pushed the gnrc_netif2/feat/ethernet branch from 7ebe533 to 4083640 Compare October 10, 2017 19:49
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Done

@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 Oct 10, 2017
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Oct 10, 2017

@kaspar030 made some comments about my usage of LOG_ERROR(). I will remove them here and in the other PRs.

Are these comments written down anywhere?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

Nope, but it is already changed to the way he wanted it.

cgundogan
cgundogan approved these changes Oct 10, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

ACK

@cgundogan cgundogan merged commit 1f03862 into RIOT-OS:master Oct 10, 2017
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Oct 10, 2017

The whole auto_init stuff has a bad smell of code duplication, but this is not the meat of this PR.

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Oct 10, 2017

Nope, but it is already changed to the way he wanted it.

And that is?

@miri64 miri64 deleted the gnrc_netif2/feat/ethernet branch October 10, 2017 21:16
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Oct 10, 2017

The whole auto_init stuff has a bad smell of code duplication, but this is not the meat of this PR.

No one likes it (including me), but for now it's the most straightforward way. I'm working on simplify it and remove all those ugly ifdefs as soon all this NIB/GNRC-Netif2 stuff is in master and the release is in feature freeze.

Nope, but it is already changed to the way he wanted it.

And that is?

That the return value of gnrc_netif2.*_create() isn't checked to potentially cause a LOG_ERROR(). gnrc_netif2.*_create() can only fail if GNRC_NETIF_NUMOF wasn't set correctly, so we just let it crash in that case.

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Oct 10, 2017

Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand left review comments

@cgundogan cgundogan cgundogan approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

+1 more reviewer

@smlng smlng smlng left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@kaspar030 kaspar030

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@miri64 @bergzand @cgundogan @OlegHahm @smlng @kaspar030