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.

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

cpu/nrf52/radio/nrf802154: fix beacon acceptance #20982

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Lukas-Luger
Copy link

Contribution description

The nrf802154_radio.c file implements a frame filter on the MAC sublayer as mentioned in IEEE 802.15.4-2020 in Section 6.7.2 Reception and rejection. For beacon frames (section e), it should check if source PAN ID matches the PAN ID of the device. This is currently not the case and beacon frames get filtered out.

This PR implements the PAN ID check, while also confirming that source PAN ID is present in the first place.

Testing procedure

Tests provided in the first commit. (requires two nrf52 devices)

Issues/PRs references

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: cpu Area: CPU/MCU ports labels Nov 13, 2024
@crasbe
Copy link
Contributor

crasbe commented Nov 13, 2024

It looks like this is your first Pull Request? It looks very good for a first PR :)
You probably noticed the failed "static tests", this is due to some coding conventions (like no trailing whitespaces at the end of the line). You can find some more information about that here: https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#coding-conventions

You can perform these static tests on your local machine before publishing a Pull Request (however this is no guarantuee that the tests here won't fail. Sometimes something slips though and that's okay :) ).


I'll try to give this a try next week. However I don't know much about 802.15.4.

For the test program, it might be good to add one or two commands to send frames without PAN ID or with a mismatched PAN ID to see if the code you added will correctly handles these cases (even though they shouldn't occur in the first place).

@crasbe
Copy link
Contributor

crasbe commented Nov 20, 2024

I tested the PR today and everything works as expected.

My only two remaining remarks are:

  1. The beacon function in the test program is called send, which doesn't make it super obvious that it'll send a beacon. bcnsnd would follow the "scheme" set by txtsnd or beaconsend, which is a bit nicer to remember.
    Maybe as a description you could write Send an IEEE 802.15.4 beacon instead of sending Beacon.

  2. While you're at it, you could add a sentence to the About section in the README file to specify for which devices this test works. I first grabbed an nRF52DK with the nRF52832, which does not support IEEE 802.15.4.
    Something like "Supported are for example nRF52840 based boards."

@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 25, 2024
@riot-ci
Copy link

riot-ci commented Nov 25, 2024

Murdock results

✔️ PASSED

ea0c0ee fixup! fixup! tests/drivers/nrf802154: test if Beacons can be received

Success Failures Total Runtime
10249 0 10249 16m:10s

Artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants