drivers/netdev: adds crc_valid to radio_rx_info by fjmolinas · Pull Request #13878 · RIOT-OS/RIOT

fjmolinas · 2020-04-16T09:40:10Z

Contribution description

This PR takes over for the staled and closed #8276. It also adds crc_valid to all other 802.15.4 drivers (except for xbee and native/socket_zep unless I missed one).

I think commits should be reviewed individually for each radio. I currently don't have access to all the radios, I should in 1 month (if everything goes well) have access to boards with the missing radios.

I could also split the untested radios out of this PR.

Testing procedure

Setup

I applied the following patch to the sniffer to print crc_info, I also made that NETDEV_CRC_ERROR event still pass the packet. I'll PR the crc_indo if this gets merged.

git diff sys/

diff --git a/sys/net/gnrc/netif/gnrc_netif.c b/sys/net/gnrc/netif/gnrc_netif.c
index 428c1e2711..7f235378d1 100644
--- a/sys/net/gnrc/netif/gnrc_netif.c
+++ b/sys/net/gnrc/netif/gnrc_netif.c
@@ -1504,6 +1504,12 @@ static void _event_cb(netdev_t *dev, netdev_event_t event)
                     _pass_on_packet(pkt);
                 }
                 break;
+            case NETDEV_EVENT_CRC_ERROR:
+                pkt = netif->ops->recv(netif);
+                if (pkt) {
+                    _pass_on_packet(pkt);
+                }
+                break;
 #ifdef MODULE_NETSTATS_L2
             case NETDEV_EVENT_TX_MEDIUM_BUSY:
                 /* we are the only ones supposed to touch this variable,

git diff sniffer/main.c

diff --git a/sniffer/main.c b/sniffer/main.c
index 0846c98..deedc71 100644
--- a/sniffer/main.c
+++ b/sniffer/main.c
@@ -56,10 +56,12 @@ void dump_pkt(gnrc_pktsnip_t *pkt)
 {
     gnrc_pktsnip_t *snip = pkt;
     uint8_t lqi = 0;
+    uint8_t crc = 0;
     if (pkt->next) {
         if (pkt->next->type == GNRC_NETTYPE_NETIF) {
             gnrc_netif_hdr_t *netif_hdr = pkt->next->data;
             lqi = netif_hdr->lqi;
+            crc = netif_hdr->flags & GNRC_NETIF_HDR_FLAGS_CRC_VALID;
             pkt = gnrc_pktbuf_remove_snip(pkt, pkt->next);
         }
     }
@@ -71,6 +73,8 @@ void dump_pkt(gnrc_pktsnip_t *pkt)
     print_byte_hex(lqi);
     print_str(" rx_time ");
     print_u64_hex(now_us);
+    print_str(" crc ");
+    print_byte_hex(crc);
     print_str("\n");
     while (snip) {
         for (size_t i = 0; i < snip->size; i++) {
@@ -129,6 +133,11 @@ int main(void)
     dump.demux_ctx = GNRC_NETREG_DEMUX_CTX_ALL;
     gnrc_netreg_register(GNRC_NETTYPE_UNDEF, &dump);
 
+    netopt_enable_t enable;
+    /* Sniffer mode only */
+    enable = NETOPT_ENABLE;
+    gnrc_netapi_set(dump.target.pid, NETOPT_PROMISCUOUSMODE, 0, &enable, sizeof(enable));
+
     /* start the shell */
     puts("All ok, starting the shell now");
     char line_buf[SHELL_DEFAULT_BUFSIZE];

I also setup a at86rf233 with AUTOCRC disabled, with this radio its easy since it will simply not fill the FCS, so it will be invalid. Any other radio transmitting normally should have valid CRC.

git diff drivers/at86rf2xx/at86rf2xx

diff --git a/drivers/at86rf2xx/at86rf2xx.c b/drivers/at86rf2xx/at86rf2xx.c
index 8ec4a9f12e..af94ae182d 100644
--- a/drivers/at86rf2xx/at86rf2xx.c
+++ b/drivers/at86rf2xx/at86rf2xx.c
@@ -113,6 +113,11 @@ void at86rf2xx_reset(at86rf2xx_t *dev)
     at86rf2xx_set_txpower(dev, AT86RF2XX_DEFAULT_TXPOWER);
     /* set default options */
 
+    /* disable AUTO_CRC */
+    uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_CTRL_1);
+    tmp &= ~(AT86RF2XX_TRX_CTRL_1_MASK__TX_AUTO_CRC_ON);
+    at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_CTRL_1, tmp);
+
     if (!IS_ACTIVE(AT86RF2XX_BASIC_MODE)) {
         at86rf2xx_set_option(dev, AT86RF2XX_OPT_AUTOACK, true);
         at86rf2xx_set_option(dev, AT86RF2XX_OPT_CSMA, true);
@@ -131,7 +136,7 @@ void at86rf2xx_reset(at86rf2xx_t *dev)
 
 #if !defined(MODULE_AT86RFA1) && !defined(MODULE_AT86RFR2)
     /* don't populate masked interrupt flags to IRQ_STATUS register */
-    uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_CTRL_1);
+    tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_CTRL_1);
     tmp &= ~(AT86RF2XX_TRX_CTRL_1_MASK__IRQ_MASK_MODE);
     at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_CTRL_1, tmp);
 #endif

Testing

drivers/cc110x: implements crc valid flag
drivers/cc2420: implements crc valid flag
drivers/at86rf2xx: implements crc valid flag

For at86rf2xx it needs to be setup in basic mode or packets are always rejected.

CFLAGS=-DAT86RF2XX_BASIC_MODE BOARD=iotlab-m3 make -C riot-applications/sniffer flash term

rftest-rx --- len 0000001C lqi FF rx_time 000000000928D8F8 crc 00
7B 3B 3A 02 85 00 E7 3C 00 00 00 00 01 02 32 8F F7 65 10 6B 11 14 00 00 00 00 00 00 

rftest-rx --- len 0000001C lqi FF rx_time 000000000C8EB8A6 crc 01
7B 3B 3A 02 85 00 48 E3 00 00 00 00 01 02 02 BC F6 65 10 6B 11 14 00 00 00 00 00 00 

rftest-rx --- len 0000001C lqi FF rx_time 000000000CBC5EF1 crc 00
7B 3B 3A 02 85 00 E7 3C 00 00 00 00 01 02 32 8F F7 65 10 6B 11 14 00 00 00 00 00 00 

rftest-rx --- len 0000001C lqi FF rx_time 000000001022405A crc 01
7B 3B 3A 02 85 00 48 E3 00 00 00 00 01 02 02 BC F6 65 10 6B 11 14 00 00 00 00 00 00 

rftest-rx --- len 0000001C lqi FF rx_time 00000000104FE4E8 crc 00
7B 3B 3A 02 85 00 E7 3C 00 00 00 00 01 02 32 8F F7 65 10 6B 11 14 00 00 00 00 00 00

drivers/at86rf215: implements crc valid flag
cpu/cc2538: implements crc valid flag
drivers/kw2xrf: set crc valid flag
drivers/kw41zrf: set crc valid flag

Invalid CRC are rejected.

BOARD=usbkw41z make -C riot-applications/sniffer flash term

2020-04-16 11:29:29,261 # rftest-rx --- len 0000001C lqi 9F rx_time 00000000013BD9FD crc 01
2020-04-16 11:29:29,268 # 7B 3B 3A 02 85 00 4F 37 00 00 00 00 01 02 00 12 4B 00 14 B5 B5 AF 00 00 00 00 00 00
2020-04-16 11:29:29,268 #
2020-04-16 11:29:36,302 # rftest-rx --- len 0000001C lqi 9D rx_time 0000000001A74D0F crc 01
2020-04-16 11:29:36,310 # 7B 3B 3A 02 85 00 4F 37 00 00 00 00 01 02 00 12 4B 00 14 B5 B5 AF 00 00 00 00 00 00 
2020-04-16 11:29:36,311 # 
2020-04-16 11:29:36,908 # rftest-rx --- len 0000001C lqi 9D rx_time 0000000001B08937 crc 01
2020-04-16 11:29:36,915 # 7B 3B 3A 02 85 00 4F 37 00 00 00 00 01 02 00 12 4B 00 14 B5 B5 AF 00 00 00 00 00 00 
2020-04-16 11:29:36,916 # 
2020-04-16 11:29:37,279 # rftest-rx --- len 0000001C lqi 9F rx_time 0000000001B63234 crc 01
2020-04-16 11:29:37,287 # 7B 3B 3A 02 85 00 4F 37 00 00 00 00 01 02 00 12 4B 00 14 B5 B5 AF 00 00 00 00 00 00 
2020-04-16 11:29:37,287 # 
2020-04-16 11:29:37,449 # rftest-rx --- len 0000001C lqi 9D rx_time 0000000001B8C757 crc 01
2020-04-16 11:29:37,455 # 7B 3B 3A 02 85 00 4F 37 00 00 00 00 01 02 00 12 4B 00 14 B5 B5 AF 00 00 00 00 00 00 
2020-04-16 11:29:37,456 #

drivers/mrf24j40: set crc valid flag
cpu/nrf52/radio/nrf802154: set crc valid flag

Non valid CRC are rejected

BOARD=nrf52840-mdkmake -C riot-applications/sniffer flash term

ftest-rx --- len 0000001C lqi 74 rx_time 000000000095F5DF crc 01
7B 3B 3A 02 85 00 9C E0 00 00 00 00 01 02 52 BD FC 65 10 6B 11 14 00 00 00 00 00 00 

rftest-rx --- len 0000001C lqi 70 rx_time 000000000222B551 crc 01
7B 3B 3A 02 85 00 9C E0 00 00 00 00 01 02 52 BD FC 65 10 6B 11 14 00 00 00 00 00 00

Issues/PRs references

Takeover of #8276.

drivers/cc110x/cc110x_rx_tx.c

jia200x · 2020-04-16T10:11:35Z

Great!

I have only one comment regarding the netif header. Do upper layers actually need the CRC flag?
At netdev level is expected because this is information that might be needed by MAC layers, but I'm not sure if the "GNRC_NETIF_HDR_FLAGS_CRC_VALID" has any use cases.

I know LQI and RSSI might be used by some routing protocols though, but I'm not aware of any use case for CRC there. Does any one know? @miri64 @roberthartung

fjmolinas · 2020-04-16T10:16:21Z

drivers/mrf24j40/mrf24j40_netdev.c

            mrf24j40_rx_fifo_read(dev, phr + 1, &(radio_info->lqi), 1);
            mrf24j40_rx_fifo_read(dev, phr + 2, &(rssi_scalar), 1);
            radio_info->rssi = mrf24j40_dbm_from_reg(rssi_scalar);
+            /* the driver currently does not support Packet Error Mode 


trailing whitespace

benpicco · 2020-04-16T10:21:21Z

Aren't most radio drivers configuring the radio to discard frames with invalid CRC automatically?

jia200x · 2020-04-16T10:47:44Z

Aren't most radio drivers configuring the radio to discard frames with invalid CRC automatically?

Yes, that's true. But some network stacks require the radios to be configured to always receive packets regardless the CRC (OpenWSN).

We would probably need compile time flags and or radio caps for this

fjmolinas · 2020-04-16T13:11:32Z

Yes, that's true. But some network stacks require the radios to be configured to always receive packets regardless the CRC (OpenWSN).

Yep, and as stated in the original PR there was a use case for:

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.

Forward Error Correction is a regular use case

fjmolinas · 2020-04-16T13:13:41Z

Aren't most radio drivers configuring the radio to discard frames with invalid CRC automatically?

We would probably need compile time flags and or radio caps for this

yep, there was a PR on this a while ago. But this is currently not the case for all drivers and not very clear either, for some radios it will depend on the operating mode or some netops. But it is true that most drivers either don't accept invalid CRC altogether, or out implementation does that. All the commits that say set valid crc flag and not implement are because of that.

drivers/include/net/netdev.h

maribu · 2020-04-23T14:30:15Z

Forward Error Correction is a regular use case

Hmm, is this likely to happen on the upper layers? E.g. the CC1101 can do FEC in hardware; also BLE since version 5 does so in hardware. And if I had to add FEC in software, I would try to do that within the netdev and try to disable the layer 2 FCS altogether, if the hardware does support this. That way every network stack could transparently benefit from this. (And by properly implementing this as a common module, a handful of lines would be needed to be added to the netdev code.)

One use case that comes to my mind is UDP-Lite, but I never heard of someone using this in a real world scenario.

But some network stacks require the radios to be configured to always receive packets regardless the CRC (OpenWSN).

Interesting. Why so?

maribu

I only had a brief look. But if I didn't miss anything, this would now pass corrupted frames to the upper layers (but marked a such) instead of dropping them. Wouldn't we need to add code that checks for the flag (or more precisely the lack of it) and drops corrupted frames in the layers above instead?

maribu · 2020-04-23T14:31:59Z

drivers/at86rf2xx/at86rf2xx_netdev.c

     */
    if (info != NULL) {
        uint8_t ed = 0;
+        uint8_t crc = 0;


Suggested change

uint8_t crc = 0;

uint8_t crc_valid = 0;

Just crc is a misleading name. I'd assume it would contain the value of the CRC.

fjmolinas · 2020-04-23T15:03:44Z

I only had a brief look. But if I didn't miss anything, this would now pass corrupted frames to the upper layers (but marked a such) instead of dropping them. Wouldn't we need to add code that checks for the flag (or more precisely the lack of it) and drops corrupted frames in the layers above instead?

Yes you are right.

benpicco · 2020-04-23T15:15:25Z

But if I didn't miss anything, this would now pass corrupted frames to the upper layers (but marked a such) instead of dropping them.

IIRC this is not enabled yet (and shouldn't be enabled by default)
I think it would also be good to have this compiled conditionally, since most use-cases won't make use of this.

maribu · 2020-04-23T15:22:20Z

But if I didn't miss anything, this would now pass corrupted frames to the upper layers (but marked a such) instead of dropping them.

IIRC this is not enabled yet (and shouldn't be enabled by default)
I think it would also be good to have this compiled conditionally, since most use-cases won't make use of this.

OK, this is better! (And unless no one is using two or more distinct network stacks in the same firmware, there is no need to e.g. add dropping of corrupted frames e.g. to GNRC; it will just never enable passing corrupted frames upward.)

Add CRC_VALID flag to the new option.

fjmolinas · 2020-06-26T13:05:43Z

Rebased and addressed current comments.

fjmolinas · 2020-06-26T13:14:06Z

@maribu @benpicco so what still need to be done here? By default invalid CRC are always dropped, some configurations can enable them like PROMISCUOUS for cc2538, and BASIC_MODE for at86rf2xx.

fjmolinas · 2020-06-26T13:16:23Z

@maribu @benpicco so what still need to be done here? By default invalid CRC are always dropped, some configurations can enable them like PROMISCUOUS for cc2538, and BASIC_MODE for at86rf2xx.

Besides testing.

fjmolinas · 2020-07-24T15:30:43Z

@fjmolinas you should test this on monday (not sure if a self tag notifies myself)

aabadie · 2020-07-24T15:33:19Z

not sure if a self tag notifies myself

ping @fjmolinas :D

stale · 2021-01-30T10:56:59Z

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.

fjmolinas added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Apr 16, 2020

fjmolinas requested review from jia200x and miri64 April 16, 2020 09:40

fjmolinas requested review from PeterKietzmann, benpicco, bergzand, cgundogan, daniel-k, haukepetersen and maribu as code owners April 16, 2020 09:40

fjmolinas requested a review from a user April 16, 2020 09:40

fjmolinas requested a review from smlng as a code owner April 16, 2020 09:40

fjmolinas commented Apr 16, 2020

View reviewed changes

drivers/cc110x/cc110x_rx_tx.c Outdated Show resolved Hide resolved

aabadie reviewed Apr 16, 2020

View reviewed changes

drivers/cc110x/cc110x_rx_tx.c Outdated Show resolved Hide resolved

fjmolinas commented Apr 16, 2020

View reviewed changes

benpicco reviewed Apr 17, 2020

View reviewed changes

drivers/include/net/netdev.h Outdated Show resolved Hide resolved

fjmolinas force-pushed the pr_netdev_crc branch from cf208c2 to 56b49b0 Compare April 22, 2020 10:44

fjmolinas mentioned this pull request Apr 23, 2020

pkg/openwsn: re-integrate the network stack as a package #13824

Merged

11 tasks

maribu reviewed Apr 23, 2020

View reviewed changes

fjmolinas added 19 commits June 26, 2020 14:37

drivers/kw2xrf: set crc valid flag

23396b5

drivers/kw41zrf: set crc valid flag

b8b2f8c

drivers/mrf24j40: set crc valid flag

62bcc9b

cpu/nrf52/radio/nrf802154: set crc valid flag

b106d85

drivers/cc110x: set crc valid flag

74aacc7

fixup! drivers/cc110x: set crc valid flag

d835b28

fixup! cpu/nrf52/radio/nrf802154: set crc valid flag

856303c

fixup! drivers/mrf24j40: set crc valid flag

3cdbb3e

fixup! drivers/kw41zrf: set crc valid flag

e917da7

fixup! drivers/kw2xrf: set crc valid flag

093db7b

fixup! cpu/cc2538: implements crc valid flag

131f8ec

fixup! drivers/at86rf215: implements crc valid flag

e7015f6

fixup! drivers/at86rf2xx: implements crc valid flag

98ee379

fixup! drivers/cc2420: implements crc valid flag

1d158af

squash! drivers/netdev: add flag field to radio_rx_info

d32a665

Add CRC_VALID flag to the new option.

fixup! gnrc/netif: implements crc valid flag

344995d

fixup! fixup! drivers/at86rf2xx: implements crc valid flag

25a163f

fixup! fixup! drivers/mrf24j40: set crc valid flag

014c8eb

fixup! fixup! drivers/cc110x: set crc valid flag

8095352

fjmolinas force-pushed the pr_netdev_crc branch from d7cc2fd to 8095352 Compare June 26, 2020 13:05

fixup! fixup! fixup! drivers/at86rf2xx: implements crc valid flag

9225c9e

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

stale bot closed this Mar 19, 2021

fjmolinas deleted the pr_netdev_crc branch June 9, 2022 09:36

Conversation

fjmolinas commented Apr 16, 2020

Contribution description

Testing procedure

Setup

Testing

Issues/PRs references

Uh oh!

Uh oh!

Uh oh!

jia200x commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjmolinas Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco commented Apr 16, 2020

Uh oh!

jia200x commented Apr 16, 2020

Uh oh!

fjmolinas commented Apr 16, 2020

Uh oh!

fjmolinas commented Apr 16, 2020

Uh oh!

Uh oh!

maribu commented Apr 23, 2020

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

maribu Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Apr 23, 2020

Uh oh!

benpicco commented Apr 23, 2020

Uh oh!

maribu commented Apr 23, 2020

Uh oh!

fjmolinas commented Jun 26, 2020

Uh oh!

fjmolinas commented Jun 26, 2020

Uh oh!

fjmolinas commented Jun 26, 2020

Uh oh!

fjmolinas commented Jul 24, 2020

Uh oh!

aabadie commented Jul 24, 2020

Uh oh!

stale bot commented Jan 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jia200x commented Apr 16, 2020 •

edited

Loading