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

netdev: add netdev_trigger_event_isr() function #13562

Merged
merged 2 commits into from
Mar 6, 2020
Merged

netdev: add netdev_trigger_event_isr() function #13562

merged 2 commits into from
Mar 6, 2020

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Mar 5, 2020

Contribution description

This PR adds a netdev_irq_end function that wraps the dev->event_callback(dev, NETDEV_EVENT_ISR)call.

This is a step required in order to separate ISR signals from driver events (RX_DONE, TX_DONE), etc. This is useful when network stacks have different implementations of driver events but they use the same "recv thread".

I will introduce a netdev_event_thread module that redefines this function to use the event_thread module for handling device ISRs.

Testing procedure

This should be low hanging fruit. Check if netdev drivers still compile (at86rf2xx, ethos).
Running gnrc_networking on native should be enough to test functionality.

Issues/PRs references

None so far

@jia200x jia200x added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Mar 5, 2020
@jia200x jia200x changed the title netdev: add netdev_event_irq function netdev: add netdev_irq_end function Mar 5, 2020
@jia200x jia200x mentioned this pull request Mar 5, 2020
Merged
@bergzand bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 6, 2020
bergzand
bergzand reviewed Mar 6, 2020
View reviewed changes
drivers/include/net/netdev.h Outdated
/**
* @brief Informs netdev there was an interrupt request from the network device.
*
* This function calls @ref event_callback with NETDEV_EVENT_ISR event.
Copy link
Member

Choose a reason for hiding this comment

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

The @ref event_callback is undefined here (see travis)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This function calls @ref event_callback with NETDEV_EVENT_ISR event.
* This function calls netdev_t::event_callback with NETDEV_EVENT_ISR event.

@bergzand
Copy link
Member

bergzand commented Mar 6, 2020

There are a number of remaining calls in:

  • cpu/esp32/esp-eth/esp_eth_netdev.c
  • cpu/esp32/esp-wifi/esp_wifi_netdev.c
  • cpu/esp8266/esp-wifi/esp_wifi_netdev.c
  • cpu/nrf52/radio/nrf802154/nrf802154.c
  • cpu/nrf5x_common/radio/nrfmin/nrfmin.c

Is there a reason why these are omitted from the cleanup here? Or are they more complex and should they be handled in a follow-up?

@bergzand
Copy link
Member

bergzand commented Mar 6, 2020

And please rebase :)

@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

There are a number of remaining calls in:

Oh, good catch! I don't know how I missed those. Thanks, I will add them

@jia200x jia200x force-pushed the pr/netdev_irq_end branch from 5e39d1a to 8b40aeb Compare March 6, 2020 09:13
@jia200x jia200x requested a review from SemjonWilke as a code owner March 6, 2020 09:13
@jia200x jia200x force-pushed the pr/netdev_irq_end branch from bb72276 to 701c7db Compare March 6, 2020 09:22
@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

done!

@bergzand
Copy link
Member

bergzand commented Mar 6, 2020

Looks good, please squash!

@jia200x jia200x force-pushed the pr/netdev_irq_end branch from 701c7db to 3254435 Compare March 6, 2020 10:04
@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

squashed :)

@kaspar030
Copy link
Contributor

kaspar030 commented Mar 6, 2020 •
edited by jia200x
Loading

Why end?

@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

Why end?

Hmmmm I named it like that because it should be called at the end of IRQ. But strictly speaking it could be called from anywhere during ISR.
Perhaps another name? netdev_event_isr?

@kaspar030
Copy link
Contributor

kaspar030 commented Mar 6, 2020 •
edited
Loading

Perhaps another name? netdev_event_isr?

We're bikeshedding again, sorry. netdev_event_isr sounds good. Maybe netdev_trigger_event_isr?

@miri64
Copy link
Member

miri64 commented Mar 6, 2020

We're bikeshedding again, sorry. netdev_event_isr sounds good. Maybe netdev_trigger_event_isr?

I'd say, @jia200x renames it to one of either and squashes immediately. And then let's go with that.

@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

netdev_trigger_event_isr

I will rename it to that and squash immediately.

@jia200x jia200x force-pushed the pr/netdev_irq_end branch from 3254435 to 3e42fb7 Compare March 6, 2020 12:27
@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

done!

@kaspar030 kaspar030 changed the title netdev: add netdev_irq_end function netdev: add netdev_trigger_event_isr() function Mar 6, 2020
@kaspar030
Copy link
Contributor

done!

maybe fix the commit messages

@jia200x jia200x mentioned this pull request Mar 6, 2020
Closed
@jia200x jia200x force-pushed the pr/netdev_irq_end branch from 3e42fb7 to 3ad574a Compare March 6, 2020 13:04
@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

forgot that. Now it should be OK

@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

all green :)

kaspar030
kaspar030 approved these changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030 kaspar030 merged commit 5798580 into RIOT-OS:master Mar 6, 2020
@jia200x
Copy link
Member Author

jia200x commented Mar 6, 2020

thanks for the review guys!

@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020
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

@kaspar030 kaspar030 kaspar030 approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@benpicco benpicco Awaiting requested review from benpicco

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@daniel-k daniel-k Awaiting requested review from daniel-k

@dylad dylad Awaiting requested review from dylad dylad is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@maribu maribu Awaiting requested review from maribu maribu is a code owner

@smlng smlng Awaiting requested review from smlng

@SemjonWilke SemjonWilke Awaiting requested review from SemjonWilke

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees
No one assigned
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 Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
5 participants
@jia200x @bergzand @kaspar030 @miri64 @leandrolanzieri