Skip to content

drivers/radios: remove default event reporting flags#9577

Merged
PeterKietzmann merged 4 commits intoRIOT-OS:masterfrom
bergzand:pr/netdev/rem_default_tell_opts
Jul 17, 2018
Merged

drivers/radios: remove default event reporting flags#9577
PeterKietzmann merged 4 commits intoRIOT-OS:masterfrom
bergzand:pr/netdev/rem_default_tell_opts

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Jul 16, 2018 •
edited by jia200x
Loading

Contribution description

Now that all network stacks set the required netdev event flags on init, these defaults can be removed from the radios. This prevents the radios from reporting events that are not handled by the network stack anyway.

Issues/PRs references

#9467, #9553, #9555, #9556

Testing

All current network stacks should be tested with at least one of the changed drivers to ensure that all required events for that network stack still work (testing on native (with socket_zep) is not enough).

  • GNRC
    • cc2420: tested with examples/default and txtsnd between 2 z1 nodes
    • at86rf2xx <-> kw2x: tested with examples/gnrc_networking and ping6 between samr21-xpro and pba-d-01-kw2x
    • at86rf2xx <-> mrf24j40: tested with examples/gnrc_networking and ping6 between samr21-xpro and nucleo-l476rg+mrf24j40
  • LwIP, tested with tests/lwip, ip and udp commands and 4 samr21-xpro nodes
  • emb6, tested with tests/emb6, ping6 and udp commands and 4 iotlab-m3 nodes
  • openthread

@miri64 Did I miss a network stack that uses one of the modified 802.15.4 radios in this list?

@bergzand bergzand added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Jul 16, 2018
@bergzand bergzand changed the title drivers/radios: removed default event reporting flags drivers/radios: remove default event reporting flags Jul 16, 2018
@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Jul 17, 2018 •
edited
Loading

@jia200x can you please test if openthread still works as expected with this change?
Edit: on a board, obviously...

@bergzand I've added some PR references to your description.

@PeterKietzmann
Copy link
Copy Markdown
Member

PeterKietzmann commented Jul 17, 2018

I hit the cc2420 checkbox even though that device has some issues that do not relate to this PR (will report separately)

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jul 17, 2018

Thanks for expanding @PeterKietzmann. I believe that it should not be necessary to check every radio changed here, but it doesn't hurt either.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jul 17, 2018

@bergzand this might break OpenThread, but it wouldn't be this PR's fault. If that happens, I will open a PR with the fix

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jul 17, 2018

@bergzand the events are initialized in the netdev_init function. So, I confirm this PR doesn't affect OpenThread ;)

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jul 17, 2018

(just marked OpenThread)

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jul 17, 2018

@bergzand the events are initialized in the netdev_init function. So, I confirm this PR doesn't affect OpenThread ;)

But did you test it? I'd like to detect any issues regarding the event flags here and fix them before merging this PR.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jul 17, 2018

But did you test it? I'd like to detect any issues regarding the event flags here and fix them before merging this PR.

Yes, via IoT-LAB. It works as expected

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jul 17, 2018

Yes, via IoT-LAB. It works as expected

Ok, thanks for testing!

@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 17, 2018
@PeterKietzmann PeterKietzmann merged commit 2bc48c3 into RIOT-OS:master Jul 17, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jul 17, 2018

@cladmi should this be backported?

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jul 18, 2018

From an offline clarification I would say yes it makes sense.

@bergzand bergzand mentioned this pull request Jul 18, 2018
Closed
8 tasks
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Jul 18, 2018

Backport provided in #9601

This was referenced Jul 19, 2018
Merged
Merged
@cladmi cladmi added this to the Release 2018.10 milestone Aug 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@kYc0o kYc0o Awaiting requested review from kYc0o

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers 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 2018.10

Development

Successfully merging this pull request may close these issues.

5 participants

@bergzand @PeterKietzmann @jia200x @miri64 @cladmi