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

drivers/nrf802154: do not filter broadcast PAN ID #18283

Merged
merged 3 commits into from
Aug 16, 2022
Merged

drivers/nrf802154: do not filter broadcast PAN ID #18283

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a27241
drivers/nrf802154: fix variable initialization
jia200x Jun 29, 2022
55dc628
drivers/nrf802154: fix CCA result
jia200x Jun 29, 2022
45ba913
drivers/nrf802154: do not filter broadcast PAN ID
jia200x Jun 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions cpu/nrf52/radio/nrf802154/nrf802154_radio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ static bool _l2filter(uint8_t *mhr)

int addr_len = ieee802154_get_dst(mhr, dst_addr, &dst_pan);

if ((mhr[0] & IEEE802154_FCF_TYPE_MASK) == IEEE802154_FCF_TYPE_BEACON) {
Copy link
Member

Choose a reason for hiding this comment

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

In earlier days (before 802.15.4 radio HAL) GNRC got stuck receiving beacon frames from devices that did not filter those in hardware. Clearly, you need to receive beacons to enable a proper MAC. I just want to make sure we don't break the "standard" CSMA sender with this.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment that explains the filter in words? Then I would feel more comfortable checking the "reviewed documentation" label

Copy link
Member Author

Choose a reason for hiding this comment

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

In case this is a problem, I think these stuff should be fixed in an upper layer (e.g SubMAC). Otherwise we will end up with too many corner cases at driver level, because some modules need it and some not

Copy link
Member

Choose a reason for hiding this comment

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

Have you tested on this PR what happens in a standard CSMA case when a beacon is received?

I agree that the driver is not the best place for it (actually for any of the common 802.15.4 filter routines) but in the first place we should not introduce a feature that breaks the (currently) most common use case.

Copy link
Contributor

Choose a reason for hiding this comment

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

GNRC should just discard the frame if it finds it to not be a valid 6LoWPAN frame, right @miri64?

Copy link
Member

Choose a reason for hiding this comment

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

GNRC should just discard the frame if it finds it to not be a valid 6LoWPAN frame, right @miri64?

No, gnrc_sixlowpan will do that, yes, but GNRC will just eat up anything you give it ;-). If there is a Zigbee handler it will also handle Zigbee packets (which are not valid 6LoWPAN frames IIRC).

if ((memcmp(&nrf802154_pan_id, pan_bcast, 2) == 0)) {
return true;
}
}
/* filter PAN ID */
/* Will only work on little endian platform (all?) */

Expand Down Expand Up @@ -197,7 +202,7 @@ static int _request_op(ieee802154_dev_t *dev, ieee802154_hal_op_t op, void *ctx)
(void) dev;

int res = -EBUSY;
int state;
int state = STATE_IDLE;

switch (op) {
case IEEE802154_HAL_OP_TRANSMIT:
Expand Down Expand Up @@ -265,6 +270,10 @@ static int _confirm_op(ieee802154_dev_t *dev, ieee802154_hal_op_t op, void *ctx)

state = STATE_IDLE;
enable_shorts = true;
if (info) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this the right location for this check and not just before if (eagain) (below switch case?)

Copy link
Member Author

Choose a reason for hiding this comment

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

because that overwrites the IEEE802154_HAL_OP_CCA case. Since info has "something", it replaces the CCA status with garbage

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I still don't fully understand.

  1. What do you mean by "that" overwrites the IEEE802154_HAL_OP_CCA case? How to overwrite a case at all?
  2. The starting point for my comment was: we are in the "confirm operation" function here and I was wondering, shouldn't be the "confirm CCA operation" actually be the most accurate place to indicate whether the CCA found a busy or free channel?

Copy link
Contributor

Choose a reason for hiding this comment

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

The IEEE802154_HAL_OP_CCA already handles ctx, in fact it always assumes it to be a bool * that must be present.

If info is then set after the switch case, it would overwrite that.

info->status = (_state == STATE_CCA_BUSY) ? TX_STATUS_MEDIUM_BUSY : TX_STATUS_SUCCESS;
}

break;
case IEEE802154_HAL_OP_SET_RX:
eagain = (radio_state == RADIO_STATE_STATE_RxRu);
Expand All @@ -291,10 +300,6 @@ static int _confirm_op(ieee802154_dev_t *dev, ieee802154_hal_op_t op, void *ctx)

_state = state;

if (info) {
info->status = (_state == STATE_CCA_BUSY) ? TX_STATUS_MEDIUM_BUSY : TX_STATUS_SUCCESS;
}

if (enable_shorts) {
NRF_RADIO->SHORTS = DEFAULT_SHORTS;
DEBUG("[nrf802154] TX Finished\n");
Expand Down