RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

gnrc_netif: remove NETDEV_MSG_TYPE_EVENT processing from thread

Open jia200x opened this issue 4 years ago • 25 comments

Contribution description

This PR is the first one for removing the netdev dependency in gnrc_netif. It moves the NETDEV_EVENT_MSG_TYPE_EVENT handling to each netif->ops->msg_handler. That way an implementation of gnrc_netif_ops_t can use other mechanisms for handling device ISRs.

With some follow ups it will be possible to use gnrc_netif without netdev, so it's possible to directly hook drivers or other interfaces (radio HALs, etc).

Testing procedure

Besides Murdock compile tests, this should be tested to ensure all gnrc_netif_ops_t implementation still work (gnrc_netif_ethernet, gnrc_netif_lorawan, etc).

Issues/PRs references

None so far

jia200x avatar Mar 10 '20 14:03 jia200x

@jia200x please address or answer my comments. Otherwise, I would be fine with merging. @miri64 do you agree?

PeterKietzmann avatar Apr 09 '20 08:04 PeterKietzmann

Yepp, AFAIR I did not test this yet, though.

miri64 avatar Apr 09 '20 08:04 miri64

Here is what I've done already. I'd wait with further tests until the code is in a "final" state

PeterKietzmann avatar Apr 09 '20 08:04 PeterKietzmann

Is this PR waiting for something (besides more testing)?

PeterKietzmann avatar May 04 '20 14:05 PeterKietzmann

Is this PR waiting for something (besides more testing)?

At least now for a rebase :-/.

miri64 avatar Jun 25 '20 14:06 miri64

some radio drivers are missing

benpicco avatar Sep 10 '20 16:09 benpicco

I can confirm that ACK timeouts still work on at86rf215 which (ab)uses NETDEV_MSG_TYPE_EVENT for ACK timeouts.

I'm happy to jettison this code when the new sub-Mac is merged :smile:

benpicco avatar Sep 28 '20 15:09 benpicco

I think I addressed all comments

jia200x avatar Sep 28 '20 16:09 jia200x

I'm happy to jettison this code when the new sub-Mac is merged :smile:

/me wonders if @benpicco knows that RIOT supports more MAC layers than the 802.15.4 one :thinking: :stuck_out_tongue_winking_eye:

miri64 avatar Sep 28 '20 18:09 miri64

Well I was hoping to get #14950 merged before any new sub-MACs pop up :wink:

benpicco avatar Sep 28 '20 18:09 benpicco

esp_wifi and atwinc15x0 are handled by gnrc_netif_ethernet.c I assume?

Feel free to squash.

benpicco avatar Oct 21 '20 22:10 benpicco

esp_wifi and atwinc15x0 are handled by gnrc_netif_ethernet.c I assume?

Sorry, after a couple of months with other tasks I am no longer up to date, but I guess you are right. atwinc15x0 is just a netdev driver and esp_wifi is using gnrc_netif_ethernet.

gschorcht avatar Oct 21 '20 23:10 gschorcht

What's the state of this?

benpicco avatar May 06 '21 14:05 benpicco

It's still alive. I will reactivate it after next Friday

jia200x avatar May 07 '21 08:05 jia200x

It's still alive. I will reactivate it after next Friday

I completely forgot about it. I just rebased it to current master.

jia200x avatar Jun 09 '21 14:06 jia200x

We can work on this as well I suppose.

MrKevinWeiss avatar Jun 24 '21 11:06 MrKevinWeiss

it's already rebased. It just needs reviewing

jia200x avatar Jun 24 '21 11:06 jia200x

As is this is pretty uncontroversial: instead of directly handling the NETDEV_MSG_TYPE_EVENT case in _gnrc_netif_thread it's now handled by the default case which then calls netif->ops->msg_handler which then handles NETDEV_MSG_TYPE_EVENT.

The only thing I don't understand is what we gain from this, other than slightly more overhead. The netdev dependency is still there, just moved to gnrc_netif_msg_handler_netdev().

benpicco avatar Jun 24 '21 12:06 benpicco

The only thing I don't understand is what we gain from this, other than slightly more overhead. The netdev dependency is still there, just moved to gnrc_netif_msg_handler_netdev().

It's just one of the steps required in order to remove the netdev dependency from gnrc_netif. Also, we can avoid compiling that part at all if we are using other mechanisms such as gnrc_netif_events or thread flags.

jia200x avatar Jun 24 '21 14:06 jia200x

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[bot] avatar Mar 02 '22 09:03 stale[bot]

It looks like we were close to merging this - do you still want this in?

benpicco avatar Mar 02 '22 10:03 benpicco

It looks like we were close to merging this - do you still want this in?

Yes! I just amended @maribu's comments.

jia200x avatar Mar 02 '22 12:03 jia200x

This doesn't look scary to me, so I would insist on full testing.

I think there a "not" missing :)

maribu avatar Mar 02 '22 22:03 maribu

CI is unhappy

benpicco avatar Mar 03 '22 08:03 benpicco

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[bot] avatar Sep 21 '22 05:09 stale[bot]

What's missing here?

maribu avatar Sep 22 '22 14:09 maribu

hmmmm this became obsolete, as we already removed this msg mechanism via https://github.com/RIOT-OS/RIOT/pull/16748. Closing.

jia200x avatar Sep 22 '22 15:09 jia200x