netdev_ieee802154/radios: refactor PAN ID reset to generic ieee802154 reset by bergzand · Pull Request #10384 · RIOT-OS/RIOT

bergzand · 2018-11-13T20:23:02Z

Contribution description

This PR refactors the PAN ID from the radio reset function to the generic netdev_ieee802154_reset and sets it via the netdev::set call. This allows for removal of the dev->netdev accesses in the radio specific set_pan calls.

Testing procedure

Verify that initialization and reset of the PAN ID works for the affected radios

Issues/PRs references

Chipping away at dependencies for #7736

miri64

ACK on the code change. It removes a lot of duplicates of setting netdev_ieee802154_t::pan for a device and centralizes it and the initialization of the value to the cental netdev_ieee802154_reset() function, which is called by either the init or reset function of all touched devices.

One minor optimization proposal I'm not sure about: I think this can be removed:

RIOT/cpu/cc2538/radio/cc2538_rf_netdev.c

Lines 364 to 365 in 83ca890

    
           netdev_ieee802154_set(&dev->netdev, NETOPT_NID, 
        
                                 &pan, sizeof(pan));

I'll test with the devices available to me as soon as I have access to them again.

bergzand · 2018-11-13T21:05:49Z

One minor optimization proposal I'm not sure about: I think this can be removed:

Thanks, I already suspected that I was missing something in that driver.

miri64

Another question after scrolling through the code a bit again

drivers/netdev_ieee802154/netdev_ieee802154.c

cpu/cc2538/radio/cc2538_rf_netdev.c

miri64 · 2018-11-13T21:26:37Z

Tested socket_zep by running test/socket_zep and by switching out gnrc_netdev_default with socket_zep in examples/gnrc_networking and pinging between two native instances.

miri64 · 2018-11-13T21:31:46Z

Also tested at86rf2xx with gnrc_networking and pinging (and checking ifconfig) on iotlab-m3

miri64 · 2018-11-13T21:35:46Z

Murdock likes it (at least compilation-wise) so I guess we can put a (virtual) tick on that as well :-)

miri64 · 2018-11-14T10:39:02Z

cc2420 doesn't work (when using example/default it doesn't send packets that are recognized by an at86rf2xx device, multicast does not work and when a packet is sent from an at86rf2xx device it doesn't show the packet). But that's the case in master (not sure if there is already an issue for that). It sends however packets with the correct PAN ID and I confirmed with ifconfig in examples/default that the PAN ID is the one configured at compile time.

miri64 · 2018-11-14T10:44:14Z

kw2xrf confirmed to work on pba-d-01-kw2x.

miri64 · 2018-11-14T11:10:07Z

@PeterKietzmann can you have a look with the cc2538 and the mrf24j40 (and maybe confirm the bug in cc2420 in master)?

miri64 · 2018-11-22T11:48:02Z

@smlng do you have access to the missing devices?

miri64 · 2018-11-22T11:48:25Z

(cc2538 I'll test myself in the testbed ;-))

smlng · 2018-11-22T11:53:17Z

remote-revb (cc2538) works as expected

smlng

PreACK, looks good to me

@smlng

@smlng please don't approve a PR, if you did not test everything. A verbal ACK and the respective Reviewed: labels should suffice.

miri64 · 2018-11-22T12:10:41Z

Tested on firefly (cc2538) with example/default (both by changing the PAN ID using ifconfig and by at compile time using CFLAGS=-DIEEE802154_DEFAULT_PANID=0x6456). ifconfig still shows the expected value and I was still able to exchange packages packets between the nodes if (and only if - as expecded) the the PAN ID was the same on both nodes. A sniffer running in parallel also showed the correct PAN ID for the exchanged packets.

Change request was addressed.

PeterKietzmann · 2018-11-27T21:06:48Z

Tested with mrf24j40 + nucleo-l476rg and gnrc_networking:

PANID after reset
changing PAN ID
verified by pinging a samr21-xpro with gnrc_networking and master

-> works as expected to I'll check the box above.

miri64

All drivers were tested. @bergzand please squash!

This commit adds the reset of the PAN ID to the generic ieee802154 reset function.

This write access is only required when a modification to the PAN ID happened directly via this function and not via a netdev::set operation. The only direct call was done in the reset function of the driver

bergzand · 2018-11-28T10:32:34Z

Squashed, thanks for testing!

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 Nov 13, 2018

bergzand requested review from PeterKietzmann, miri64 and smlng November 13, 2018 20:23

bergzand mentioned this pull request Nov 13, 2018

RFC: A layered approach to netdev #7736

Open

miri64 reviewed Nov 13, 2018

View reviewed changes

miri64 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines labels Nov 13, 2018

miri64 reviewed Nov 13, 2018

View reviewed changes

drivers/netdev_ieee802154/netdev_ieee802154.c Outdated Show resolved Hide resolved

miri64 removed the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Nov 13, 2018

miri64 previously requested changes Nov 13, 2018

View reviewed changes

cpu/cc2538/radio/cc2538_rf_netdev.c Show resolved Hide resolved

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines labels Nov 13, 2018

miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 13, 2018

smlng previously approved these changes Nov 22, 2018

View reviewed changes

miri64 approved these changes Nov 27, 2018

View reviewed changes

bergzand added 11 commits November 28, 2018 11:30

netdev_ieee802154: Add PAN ID to reset function

1678580

This commit adds the reset of the PAN ID to the generic ieee802154 reset function.

cc2538_rf: Remove PAN ID initialization from reset

da0866a

zep: Remove PAN ID initialization from reset

7ed3f80

at86rf2xx: Remove PAN ID initialization from reset

6306308

cc2420: Remove PAN ID initialization from reset

d2ddc4e

kw2xrf: Remove PAN ID initialization from reset

52e6873

mrf24j40: Remove PAN ID initialization from reset

03653ec

at86rf2xx: Remove netdev access from set_pan

357eed5

This write access is only required when a modification to the PAN ID happened directly via this function and not via a netdev::set operation. The only direct call was done in the reset function of the driver

cc2420: Remove netdev access from set_pan

bd4592a

This write access is only required when a modification to the PAN ID happened directly via this function and not via a netdev::set operation. The only direct call was done in the reset function of the driver

kw2xrf: Remove netdev access from set_pan

5566f48

This write access is only required when a modification to the PAN ID happened directly via this function and not via a netdev::set operation. The only direct call was done in the reset function of the driver

mrf24j40: Remove netdev access from set_pan

2abf86a

This write access is only required when a modification to the PAN ID happened directly via this function and not via a netdev::set operation. The only direct call was done in the reset function of the driver

bergzand force-pushed the pr/netdev/ref_nid branch from 4d88490 to 2abf86a Compare November 28, 2018 10:32

This was referenced Nov 28, 2018

drivers: Add support for KW41Z builtin transceiver #7107

Closed

native: add raw802154 driver #9942

Draft

smlng merged commit 4eaf1d8 into RIOT-OS:master Nov 28, 2018

bergzand deleted the pr/netdev/ref_nid branch November 28, 2018 11:17

aabadie added this to the Release 2019.01 milestone Dec 2, 2018

	netdev_ieee802154_set(&dev->netdev, NETOPT_NID,
	&pan, sizeof(pan));

Conversation

bergzand commented Nov 13, 2018 • edited by PeterKietzmann Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Nov 13, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miri64 commented Nov 13, 2018

Uh oh!

miri64 commented Nov 13, 2018

Uh oh!

miri64 commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Nov 14, 2018

Uh oh!

miri64 commented Nov 14, 2018

Uh oh!

miri64 commented Nov 14, 2018

Uh oh!

miri64 commented Nov 22, 2018

Uh oh!

miri64 commented Nov 22, 2018

Uh oh!

smlng commented Nov 22, 2018

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 22, 2018

Uh oh!

PeterKietzmann commented Nov 27, 2018

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Nov 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergzand commented Nov 13, 2018 •

edited by PeterKietzmann

Loading

miri64 commented Nov 13, 2018 •

edited

Loading