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

drivers: add kw41zrf #12277

Merged
merged 2 commits into from
Mar 19, 2020
Merged

drivers: add kw41zrf #12277

merged 2 commits into from
Mar 19, 2020

Conversation

benemorius
Copy link
Member

@benemorius benemorius commented Sep 20, 2019

Contribution description

Taken from #7107 by @gebart:

This is the radio found in NXP Kinetis KW41Z, KW21Z. Only 802.15.4 mode
is implemented (KW41Z also supports BLE on the same transceiver).

The driver uses vendor supplied initialization code for the low level
XCVR hardware, these files were imported from mcuxpresso.nxp.com (KSDK 2.2.0, framework_5.3.5)

The reason for using the vendor code is that setting up the XCVR module requires a lot of precalculated values which I don't have time to recreate. The vendor code works and was imported with minimal modifications to support easy updates if a new version comes out.

Tested with two FRDM-KW41Z boards running gnrc_networking example

Since #7107 I fixed the known bugs that were reported there and I've used it extensively for months. By now I think it's getting to be a pretty mature radio driver.

Testing procedure

Confirm gnrc_networking example works as intended on an frdm-kw41z or usb-kw41z or other kw41z board.

Issues/PRs references

Taken from #7107. Compatible with (but does not depend on) #11789.

@aabadie aabadie requested a review from jia200x September 20, 2019 05:49
@aabadie aabadie added Area: drivers Area: Device drivers Area: network Area: Networking labels Sep 20, 2019
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 20, 2019
@aabadie aabadie added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Sep 23, 2019
@PeterKietzmann
Copy link
Member

@Ilmu011 mind to review the code? You could test it as well on a new phytec board or the kw41 mini

@Ilmu011
Copy link

Ilmu011 commented Sep 25, 2019

Originally posted by @benemorius in #10364

By the way there's a bug in the #7107 CSMA implementation that prevents any retransmissions from actually happening if the initial CCA fails.

After _isr_event_seq_t_ccairq() calls kw41zrf_tx_exec() to retransmit the last frame, _isr_event_seq_tr() cancels the transmission by calling kw41zrf_abort_sequence() while handling ZLL_IRQSTS_SEQIRQ_MASK.

But with that fixed I think it works as intended.

Isn't this still the case?

@benemorius
Copy link
Member Author

Originally posted by @benemorius in #10364

By the way there's a bug in the #7107 CSMA implementation that prevents any retransmissions from actually happening if the initial CCA fails.
After _isr_event_seq_t_ccairq() calls kw41zrf_tx_exec() to retransmit the last frame, _isr_event_seq_tr() cancels the transmission by calling kw41zrf_abort_sequence() while handling ZLL_IRQSTS_SEQIRQ_MASK.
But with that fixed I think it works as intended.

Isn't this still the case?

No I resolved that by having _isr_event_seq_t_ccairq() handle the SEQIRQ and then masking the already-handled IRQs when calling _isr_event_seq_tr().

@PeterKietzmann
Copy link
Member

@benemorius the PR is big. Could you indicate the fix next to the code?

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

At a first glance, the code looks good.

I tested the driver with a kw41z-mini. There are two other nodes with a mrf24j40 radio.
However, I have trouble receiving on the kw41z-mini:

kw41z-mini

2019-10-11 22:11:25,201 # Iface  7  HWaddr: 6D:56  Channel: 26  Page: 0  NID: 0x23
2019-10-11 22:11:25,202 #           Long HWaddr: 23:64:63:2B:62:41:6D:56 
2019-10-11 22:11:25,205 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-11 22:11:25,206 #           ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2019-10-11 22:11:25,207 #           6LO  IPHC  
2019-10-11 22:11:25,208 #           Source address length: 8
2019-10-11 22:11:25,209 #           Link type: wireless
2019-10-11 22:11:25,211 #           inet6 addr: fe80::2164:632b:6241:6d56  scope: local  VAL
2019-10-11 22:11:25,212 #           inet6 group: ff02::2
2019-10-11 22:11:25,213 #           inet6 group: ff02::1
2019-10-11 22:11:25,242 #           inet6 group: ff02::1:ff41:6d56
2019-10-11 22:11:25,243 #           inet6 group: ff02::1a
2019-10-11 22:11:25,243 #           
2019-10-11 22:11:25,244 #           Statistics for Layer 2
2019-10-11 22:11:25,245 #             RX packets 9  bytes 359
2019-10-11 22:11:25,247 #             TX packets 5 (Multicast: 5)  bytes 201
2019-10-11 22:11:25,248 #             TX succeeded 5 errors 0
2019-10-11 22:11:25,249 #           Statistics for IPv6
2019-10-11 22:11:25,250 #             RX packets 5  bytes 291
2019-10-11 22:11:25,251 #             TX packets 5 (Multicast: 5)  bytes 306
2019-10-11 22:11:25,251 #             TX succeeded 5 errors 0
2019-10-11 22:11:25,251 # 
udp server start 1111
2019-10-11 22:11:50,944 #  udp server start 1111
2019-10-11 22:11:50,945 # Success: started UDP server on port 1111
> ping6 ff02::1
2019-10-11 22:12:02,017 #  ping6 ff02::1
2019-10-11 22:12:05,003 # 
2019-10-11 22:12:05,025 # --- ff02::1 PING statistics ---
2019-10-11 22:12:05,027 # 3 packets transmitted, 0 packets received, 100% packet loss
> ping6 fe80::2123:2323:2323:2322
2019-10-11 22:12:11,650 #  ping6 fe80::2123:2323:2323:2322
2019-10-11 22:12:14,665 # 
2019-10-11 22:12:14,666 # --- fe80::2123:2323:2323:2322 PING statistics ---
2019-10-11 22:12:14,668 # 3 packets transmitted, 0 packets received, 100% packet loss
> udp send fe80::2123:2323:2323:2322 2222 Hello
2019-10-11 22:12:26,280 #  udp send fe80::2123:2323:2323:2322 2222 Hello
2019-10-11 22:12:26,296 # Success: sent 5 byte(s) to [fe80::2123:2323:2323:2322]:2222

ek-lm4f120xl_mrf24j40

2019-10-11 22:11:29,505 # Iface  7  HWaddr: 23:22  Channel: 26  Page: 0  NID: 0x23
2019-10-11 22:11:29,509 #           Long HWaddr: 23:23:23:23:23:23:23:22 
2019-10-11 22:11:29,515 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-11 22:11:29,521 #           ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2019-10-11 22:11:29,522 #           6LO  IPHC  
2019-10-11 22:11:29,526 #           Source address length: 8
2019-10-11 22:11:29,528 #           Link type: wireless
2019-10-11 22:11:29,534 #           inet6 addr: fe80::2123:2323:2323:2322  scope: local  VAL
2019-10-11 22:11:29,537 #           inet6 group: ff02::2
2019-10-11 22:11:29,540 #           inet6 group: ff02::1
2019-10-11 22:11:29,543 #           inet6 group: ff02::1:ff23:2322
2019-10-11 22:11:29,546 #           inet6 group: ff02::1a
2019-10-11 22:11:29,547 #           
2019-10-11 22:11:29,550 #           Statistics for Layer 2
2019-10-11 22:11:29,553 #             RX packets 6  bytes 244
2019-10-11 22:11:29,557 #             TX packets 4 (Multicast: 4)  bytes 158
2019-10-11 22:11:29,560 #             TX succeeded 4 errors 0
2019-10-11 22:11:29,563 #           Statistics for IPv6
2019-10-11 22:11:29,566 #             RX packets 6  bytes 370
2019-10-11 22:11:29,570 #             TX packets 4 (Multicast: 4)  bytes 242
2019-10-11 22:11:29,574 #             TX succeeded 4 errors 0
2019-10-11 22:11:29,574 # 
> udp server start 2222
2019-10-11 22:11:56,725 #  udp server start 2222
2019-10-11 22:11:56,728 # Success: started UDP server on port 2222
> 2019-10-11 22:12:26,283 #  PKTDUMP: data received:
2019-10-11 22:12:26,288 # ~~ SNIP  0 - size:   5 byte, type: NETTYPE_UNDEF (0)
2019-10-11 22:12:26,291 # 00000000  48  65  6C  6C  6F
2019-10-11 22:12:26,295 # ~~ SNIP  1 - size:   8 byte, type: NETTYPE_UDP (4)
2019-10-11 22:12:26,298 #    src-port:  2222  dst-port:  2222
2019-10-11 22:12:26,300 #    length: 13  cksum: 0xeef1
2019-10-11 22:12:26,305 # ~~ SNIP  2 - size:  40 byte, type: NETTYPE_IPV6 (2)
2019-10-11 22:12:26,309 # traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
2019-10-11 22:12:26,311 # flow label: 0x00000
2019-10-11 22:12:26,314 # length: 13  next header: 17  hop limit: 64
2019-10-11 22:12:26,318 # source address: fe80::2164:632b:6241:6d56
2019-10-11 22:12:26,322 # destination address: fe80::2123:2323:2323:2322
2019-10-11 22:12:26,327 # ~~ SNIP  3 - size:  24 byte, type: NETTYPE_NETIF (-1)
2019-10-11 22:12:26,329 # if_pid: 7  rssi: -35  lqi: 117
2019-10-11 22:12:26,330 # flags: 0x0
2019-10-11 22:12:26,334 # src_l2addr: 23:64:63:2B:62:41:6D:56
2019-10-11 22:12:26,336 # dst_l2addr: 23:23:23:23:23:23:23:22
2019-10-11 22:12:26,340 # ~~ PKT    -  4 snips, total size:  77 byte
ping6 ff02::1
2019-10-11 22:12:39,660 # ping6 ff02::1
2019-10-11 22:12:39,676 # 12 bytes from fe80::686a:5673:a259:c87e: icmp_seq=0 ttl=64 rssi=-38 dBm time=9.303 ms # this is the blackpill-128_mrf24j40
2019-10-11 22:12:40,676 # 12 bytes from fe80::686a:5673:a259:c87e: icmp_seq=1 ttl=64 rssi=-38 dBm time=8.656 ms
2019-10-11 22:12:41,676 # 12 bytes from fe80::686a:5673:a259:c87e: icmp_seq=2 ttl=64 rssi=-38 dBm time=8.984 ms
2019-10-11 22:12:41,676 # 
2019-10-11 22:12:41,678 # --- ff02::1 PING statistics ---
2019-10-11 22:12:41,684 # 3 packets transmitted, 3 packets received, 0% packet loss
2019-10-11 22:12:41,688 # round-trip min/avg/max = 8.656/8.981/9.303 ms
> udp send fe80::2164:632b:6241:6d56 1111 Test
2019-10-11 22:12:54,030 #  udp send fe80::2164:632b:6241:6d56 1111 Test
2019-10-11 22:12:54,036 # Success: sent 4 byte(s) to [fe80::2164:632b:6241:6d56]:1111

drivers/kw41zrf/include/kw41zrf_getset.h Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Outdated Show resolved Hide resolved
@fjmolinas
Copy link
Contributor

@benemorius thanks for taking over @gebart PR, its been a long awaited FEATYRE, it would be really nice to see this driver in soon. @benpicco seemed to have some initial issues, but If you have time to go back to this PR I can help with testing as well and figure out how to push this forward.

@benemorius
Copy link
Member Author

I noticed finally that CSMA was causing a serious bug with this.

Actually, anytime the radio tries to backoff and retransmit, it ends up stuck not receiving until after the next time it transmits (supposing it doesn't need CSMA that time too). So it's worse than just not having CSMA. It's been the cause of a lot of unexpectedly silent nodes for me. In fact disabling CSMA like ifconfig 7 -csma was the workaround I was accidentally using to avoid seeing this bug.

I tried rather hard to get the hardware CSMA in the radio working as intended but I can't get the transmission to begin any time other than immediately. The reference manual isn't the most efficient.

Instead I've implemented software CSMA in the driver. This seems to have resolved the silent nodes I used to get from leaving CSMA turned on.

I'm getting the refactoring cleaned up and then I'll push an update.

@benpicco
Copy link
Contributor

kw41r-mini is merged, please rebase

@benemorius
Copy link
Member Author

rebased

@benpicco
Copy link
Contributor

benpicco commented Mar 1, 2020

Now this is getting weird.
I tried this branch on two boards now (one with kw41z256, one with kw41z512) and neither of them would receive anything.

But the thing is it would receive packets with the RIOT firmware that came with the board (2019.10-devel-148-g0d867-cleanthismess) - but even when I flash examples/gnrc_networking from your openlabs branch, I still get nothing.

@benemorius
Copy link
Member Author

@benpicco can you try the latest commit?

I was able to reproduce failed packet reception only on the gcc that ships with raspbian. I think you found the bug I once had and was looking for but couldn't remember the details of. For me the latest commit fixes it.

@benpicco
Copy link
Contributor

benpicco commented Mar 2, 2020

That fixes it indeed - RX is working now!

drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Outdated Show resolved Hide resolved
@benpicco
Copy link
Contributor

benpicco commented Mar 5, 2020

FYI: netif auto_init got moved to sys/net/gnrc/netif/init_devs/init.c.

drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

First round of review, looks pretty good!

Just something strange I've noticed: when I run the examples/gnrc_border_router example on a second node (samr21-xpro), the kw41z-mini won't get a public IP while other nodes (e.g. avr-rss2 or openmote-b) do.

I can ping the link-local address of the border router and I can ping the link-local address of the kw41z-mini just fine, it also answers to broadcast ping and fragmented pings, so this is odd.

Especially since with the original 2019.10-devel-148-g0d867-cleanthismess image this is working, but not with your telnet-shell branch.

drivers/kw41zrf/kw41zrf.c Show resolved Hide resolved
drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Show resolved Hide resolved
Comment on lines 75 to 76
if (num_irqs_queued == num_irqs_handled) {
++num_irqs_queued;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get this dance - can num_irqs_queued ever be something other an 0 or 1?
I think a atomic_bool or atomic_int would work here as well.

drivers/kw41zrf/kw41zrf_netdev.c Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Outdated Show resolved Hide resolved
drivers/kw41zrf/kw41zrf_netdev.c Show resolved Hide resolved
Comment on lines 58 to 60
/* The implementations for these functions are taken from the vendor provided XCVR
* driver from NXP MCUXpresso. The code has been refactored to eliminate a lot
* of preprocessor conditionals. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still know which version of MCUXpresso this was taken from?
Unless by necessity it's usually a bad idea to modify the vendor files, since it makes updating them impossible.

Copy link
Member Author

Choose a reason for hiding this comment

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

I certainly agree in general. My understanding from reading the dialog in #7107 is that this was necessary or at least prohibitively inconvenient to avoid.

The version (which I can't confirm personally) is mentioned in the PR only. I'll add it to this comment.

@benemorius
Copy link
Member Author

when I run the examples/gnrc_border_router example on a second node (samr21-xpro), the kw41z-mini won't get a public IP while other nodes (e.g. avr-rss2 or openmote-b) do.

Ok so you're saying that with a samr21-xpro as border router, everything but the kw41z-mini gets an IP address, is that right?

Is the kw41z-mini firmware the only one you're using raspbian to compile? With the first bug you found (broken packet rx) the "fix" was to convert a memcpy to a manual memcpy with a for loop and this makes me suspicious that that wasn't the only bug that will appear with raspbian-compiled firmwares. I didn't actually dig down to find the real source of the bug after finding the workaround.

Especially since with the original 2019.10-devel-148-g0d867-cleanthismess image this is working, but not with your telnet-shell branch.

I've cleaned up my local branch finally and now openlabs points to the same firmwares I compile locally so that should eliminate any differences between what comes flashed on the boards and what you can compile locally.

@benpicco
Copy link
Contributor

benpicco commented Mar 16, 2020

Ok so you're saying that with a samr21-xpro as border router, everything but the kw41z-mini gets an IP address, is that right?

Yes, that's the observation.

Is the kw41z-mini firmware the only one you're using raspbian to compile? With the first bug you found (broken packet rx) the "fix" was to convert a memcpy to a manual memcpy with a for loop and this makes me suspicious that that wasn't the only bug that will appear with raspbian-compiled firmwares. I didn't actually dig down to find the real source of the bug after finding the workaround.

I'm using arm-none-eabi-gcc (15:7-2018-q2-6) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907] as shipped by Ubuntu 19.10

I've cleaned up my local branch finally and now openlabs points to the same firmwares I compile locally so that should eliminate any differences between what comes flashed on the boards and what you can compile locally.

There are some nice goodies in that branch! I'm keen on that telnet server and ZBoss pkg, but also small fixes like 9b20f53 would be nice to have upstream 😉
I see the same behavior as with this this branch though - which compiler do you use?

@fjmolinas
Copy link
Contributor

I used this PR to test #13486 on a usb-kw41z, works like a charm, was able to perform a full OTA update (its getting a public ipv6 address as well):

2020-03-18 10:18:50,117 # ifconfig
2020-03-18 10:18:50,122 # Iface  6  HWaddr: 11:6E  Channel: 26  Page: 0  NID: 0x23
2020-03-18 10:18:50,127 #           Long HWaddr: 22:63:63:2B:62:2A:6D:52 
2020-03-18 10:18:50,133 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2020-03-18 10:18:50,139 #           ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2020-03-18 10:18:50,142 #           RTR_ADV  6LO  IPHC  
2020-03-18 10:18:50,145 #           Source address length: 8
2020-03-18 10:18:50,148 #           Link type: wireless
2020-03-18 10:18:50,154 #           inet6 addr: fe80::2063:632b:622a:6d52  scope: link  VAL
2020-03-18 10:18:50,160 #           inet6 addr: 2001:db8::2063:632b:622a:6d52  scope: global  VAL
2020-03-18 10:18:50,163 #           inet6 group: ff02::2
2020-03-18 10:18:50,165 #           inet6 group: ff02::1
2020-03-18 10:18:50,169 #           inet6 group: ff02::1:ff2a:6d52
2020-03-18 10:18:50,170 #           
> 2020-03-18 10:19:49,657 #  suit: received URL: "coap://[2001:db8::1]/fw/usb-kw41z/suit_update-riot.suitv3_signed.latest.bin"
2020-03-18 10:19:49,660 # suit_coap: trigger received
2020-03-18 10:19:49,669 # suit_coap: downloading "coap://[2001:db8::1]/fw/usb-kw41z/suit_update-riot.suitv3_signed.latest.bin"
2020-03-18 10:19:50,056 # suit_coap: got manifest with size 458
2020-03-18 10:19:50,059 # suit: verifying manifest signature
2020-03-18 10:19:54,529 # suit: validated manifest version
2020-03-18 10:19:54,534 # )riotboot_hdr_validate: riotboot_hdr magic number invalid
2020-03-18 10:19:54,537 # suit: validated sequence number
2020-03-18 10:19:54,539 # )validating vendor ID
2020-03-18 10:19:54,548 # Comparing 547d0d74-6d3a-5a92-9662-4881afd9407b to 547d0d74-6d3a-5a92-9662-4881afd9407b from manifest
2020-03-18 10:19:54,550 # validating vendor ID: OK
2020-03-18 10:19:54,552 # validating class id
2020-03-18 10:19:54,560 # Comparing 38204045-80d5-5db8-a8a8-16afa1033217 to 38204045-80d5-5db8-a8a8-16afa1033217 from manifest
2020-03-18 10:19:54,563 # validating class id: OK
2020-03-18 10:19:54,568 # Comparing manifest offset 4096 with other slot offset 264192
2020-03-18 10:19:54,574 # Comparing manifest offset 264192 with other slot offset 264192
2020-03-18 10:19:54,579 # Comparing manifest offset 4096 with other slot offset 264192
2020-03-18 10:19:54,585 # Comparing manifest offset 264192 with other slot offset 264192
2020-03-18 10:19:54,590 # riotboot_flashwrite: initializing update to target slot 1
2020-03-18 10:19:54,638 # Fetching firmware |█████████████████████████| 100%
2020-03-18 10:21:29,352 # Verifying image digest
2020-03-18 10:21:29,358 # riotboot: verifying digest at 0x1fffb84f (img at: 0x40800 size: 96632)
2020-03-18 10:21:29,709 # Verifying image digest
2020-03-18 10:21:29,715 # riotboot: verifying digest at 0x1fffb84f (img at: 0x40800 size: 96632)
2020-03-18 10:21:30,067 # suit_parse() success
2020-03-18 10:21:30,069 # SUIT policy check OK.
2020-03-18 10:21:30,072 # suit_coap: finalizing image flash
2020-03-18 10:21:30,114 # riotboot_flashwrite: riotboot flashing completed successfully
2020-03-18 10:21:30,117 # Image magic_number: 0x544f4952
2020-03-18 10:21:30,119 # Image Version: 0x5e71e784
2020-03-18 10:21:30,122 # Image start address: 0x00040900
2020-03-18 10:21:30,124 # Header chksum: 0x28e6ec9b
2020-03-18 10:21:30,124 # 
2020-03-18 10:21:31,135 # main(): This is RIOT! (Version: 2020.04-devel-1336-gab8c6-pr-13486)
2020-03-18 10:21:31,140 # RIOT SUIT update example application
2020-03-18 10:21:31,141 # running from slot 1
2020-03-18 10:21:31,143 # slot start addr = 0x40800
2020-03-18 10:21:31,146 # Image magic_number: 0x544f4952
2020-03-18 10:21:31,149 # Image Version: 0x5e71e784
2020-03-18 10:21:31,152 # Image start address: 0x00040900
2020-03-18 10:21:31,154 # Header chksum: 0x28e6ec9b
2020-03-18 10:21:31,154 # 
2020-03-18 10:21:31,156 # suit_coap: started.
2020-03-18 10:21:31,157 # Starting the shell

@benpicco
Copy link
Contributor

@fjmolinas what compiler did you use?

@fjmolinas
Copy link
Contributor

Installed compiler toolchains
-----------------------------
               native gcc: gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
        arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 8-2018-q4-major) 8.2.1 20181213 (release) [gcc-8-branch revision 267074]
                  avr-gcc: avr-gcc (GCC) 5.4.0
         mips-mti-elf-gcc: missing
               msp430-gcc: msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)
     riscv-none-embed-gcc: riscv-none-embed-gcc (GNU MCU Eclipse RISC-V Embedded GCC, 64-bit) 8.2.0
     xtensa-esp32-elf-gcc: missing
   xtensa-esp8266-elf-gcc: xtensa-esp8266-elf-gcc (crosstool-NG crosstool-ng-1.22.0-80-g6c4433a5) 5.2.0
                    clang: missing

@fjmolinas
Copy link
Contributor

@fjmolinas what compiler did you use?

I could try with BUILD_IN_DOCKER as well.

@fjmolinas
Copy link
Contributor

@fjmolinas what compiler did you use?

I could try with BUILD_IN_DOCKER as well.

Works for me when built in docker as well.

@benemorius
Copy link
Member Author

I'm able to reproduce the behavior where ping works but no public IP address appears. It's because the router solicitations are coming out malformed for some reason.

Screenshot_20200318_150116

I can reproduce it with this compiler:

Operating System Environment
----------------------------
         Operating System: "Raspbian GNU/Linux" "10 (buster)"
                   Kernel: Linux 4.19.93-v7+ armv7l unknown
             System shell: /bin/dash (probably dash)
             make's shell: /bin/dash (probably dash)

Installed compiler toolchains
-----------------------------
               native gcc: gcc (Raspbian 8.3.0-6+rpi1) 8.3.0
        arm-none-eabi-gcc: arm-none-eabi-gcc (15:7-2018-q2-6) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907]

I can't reproduce it with the compiler I use normally:

Operating System Environment
----------------------------
         Operating System: "Ubuntu" "18.04.4 LTS (Bionic Beaver)"
                   Kernel: Linux 5.3.0-40-generic x86_64 x86_64
             System shell: /bin/dash (probably dash)
             make's shell: /bin/dash (probably dash)

Installed compiler toolchains
-----------------------------
               native gcc: gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
        arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 9-2019-q4-major) 9.2.1 20191025 (release) [ARM/arm-9-branch revision 277599]

@benemorius
Copy link
Member Author

benemorius commented Mar 18, 2020

@benpicco can you try the latest commit?

Once again the workaround was to convert a memcpy to a for loop. I have no other explanation for this so far. Apparently there's something odd about copying to/from the radio hardware's packet buffer because the two memcpy I've deleted now are the ones that get/put the packet buffer.

@benpicco
Copy link
Contributor

benpicco commented Mar 18, 2020

Yes that fixes it indeed!
That's interesting - maybe memcpy is doing some optimizations in how it reads the memory that the hardware doesn't like - I'd expect it to at least always read whole words when possible, but the hardware expects the buffer to be read byte-wise and sequentially?
So it might not even be a compiler bug but a hardware limitation.

@benpicco
Copy link
Contributor

This driver is looking pretty good now - but using volatile for synchronization just smells like a race condition waiting to happen. Can you please document how num_irqs_queued is supposed to work?

@benemorius
Copy link
Member Author

Can you please document how num_irqs_queued is supposed to work?

I changed it to use atomic_bool and I think it's more readable now. Tell me if you think otherwise.


if (!spinning_for_irq) {
irq_is_queued = false;
if (!blocking_for_irq) {
thread_flags_clear(KW41ZRF_THREAD_FLAG_ISR);
Copy link
Contributor

Choose a reason for hiding this comment

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

So blocking_for_irq is set when this function gets called from kw41zrf_wait_idle() - when are those flags then cleared?

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking at it I don't think that if should even be there. Maybe it used to be needed but now I think it's a bug so I removed it. That leaves blocking_for_irq being used only for an assertion but I didn't want to remove that one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing those spurious gnrc_netif: possibly lost interrupt anymore - nice!

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

My comments have been addressed and the driver is working fine - please squash!

Joakim Nohlgård added 2 commits March 19, 2020 17:00
This is the radio found in NXP Kinetis KW41Z, KW21Z. Only 802.15.4 mode
is implemented (KW41Z also supports BLE on the same transceiver).

The driver uses vendor supplied initialization code for the low level
XCVR hardware, these files were imported from KSDK 2.2.0 (framework_5.3.5)
@benpicco benpicco merged commit 5435792 into RIOT-OS:master Mar 19, 2020
@benemorius
Copy link
Member Author

Thanks very much for the review and testing and bug reporting!

@benemorius benemorius deleted the pr/kw41zrf branch March 20, 2020 00:28
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants