Skip to content

netdev: Rework netdev_eth to layered structure#9330

Closed
bergzand wants to merge 14 commits intoRIOT-OS:masterfrom
bergzand:pr/netdev/layer/ethernet
Closed

netdev: Rework netdev_eth to layered structure#9330
bergzand wants to merge 14 commits intoRIOT-OS:masterfrom
bergzand:pr/netdev/layer/ethernet

Conversation

@bergzand
Copy link
Copy Markdown
Member

Contribution description

This PR changes the netdev_eth module to a layered structure. The change here is that the netdev_driver_t functions of the netdev_eth struct are called first, and the functions of the hardware driver are called next.

The hardware drivers are adapted to not call the netdev_eth get and set functions anymore.

The hwdev struct member in the netdev_eth_t is only required to handle the L2 network stats and can be removed as soon as the stats are moved to a separate layer.

@miri64 Could it be that I'm also breaking the other (non-gnrc) network stacks here?

Issues/PRs references

Depends on #9329

@bergzand bergzand added Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Jun 12, 2018
@bergzand bergzand requested a review from miri64 June 12, 2018 10:30
@bergzand bergzand added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 12, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 12, 2018

@miri64 Could it be that I'm also breaking the other (non-gnrc) network stacks here?

The only stack effected by this PR could be lwip (all the other stacks don't support Ethernet to my knowledge, but I'm not sure about OpenThread. @jia200x?)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 12, 2018

It definitely breaks lwip. It tests/lwip segfaults (reason at the moment unclear), which it definitely doesn't do in master.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jun 12, 2018

The only stack effected by this PR could be lwip (all the other stacks don't support Ethernet to my knowledge, but I'm not sure about OpenThread. @jia200x?)

OpenThread only uses raw netdev. So, shouldn't break anything there

dev->context = netif;
/* register the event callback with the device driver */
dev->event_callback = _event_cb;
dev->context = netif;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still unrelated move?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ping?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I have to check this, probably still unrelated, but I have a hunch that I switched the order intentionally

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Always document your work. Future you and your peers will thank you. ;-)

@bergzand
Copy link
Copy Markdown
Member Author

Adapted LwIP to this PR

netdev_tap_setup(&netdev_taps[i], &netdev_tap_params[i]);
if (netif_add(&netif[i], &netdev_taps[i], lwip_netdev_init,
netdev_eth_add((netdev_t*)&netdev_taps[i], &eth_layer[i]);
if (netif_add(&netif[i], &eth_layer[i], lwip_netdev_init,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah... easier than I anticipated ^^

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, but not a solution that scales nicely. If we ever start supporting more devices we might want to refactor this to something that scales.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yupp, but then lwip_netdev_init needs some heavy refactoring anyways.

@stale
Copy link
Copy Markdown

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@stale stale bot closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: stale State: The issue / PR has no activity for >185 days State: waiting for other PR State: The PR requires another PR to be merged first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants