gnrc_netif: Add bootstrap function for device setup by bergzand · Pull Request #9329 · RIOT-OS/RIOT

bergzand · 2018-06-12T09:17:48Z

Contribution description

This adds a bootstrap function to the gnrc_netif initialization. This bootstrap function can be used to initialize a layered netdev structure. This way the required structures can be kept on the stack of the netif thread while keeping the total structure somewhat dynamic.

For now the bootstrap functions are empty. They are used as soon as there are separate netdev layers that require them.

Issues/PRs references

next step of #8198

jnohlgard · 2018-06-12T09:42:39Z

This is something that could be useful in a project I am working on. Thanks @bergzand!

jnohlgard · 2018-06-12T09:43:42Z

sys/net/gnrc/netif/gnrc_netif.c

+    dev->context = netif;
    /* register the event callback with the device driver */
    dev->event_callback = _event_cb;
-    dev->context = netif;


Unrelated move?

miri64

Some initial comments

miri64 · 2018-06-12T09:43:23Z

sys/include/net/gnrc/netif.h

+ *
+ * @param[in] args   The network interface
+ */
+void *gnrc_netif_thread(void *args);


Mhhh... Can we somehow get around exposing this function?

Would it be okay to have it in sys/include/net/gnrc/netif/internal.h?

Maybe you can use a trampoline function somehow?

miri64 · 2018-06-12T09:43:30Z

sys/include/net/gnrc/netif.h

+ *
+ * @param[in] args   The network interface
+ */
+void *gnrc_netif_thread(void *args);


Line below missing.

miri64 · 2018-06-12T09:45:35Z

sys/include/net/gnrc/netif.h

 typedef struct {
    const gnrc_netif_ops_t *ops;            /**< Operations of the network interface */
    netdev_t *dev;                          /**< Network device of the network interface */
+    netdev_t *hwdev;                        /**< Hardware layer of the network device */


Either change name of hwdev or the name and doc on dev (that is the top-layer of netdev). hwdev can be misleading if you have "virtual" devices.

miri64 · 2018-06-12T09:46:25Z

sys/net/gnrc/netif/gnrc_netif_ieee802154.c

+static void *_bootstrap(void *args)
+{
+    gnrc_netif_t *netif = args;
+    netif->hwdev = netif->dev;


Why don't netif_ethernet or netif_raw require this change?

gnrc_netif_ieee802154.c requires access to the ieee802154_netdev struct, for the pan_id, and sequence numbers and more. The other mac layers don't require this.

If we can build a getter or something for this we can omit the whole hwdev member here altogether (solving the issue above too)

Mh... if that is possible, I would prefer that. Then most of the changes in #9330 wouldn't be necessary also.

By "make a separate layer" I mean move the sequence number, pan id, lladdr etc to a gnrc_netif_ieee802154 internal variables, or a proper layer, whichever is most feasible

@gebart I would like to stick with the proposed solution of doing a get() call for the struct for now. I agree with your solution, but I'd rather not rework half of this PR at the moment, but do it in a follow up afterwards.

@miri64 I've implemented a getter in #9335

Sounds good. A future possible TSCH implementation needs to use a different frame format than the traditional 802.15.4 frames use, so it would be a good improvement to decouple the framing from the device driver

See #9335 (comment). I think something is wrong with the concept here.... The layered approach should make it possible that it doesn't matter which pointer you use. And if it's not the case the pointer change should certainly not be implemented here IMHO, but in the netdev_ieee802154 adaption PR.

miri64 · 2018-06-12T09:47:42Z

sys/net/gnrc/netif/gnrc_netif_raw.c

                                    netdev_t *dev)
 {
    return gnrc_netif_create(stack, stacksize, priority, name, dev,
+                             gnrc_netif_thread,


why not here a special _bootstrap method?

Probably because I forgot to correctly adapt this one

miri64 · 2018-06-12T10:25:38Z

I think lwmac and gomac also need adaptation.

bergzand · 2018-06-12T11:51:11Z

I think lwmac and gomac also need adaptation.

Done!

bergzand · 2018-06-12T11:53:53Z

Done!

Now even on the correct branch

Adds a bootstrap function to provide setting up the layered structure for the netdev devices. The structs end up on the stack providing some sort of dynamic allocation for the layers

bergzand · 2018-07-09T10:18:40Z

Reworked a bit to contain only the minimal required for the this PR.

Also rebased to resolve the merge conflict.

miri64 · 2018-07-09T10:26:36Z

Hi, while I agree with your approach, now after looking at it after some time, I think the name "bootstrip" might be confusing. I was searching for a while where this "bootstrap" method is called, only to realize that it is the thread handler. Maybe rename the variable to thread_run or something to clear things up?

bergzand · 2018-07-09T10:29:12Z

While I agree that it might be confusing, I think that thread_run doesn't cover the full goal of the function here. Maybe thread_setup_and_run? :)

miri64 · 2018-07-09T10:34:32Z

The easiest would be thread_handler ;-). Why is thread_setup_and_run less confusing? The setup happens in the "run" method of the thread (forgive my Java/Python linguo), so why distinguish specifically?

bergzand · 2018-07-09T11:41:22Z

The setup happens in the "run" method of the thread (forgive my Java/Python linguo), so why distinguish specifically?

The idea behind this "setup" function is to be able to have L2 protocol specific setup before the handling of the netif loop starts. In my case specifically to have a place to instantiate netdev structs on the stack of the netif thread.

miri64 · 2018-07-09T12:20:33Z

I know, but what you are doing here at the moment is allowing to customize the thread handler. With a function name called setup I would expect it to be called before starting the thread handler or maybe to be the very first thing to be called within the thread handler, not the thread handler itself. "bootstrap" implies on the otherhand that it is what bootstraps the interface, which is completely wrong here also.

bergzand · 2018-07-09T12:53:31Z

Okay, I was afraid that the goal of the function was not completely clear anymore.

I don't have a better idea at the moment, so I'm fine with thread_run if you think that is the better(documentation-wise) name here. gnrc_netif_ieee802154_thread_run for completeness or just thread_run?

miri64 · 2018-11-22T11:42:06Z

Needs rebase.

stale · 2019-08-10T04:07:59Z

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.

bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking GNRC labels Jun 12, 2018

bergzand requested a review from miri64 June 12, 2018 09:17

jnohlgard reviewed Jun 12, 2018

View reviewed changes

miri64 reviewed Jun 12, 2018

View reviewed changes

bergzand mentioned this pull request Jun 12, 2018

netdev: Rework netdev_eth to layered structure #9330

Closed

bergzand mentioned this pull request Jun 12, 2018

netdev: Add netopt to retrieve netdev_ieee802154_t #9335

Closed

bergzand added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 12, 2018

bergzand mentioned this pull request Jun 28, 2018

netdev_ieee802154: Use intermediate layer for mac header #9417

Closed

bergzand added 14 commits July 9, 2018 12:18

gnrc_netif: Adapt gnrc_netif to netdev_layer

229a078

Adds a bootstrap function to provide setting up the layered structure for the netdev devices. The structs end up on the stack providing some sort of dynamic allocation for the layers

gnrc_netif: adapt ethernet to netdev_layer

e4b9761

gnrc_netif: adapt raw layer to netdev_layered api

97b2259

gnrc_netif: adapt ieee802154 to netdev_layered

d5ec986

fixup! gnrc_netif: adapt ieee802154 to netdev_layered

987bc07

fixup! gnrc_netif: Adapt gnrc_netif to netdev_layer

de2f2b5

fixup! gnrc_netif: adapt raw layer to netdev_layered api

ad9a546

lwmac: adapt to netdev_layer

1776eac

gomach: adapt to netdev_layer

0ffc1e1

fixup! gomach: adapt to netdev_layer

8a67648

fixup! lwmac: adapt to netdev_layer

d31b87f

fixup! gnrc_netif: adapt raw layer to netdev_layered api

793051a

fixup! fixup! gnrc_netif: Adapt gnrc_netif to netdev_layer

cbde191

fixup! fixup! gnrc_netif: adapt ieee802154 to netdev_layered

9b9ac70

bergzand force-pushed the pr/netdev/layer/gnrc_netif branch from 378de5f to 9b9ac70 Compare July 9, 2018 10:18

miri64 added State: waiting for other PR and removed GNRC labels Oct 5, 2018

miri64 removed the State: waiting for other PR label Oct 12, 2018

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot closed this Sep 10, 2019

Conversation

bergzand commented Jun 12, 2018

Contribution description

Issues/PRs references

Uh oh!

jnohlgard commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jun 12, 2018

Uh oh!

bergzand commented Jun 12, 2018

Uh oh!

bergzand commented Jun 12, 2018

Uh oh!

bergzand commented Jul 9, 2018

Uh oh!

miri64 commented Jul 9, 2018

Uh oh!

bergzand commented Jul 9, 2018

Uh oh!

miri64 commented Jul 9, 2018

Uh oh!

bergzand commented Jul 9, 2018

Uh oh!

miri64 commented Jul 9, 2018

Uh oh!

bergzand commented Jul 9, 2018

Uh oh!

miri64 commented Nov 22, 2018

Uh oh!

stale bot commented Aug 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development