-
Notifications
You must be signed in to change notification settings - Fork 2k
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/nrf24l01p: Netdev driver for nrf24l01p #13743
drivers/nrf24l01p: Netdev driver for nrf24l01p #13743
Conversation
What is the difference between this and #12681? |
#12681 :
This resulted in a lot of ugly #ifdefs in the code. |
Maybe it is better not rename the old driver, but to give the new driver a new distinct name? Github seems to be greatly confused by the rename. (When looking at the individual commits, the rename is correctly detected. But when looking the overall changes of this PR, it looks like Or maybe we could have the rename split out as individual PR? That should be something that requires zero code review, so that a decision on that could be reached swiftly. And then this PR could be rebased on |
If this driver should not be named |
|
Ok, I could use this. |
Note that for the GNRC stuff you could still go with |
0b5768b
to
e318375
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally received my modules and can do some testing now!
After flashing examples/gnrc_networking
I noticed some failed asserts - you'll need to add NETDEV_TYPE_NRF24L01P_NG
to sys/net/link_layer/l2util/l2util.c
.
You probably want to add it in some other places too - I'd say searching for NETDEV_TYPE_CC110X
is a good guide as that's a similar transceiver.
This does two things: The documentation of `luid_get()` is wrong, or at least confusing. It talks about > an 8-bit incrementing counter value into the most significant byte while the implementation does ((uint8_t *)buf)[len - 1] ^= lastused++; Now it could be argued that the intention was that the ID is supposed to be used in Big Endian contexts and that was an omission, however to keep everyone's sanity, let's keep it simple and ust state that this actually changes the LSB. Also add a `luid_get_be()` function that does the same, but modifies the most significant bit - or the least significant one if interpreted as Big Endian. This can then be used directly by e.g. RIOT-OS#13743
This does two things: The documentation of `luid_get()` is wrong, or at least confusing. It talks about > an 8-bit incrementing counter value into the most significant byte while the implementation does ((uint8_t *)buf)[0] ^= lastused++; // 0 is LSB! Now it could be argued that the intention was that the ID is supposed to be used in Big Endian contexts and that was an omission, however to keep everyone's sanity, let's keep it simple and ust state that this actually changes the LSB. Also add a `luid_get_be()` function that does the same, but modifies the most significant bit - or the least significant one if interpreted as Big Endian. This can then be used directly by e.g. RIOT-OS#13743
This does two things: The documentation of `luid_get()` is wrong, or at least confusing. It talks about > an 8-bit incrementing counter value into the most significant byte while the implementation does ((uint8_t *)buf)[0] ^= lastused++; // 0 is LSB! Now it could be argued that the intention was that the ID is supposed to be used in Big Endian contexts and that was an omission, however to keep everyone's sanity, let's keep it simple and just state that this actually changes the LSB. Also add a `luid_get_be()` function that does the same, but modifies the most significant byte - or the least significant one if interpreted as Big Endian. This can then be used directly by e.g. RIOT-OS#13743
I flashed the driver on two nodes and they do get different addresses now, however they can not communicate: same54-xpro
WeAct-f411ce
(Not sure why stdio looks different here, this is over CDC ACM) Can't receive anything with |
They cannot communicate over ipv6. This is the major difference between this PR and the previous one.
Thats weird ...
|
Yes. Thank you @benpicco that you looked at the driver again. |
Right now it is like this:
pipe 1:
pipe 2 - 5:
I am going to change pipe 0 and pipe 1 like this:
pipe 1:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good!
I noticed a problem with large fragmented packets though
2020-12-20 01:41:38,595 # ping fe80::a07f:b6ff:fe84:b47f -s 250
2020-12-20 01:41:38,652 # 258 bytes from fe80::a07f:b6ff:fe84:b47f%7: icmp_seq=0 ttl=64 time=54.932 ms
2020-12-20 01:41:39,709 # 258 bytes from fe80::a07f:b6ff:fe84:b47f%7: icmp_seq=1 ttl=64 time=112.845 ms
2020-12-20 01:41:40,661 # 258 bytes from fe80::a07f:b6ff:fe84:b47f%7: icmp_seq=2 ttl=64 time=64.198 ms
2020-12-20 01:41:40,661 #
2020-12-20 01:41:40,661 # --- fe80::a07f:b6ff:fe84:b47f PING statistics ---
2020-12-20 01:41:40,662 # 3 packets transmitted, 3 packets received, 0% packet loss
2020-12-20 01:41:40,663 # round-trip min/avg/max = 54.932/77.325/112.845 ms
2020-12-20 01:41:42,604 # ping fe80::a07f:b6ff:fe84:b47f -s 300
2020-12-20 01:41:42,606 # gnrc_netif: possibly lost interrupt.
2020-12-20 01:41:44,606 # gnrc_netif: possibly lost interrupt.
2020-12-20 01:41:45,604 #
2020-12-20 01:41:45,605 # --- fe80::a07f:b6ff:fe84:b47f PING statistics ---
2020-12-20 01:41:45,607 # 3 packets transmitted, 0 packets received, 100% packet loss
my suspicion is that this has to do with that busy send loop.
I tried returning -EBUSY
instead of -EGAIN
and not releasing the pkt
on failure so gnrc_netif_pktq
can re-send it.
However, after that fragmented packets wouldn't work at all anymore.
My suspicion is that this might have to do with the added headers in _nrf24l01p_ng_adpt_send()
.
😕 At least it´s reproducible for me this time. I did some trial and error and noticed, if I increase the message queue in ping6
This appears to be natural to me because: May this be the simple solution? |
Sound very plausible to me. We discussed this is a few times recently in the context of the IEEE 802.15.4 Radio HAL. If I recall correctly, there was some consensus that this ideally should be managed by preventing the upper layers to send UDP datagrams at a faster rate than the driver and lower stacks can transmit the corresponding frames. That way, the packet queue would never overflow. Additionally, there seems to me also be a consensus that the message queue is not a good fit for handling IRQ events. There already is code in master that mutliplexes an But IMO this is not a good solution, as simply multiplexing |
It looks like
But IIRC there is noone yet to translate this into code 😉 |
I have some work lying around based upon #14660 that goes a step in that direction. But only with the API change of #14660, we would have a clearly defined point at which the driver as completed a transmission. Currently, e.g. the However, my code would only make sure that the |
Well, for drivers that do block themselves (like e.g. |
Fragmented packets now work with and without What's pretty cool is how fast the round-trip is with this driver:
|
Please squash (& rebase) |
9f2c002
to
5b93427
Compare
Looks like |
5b93427
to
e83d2a3
Compare
I have renamed: |
Ok, looks like I am not allowed to use binary integers. |
The driver uses the netdev interface. Due to the limited capabilities of the transceiver (32 byte FIFO and no source address in the layer2 frame), it relies on 6LowPAN compression and adds the source address to the frame for that.
e83d2a3
to
7d618af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and still works - ACK!
btw.: what would be needed to adapt this for nrf24l01 (non-plus)?
I now have a bunch of these Chips as well and would like to try that out - from a previous iteration I remember that the 32 byte fifo always needs to be filled with these - is there anything else?
Another interesting bit is that the radio of the nrf518xx/nrf528xx series claims compatibility with nrf24l01p - do you know if this is related to nrfmin?
The adaptations for nrf24l01 don´t seem to be so complicated. The main drawback IMO is the static payload size that must be configured equally for sender and receiver. There is a section in the nrf24l01+ datasheet (Appendix B) that tells how to set up compatibility. There they say that you also must disable auto-ACK. I have no experience with nrf518xx/nrf528xx. |
🎉 |
Hurray 😃 🎊 |
This does two things: The documentation of `luid_get()` is wrong, or at least confusing. It talks about > an 8-bit incrementing counter value into the most significant byte while the implementation does ((uint8_t *)buf)[0] ^= lastused++; // 0 is LSB! Now it could be argued that the intention was that the ID is supposed to be used in Big Endian contexts and that was an omission, however to keep everyone's sanity, let's keep it simple and just state that this actually changes the LSB. Also add a `luid_get_lb()` function that does the same, but modifies the most significant byte - or the last byte if looking at the index. This can then be used directly by e.g. RIOT-OS#13743
Contribution description
This PR is a shorter version of #12681, which aimed to contribute a
netdev
driver for thenrf24l01+
but was too large to be reviewed.The old driver for thenrf24l01+
is simply renamed tonrf24l01p_lowlevel
.With this driver you can only use
Enhanced ShockBurst
as a protocol.There is no custom header that includes the transceiver´s source address.
Testing procedure
Try
txtsnd
:Issues/PRs references
#1808
#2021
#12681