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 support for KW41Z builtin transceiver #7107

Closed
wants to merge 82 commits into from

Conversation

jnohlgard
Copy link
Member

@jnohlgard jnohlgard commented May 30, 2017

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 Kinetis Connectivity
Software version 1.0.2 (framework_5.3.2)
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

Depends on: #6994 #6995 #6916 #6978 #7362 #7897

@jnohlgard jnohlgard 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 May 30, 2017
@jnohlgard jnohlgard added this to the Release 2017.07 milestone May 30, 2017
@aabadie
Copy link
Contributor

aabadie commented Jun 21, 2017

Is there someone with the hardware that can review and test this ? @kYc0o ?

@jnohlgard jnohlgard added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 21, 2017
@aabadie aabadie added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 22, 2017
@jnohlgard
Copy link
Member Author

Rebased

@jnohlgard
Copy link
Member Author

rebased

@jnohlgard
Copy link
Member Author

I have observed random hard faults with this driver, I think the stack usage is to blame. I will investigate further

@jnohlgard jnohlgard force-pushed the pr/kw41zrf branch 3 times, most recently from 517112c to 91e106e Compare August 6, 2017 19:59
@jnohlgard
Copy link
Member Author

Rebased on latest master

@jia200x
Added radio deps to phynode-kw41z config, untested.

@jia200x
Copy link
Member

jia200x commented Dec 14, 2018

@jia200x
Added radio deps to phynode-kw41z config, untested.

Thank you @gebart !

@jia200x
Copy link
Member

jia200x commented Dec 14, 2018

basic testing on phynode-kw41z is going ok so far. I tried basic commands (channels, etc) and works as expected. I will do more stress testing :)

@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

the radio has ben running for 18 days and haven't seen any crash. The issue described before might not be present anymore

@jnohlgard
Copy link
Member Author

I have seen a hard to trace problem where the netif thread hangs in the wait_idle function, waiting for an IRQ which never comes. It seems to be related to state switching because I sometimes see IRQ triggers right when switching sequences. It is difficult to debug and I have not yet found the cause.

@taherrera
Copy link

Hi, there is a bug in this implementation, the order of the bytes to set the short address in kw41zrf_set_addr_short(...) in kw41zrf_getset.c should be something like this:

 ZLL->MACSHORTADDRS0 = (ZLL->MACSHORTADDRS0 & ~ZLL_MACSHORTADDRS0_MACSHORTADDRS0_MASK) |
       ZLL_MACSHORTADDRS0_MACSHORTADDRS0( ((addr&0xff)<<8) | (addr>>8));

Nice work btw ! @gebart

@benemorius
Copy link
Member

benemorius commented May 31, 2019

Looks like short addresses work correctly now, just they are not shown by ifconfig.

@@ -615,12 +615,13 @@ int kw41zrf_netdev_get(netdev_t *netdev, netopt_t opt, void *value, size_t len)
             break;
 
         case NETOPT_ADDRESS:
-            if (len != sizeof(uint16_t)) {
+            if (len < sizeof(uint16_t)) {
                 res = -EOVERFLOW;
                 break;
             }
             *((uint16_t *)value) = kw41zrf_get_addr_short(dev);
-            res = len;
+            res = sizeof(uint16_t);
             break;
 
         case NETOPT_ADDRESS_LONG:

ifconfig uses the same uint8_t[GNRC_NETIF_L2ADDR_MAXLEN] buffer to get both long and short addresses, so for the short address it ends up calling netdev_get with a size of 8 rather than 2.

@benemorius
Copy link
Member

@gebart

I have seen a hard to trace problem where the netif thread hangs in the wait_idle function, waiting for an IRQ which never comes.

I've seen these indefinite IRQ waits too. I noticed at least that they aren't all happening from any one specific transceiver state. At one point I was unintentionally getting close to having a repeatable test to reproduce it, but in normal use I only see it very rarely. I will look for a solution next time it starts happening more.

However, I don't think it's bad enough to block the PR. I am finding this reliable enough that I've stopped using a watchdog timer in desperation to find more crashes.

The discussion here is getting almost too long to read thoroughly, so I have to apologize for asking: is anything blocking this PR from proceeding now that it was decoupled from #7897?

@emmanuelsearch
Copy link
Member

@benemorius from what I know through other channels @gebart is no longer active on this PR.
So the PR could be "adopted" by someone else towards merging it into master.
If the issue is rare, but there are no deterministic test to reproduce it yet, we could indeed consider moving this into master and open a "known issue" ? Anyways: the PR needs a new shepherd. Are you willing to be this shepherd?

@benemorius
Copy link
Member

benemorius commented Jun 24, 2019

Yes if that's the case I'm very glad to do that. I'll open a new PR. Should I duplicate it as-is first or is this a good time to squash it? Rebasing is much easier if I can squash it first but I am unsure of the protocol for adopting a PR in progress. I may have been thinking of a different PR so either way is fine. Do you know @emmanuelsearch if #7897 is in the same inactive state?

Thanks very much @gebart for all your hard work.

@emmanuelsearch
Copy link
Member

@benemorius yes #7897 is in the same state so feel free to take over that PR too to shepherd it towards merging. I'd say:go ahead, squash and open 2 new PRs!

@benemorius
Copy link
Member

Sorry for the delay. I haven't dropped this. I did fix the IRQ wait bug and a few others and I've been testing it for a while and I'll clean up the result and open a new PR "very soon". In the mean time there is a usable version of this (currently with less bugfixes) in benemorius/RIOT/openlabs

@PeterKietzmann
Copy link
Member

An updated version is now in #12277. @gebart thanks for your initial work. You are still author and committer in #12277. I'm closing this PR now in favor of the updated one.

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: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first 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.