semtech-loramac: fix wrong behavior with RX windows #9618
Conversation
jia200x
changed the title
jia200x
added
Type: bug
There was a problem hiding this comment.
Changes make sense (the same changes are also applied in #8864). I have one coding convention comment, see below and static tests fail because of trailing whitespaces.
I tested this PR on iotlab (st-lrwan1-1 node with TTN backend) but sometimes RX fails with DR5. I changed the time shift from 50ms to 22ms here and with that it works more reliably with both DR0 and DR5.
|
|
||
| int32_t randr( int32_t min, int32_t max ) | ||
| { | ||
| - return ( int32_t )rand1( ) % ( max - min + 1 ) + min; | ||
| -} | ||
| + return (int32_t)random_uint32() % (max - min + 1) + min; | ||
| +}; | ||
| + return (int32_t) (random_uint32_range(0,max-min) + min); |
There was a problem hiding this comment.
put a space after the comma and around arithmetic operator:
(random_uint32_range(0, max - min) + min);
jia200x
force-pushed
the
pr/loramac_rx_window_fix
branch
from
d8e1539 to
669c50b
Compare
|
@jia200x please address comments, so we can merge this - or is this outdated? Then close. |
#8864 doesn't remove the timeout in the driver but only in the package adaption. I think I did all the cleanup initially but removed it when I saw this PR. I can reapplied the change in #8864 if you want. I also think that #8864 is more priority since it improves quite some things and is opened for a long time now (~ 6 months). |
|
@smlng I've still seen some issues regarded this. I prefer to keep it open |
|
and I prefer to get it merged 😉 especially if the issue is still present and this is (kind of) a fix ... which might need some adaption, due to #8864 being merged |
Do you plan to update this PR soon ? Some changes were applied in the last release but not all. Maybe it's worth having them merged ? |
Yes, I agree. I will try to do it during the next days |
jia200x
force-pushed
the
pr/loramac_rx_window_fix
branch
from
669c50b to
0232f14
Compare
|
I confirm the original issue is not present anymore in master. The "RX Timeout" is set as expected from the Semtech LoRaMAC pkg. Here's a picture of how the class A cycle look like on There are still some timing issues (related to #10545) which will be fixed as soon as that issue is solved. |
|
I will then close this PR ;) |
Contribution description
Sometimes when receiving ACK (confirmed messages) with the Semtech LoRaMAC pkg, the RX_TIMEOUT event is called twice (one from this timer and also from this interrupt).
The reason is because the timer is used as an escape route if the longest possible LoRaWAN packet is not received in the whole process (around 3 seconds in EU868), and not for RX window timeout. This is not intended by the original LoRaMAC implementation (as seen here) and sometimes produces a wrong behavior in the MAC internal states.
This PR fixes that issue.
Also, another bug appeared when using the proper RX Symbol Timeout mechanism. The(already fixed in #8864 )randrfunction from0003-adapt-utilities-functions.patchhad a wrong behavior due to int32_t casting and gave several numbers out of the given range, which caused unexpected behaviors in LoRaMAC. This is also addressed hereIssues/PRs references
None so far