cc2420/gnrc_netif: Adds crc_valid to netif #8276

roberthartung · 2017-12-19T08:04:24Z

Provides a way to check CRC inside the application, by adding a crc_valid field to the netif hdr.

drivers/cc2420/cc2420.c

sys/include/net/gnrc/netif/hdr.h

miri64 · 2018-03-15T12:28:23Z

Ping @roberthartung?

roberthartung · 2018-04-03T14:35:47Z

Sorry -> Vacation. Still on it, but other tasks have higher priority ;)

miri64 · 2018-08-24T14:01:57Z

The crc_valid member can go into flags, IMHO.

miri64 · 2018-08-24T14:14:00Z

I took the liberty to that as a fixup ;-) (needs rebase btw)

@bergzand

@bergzand @smlng @kYc0o since I now contributed, could you please review? Also I don't have a cc2420 to test.

miri64 · 2018-10-03T17:20:26Z

@bergzand ping?

miri64 · 2018-11-22T12:21:34Z

@bergzand @smlng @kYc0o since I now contributed, could you please review? Also I don't have a cc2420 to test.

Ping? I also rebased this PR.

miri64 · 2018-12-18T15:16:10Z

~~Rebased to current master.~~ Can't push to @roberthartung's branch anymore :(

miri64 · 2019-02-26T18:44:13Z

Rebased to current master. Can't push to @roberthartung's branch anymore :(

Nope, was just stupid.

roberthartung · 2019-03-05T13:06:40Z

@miri64 yeehaa :-) So what's our plan?

miri64 · 2019-03-12T08:12:48Z

That someone reviews this. @bergzand?

miri64 · 2019-03-12T08:21:40Z

In the meantime I simplified the code a bit and amended the doc.

roberthartung · 2019-03-12T08:29:32Z

rebased, let's see what the checks say!

bergzand · 2019-03-12T08:33:07Z

Out of curiosity, what's the use case for this? I can imagine the sniffer application as one, but for "regular" radio operation I'm assuming that most simply drop the frame if the CRC is invalid (so frames with invalid CRC are never going to reach the radio driver).

bergzand · 2019-03-12T08:33:51Z

Besides trying to compile this, are there any test instructions?

miri64 · 2019-03-12T08:34:22Z

rebased, let's see what the checks say!

arghs arghs arghs, did you make sure my latest changes were in it?

miri64 · 2019-03-12T08:35:15Z

rebased, let's see what the checks say!

arghs arghs arghs, did you make sure my latest changes were in it?

yes, they are :-). Thank you!

miri64 · 2019-03-12T08:37:08Z

However, can you please have it according to the contribution guidelines and prefix the commit with the module you change? Also since you are touching at least 3 modules/APIs (netdev, gnrc_netif_hdr, and cc2420), there should be a split-up of commits. At least the API changes should be distinct from the implementation example in the cc2420.

bergzand · 2019-03-12T08:44:40Z

drivers/include/net/netdev.h

@@ -249,6 +249,7 @@ typedef enum {
 struct netdev_radio_rx_info {
    int16_t rssi;       /**< RSSI of a received packet in dBm */
    uint8_t lqi;        /**< LQI of a received packet */
+    uint8_t crc_valid;  /**< CRC was valid (when != 0) */


Now this might be in the "premature optimization" area. Is there a use case for changing this to uint8_t flags; and use it as a number of boolean flags for received frames, something like

NETDEV_RADIO_RX_CRC_VALID (0x01) /**< Set if the CRC was valid */ NETDEV_RADIO_RX_DATA_PEND (0x02) /**< Set if the received frame has the data pending bit set */

I don't want to stall this PR more than necessary with this idea. if we might need this in the future, having a bool netdev_radio_crc_valid(netdev_radio_rx_info_t *info) getter for the crc status would prevent a migration to flags being a significant API change.

Sounds valid to me, @miri64 what's your opinion?

Agreed, however also see #11023 for pending data handling ;) (since that can be read from the MAC header at least in IEEE 802.15.4's case, I'm not sure it needs to be in the info struct ;-)

miri64 · 2019-03-12T08:50:18Z

Out of curiosity, what's the use case for this? I can imagine the sniffer application as one, but for "regular" radio operation I'm assuming that most simply drop the frame if the CRC is invalid (so frames with invalid CRC are never going to reach the radio driver).

Not sure this is the intention, but apart from the experimental use-case you pointed out, I believe a MAC protocol might also benefit from this, e. g. if it wants to be notified that something was received, even is it was invalid.

roberthartung · 2019-03-12T09:09:43Z

Out of curiosity, what's the use case for this? I can imagine the sniffer application as one, but for "regular" radio operation I'm assuming that most simply drop the frame if the CRC is invalid (so frames with invalid CRC are never going to reach the radio driver).

Forward Error Correction is a regular use case.

roberthartung · 2019-03-12T09:11:01Z

Out of curiosity, what's the use case for this? I can imagine the sniffer application as one, but for "regular" radio operation I'm assuming that most simply drop the frame if the CRC is invalid (so frames with invalid CRC are never going to reach the radio driver).

Not sure this is the intention, but apart from the experimental use-case you pointed out, I believe a MAC protocol might also benefit from this, e. g. if it wants to be notified that something was received, even is it was invalid.

ACK. Very common use case to use dropped packets as a metric.

jia200x · 2019-07-25T09:51:43Z

sys/net/gnrc/netif/ieee802154/gnrc_netif_ieee802154.c

@@ -90,7 +90,7 @@ static inline bool _already_received(gnrc_netif_t *netif,
 static gnrc_pktsnip_t *_recv(gnrc_netif_t *netif)
 {
    netdev_t *dev = netif->dev;
-    netdev_ieee802154_rx_info_t rx_info;
+    netdev_ieee802154_rx_info_t rx_info = { .crc_valid = 0 };


Why is crc_valid set to 0? AFAIS this will be always set by the driver in this case.

It is not guaranteed that the driver will set it. In that case not setting it to 0 would mean the value would be in an undefined state (as it is allocated on a dirty stack).

I think the driver should always set it. If the driver doesn't set it, it will be seen as invalid by the MAC layer.

If the radio doesn't support CRC calculation, this should be ignored via radio_caps (see #11473 ). If the driver doesn't provide a flag for the CRC (but does the calculations), it can be set to valid anyway.

In this PR we can't assume that and we also should not. Otherwise this PR needs to touch all the drivers.

It is also in general not save to assume the callee changes a value, so it should always be set if it is checked later. Just from a security perspective ;-).

Most of the devices calculate the CRC themselves and don't (necessarily) expose it. Furthermore, oftentimes only frames with valid CRCs are even indicated by our drivers. In that case, .crc needs to be set to a value !=0. Where does this happen? Or did I misunderstand something?

In that case, .crc needs to be set to a value !=0. Where does this happen? Or did I misunderstand something?

It the drivers. However, this PR only ports the cc2420. That's why I and @jia200x suggested that @roberthartung adds the big fat warning to the doc of that field. Requiring all the drivers to be ported in this PR would explode this PR.

The initialization as used above does this implicitly

So you are saying that netdev_ieee802154_rx_info_t rx_info = { .crc_valid = 0 }; initializes the whole structure with 0? (And I'm not talking about some smart compiler things going on in the background)

Requiring all the drivers to be ported in this PR would explode this PR.

How about we wait until someone opens the follow-up PR and then we both consecutively?

So you are saying that netdev_ieee802154_rx_info_t rx_info = { .crc_valid = 0 }; initializes the whole structure with 0? (And I'm not talking about some smart compiler things going on in the background)

To say it semantically correct: struct foobar foo = { .field = x } initializes field with x and all fields of foobar that are not provided with 0. So in the case above, yes it initializes the whole struct with 0 (AFAIK in the background most compilers solve this by just implicitly calling memset() though). See https://en.cppreference.com/w/c/language/struct_initialization for reference.

How about we wait until someone opens the follow-up PR and then we both consecutively?

See #9755 for at86rf2xx

jia200x · 2019-07-25T09:54:01Z

Not sure this is the intention, but apart from the experimental use-case you pointed out, I believe a MAC protocol might also benefit from this, e. g. if it wants to be notified that something was received, even is it was invalid.

There are even some radios that don't calculate CRC. This feature (together with #11473 ) would provide part of the IEEE802.15.4 PHY/MAC

stale · 2020-01-26T13:05:29Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

jia200x · 2020-04-08T15:38:32Z

@fjmolinas this concept might still be interesting for #13824

roberthartung mentioned this pull request Dec 19, 2017

app/netdev: application stops working after receiving frames with assertion or completely without error #8271

Open

miri64 reviewed Dec 19, 2017

View reviewed changes

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

miri64 self-assigned this Dec 19, 2017

miri64 added Area: drivers Area: Device drivers Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Dec 19, 2017

miri64 previously requested changes Feb 15, 2018

View reviewed changes

sys/include/net/gnrc/netif/hdr.h Outdated Show resolved Hide resolved

miri64 mentioned this pull request Aug 24, 2018

crc validity variable in netdev_radio_rx_info and gnrc_netif_hdr_t and read it for at86rf2xx and cc2420 #9755

Closed

miri64 force-pushed the netif-crc-valid branch 2 times, most recently from 0658ffc to 9dfbed5 Compare October 3, 2018 17:33

miri64 force-pushed the netif-crc-valid branch from 9dfbed5 to 6ba32fb Compare November 22, 2018 12:28

miri64 force-pushed the netif-crc-valid branch from 6ba32fb to 1a9410f Compare February 26, 2019 18:44

miri64 requested a review from bergzand February 26, 2019 18:44

roberthartung force-pushed the netif-crc-valid branch from bec9ac5 to 02bde1e Compare March 12, 2019 08:29

bergzand reviewed Mar 12, 2019

View reviewed changes

roberthartung added 3 commits March 12, 2019 10:15

drivers/netdev: adds crc_valid to radio_rx_info

2b373f5

gnrc/netif: implements crc valid flag

50bc6ad

drivers/cc2420: implements crc valid flag

a2f1f20

roberthartung force-pushed the netif-crc-valid branch from 02bde1e to a2f1f20 Compare March 12, 2019 09:15

miri64 changed the title ~~WIP: Adds crc_valid to netif~~ cc2420/gnrc_netif: Adds crc_valid to netif Jul 25, 2019

jia200x reviewed Jul 25, 2019

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 26, 2020

stale bot closed this Feb 26, 2020

fjmolinas mentioned this pull request Apr 16, 2020

drivers/netdev: adds crc_valid to radio_rx_info #13878

Closed

9 tasks

cc2420/gnrc_netif: Adds crc_valid to netif #8276

cc2420/gnrc_netif: Adds crc_valid to netif #8276

Conversation

roberthartung commented Dec 19, 2017

miri64 commented Mar 15, 2018

roberthartung commented Apr 3, 2018

miri64 commented Aug 24, 2018

miri64 commented Aug 24, 2018

miri64 commented Oct 3, 2018

miri64 commented Nov 22, 2018

miri64 commented Dec 18, 2018 • edited Loading

miri64 commented Feb 26, 2019

roberthartung commented Mar 5, 2019

miri64 commented Mar 12, 2019

miri64 commented Mar 12, 2019

roberthartung commented Mar 12, 2019

bergzand commented Mar 12, 2019

bergzand commented Mar 12, 2019

miri64 commented Mar 12, 2019

miri64 commented Mar 12, 2019

miri64 commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Mar 12, 2019

roberthartung commented Mar 12, 2019

roberthartung commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jia200x commented Jul 25, 2019

stale bot commented Jan 26, 2020

jia200x commented Apr 8, 2020

miri64 commented Dec 18, 2018 •

edited

Loading

PeterKietzmann Jul 25, 2019 •

edited

Loading