drivers/nrf802154: do not filter broadcast PAN ID#18283
drivers/nrf802154: do not filter broadcast PAN ID#18283benpicco merged 3 commits intoRIOT-OS:masterfrom
Conversation
github-actions
bot
added
Area: cpu
|
|
||
| state = STATE_IDLE; | ||
| enable_shorts = true; | ||
| if (info) { |
There was a problem hiding this comment.
Why is this the right location for this check and not just before if (eagain) (below switch case?)
There was a problem hiding this comment.
because that overwrites the IEEE802154_HAL_OP_CCA case. Since info has "something", it replaces the CCA status with garbage
There was a problem hiding this comment.
Sorry, I still don't fully understand.
- What do you mean by "that" overwrites the
IEEE802154_HAL_OP_CCAcase? How to overwrite a case at all? - 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?
There was a problem hiding this comment.
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.
|
|
||
| int addr_len = ieee802154_get_dst(mhr, dst_addr, &dst_pan); | ||
|
|
||
| if ((mhr[0] & IEEE802154_FCF_TYPE_MASK) == IEEE802154_FCF_TYPE_BEACON) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Could you add a comment that explains the filter in words? Then I would feel more comfortable checking the "reviewed documentation" label
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
GNRC should just discard the frame if it finds it to not be a valid 6LoWPAN frame, right @miri64?
There was a problem hiding this comment.
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).
PeterKietzmann
added
Reviewed: 2-code-design
benpicco
added
the
Type: bug
benpicco
added
the
CI: ready for build
5 participants
Contribution description
This changes were (almost) cherry-picked from #18156.
It addresses three things:
These commits are required in order to run #18156.
Testing procedure
tests/ieee802154_hal. It should report a mixture of "BUSY" and "CLEAR".Issues/PRs references
#18156