pkg/semtech-loramac: rework interaction with the MAC #11541

aabadie · 2019-05-17T10:08:47Z

Contribution description

This is another attempt to fix #11530.
It follows the suggestion of @ParksProjets in #11535 (comment):

TX status is now only reported after an mcps confirm event
RX data are notified from mcps indication events
one needs an external thread to receive data from the MAC. This is much simpler but it consume a little more RAM if one wants to manage downlinks message outside of the MAC. The bonus point with this approach is the ability to correctly support class C devices.

I have tested this PR using OTAA, ABP activations and with CONFIRMED and UNCONFIRMED messages.

Everything is working as expected.

The only small thing is the link check which is not working correctly but it's already the case in master I think.

Finally, I also had to update the examples/lorawan application and doxygen documentation to avoid potential lock of autonomous application. Maybe examples/lorawan could be later refactored with downling messages better handled or adapted for class C devices.

Testing procedure

Run tests/pkg_semtech-loramac, send messages from the device, send messages from the network using CONFIRMED/UNCONFIRMED (cnf/uncnf option of loramac tx: everything should work, with this PR, the application doesn't block.

Issues/PRs references

fixes #11530, closes #11535 (cleaner alternative)

fjmolinas

Some initial comments.

fjmolinas · 2019-05-23T15:10:18Z

pkg/semtech-loramac/contrib/semtech_loramac.c

@@ -839,7 +837,7 @@ uint8_t semtech_loramac_send(semtech_loramac_t *mac, uint8_t *data, uint8_t len)
        return SEMTECH_LORAMAC_NOT_JOINED;
    }

-    /* Correctly set the caller pid */
+    /* Correctly set the sender thread pid */


I think the comment should be inline with the variable names.

fjmolinas · 2019-05-23T15:14:07Z

pkg/semtech-loramac/include/semtech_loramac.h

@@ -110,6 +110,7 @@ typedef struct {
 typedef struct {
    mutex_t lock;                                /**< loramac access lock */
    uint8_t caller_pid;                          /**< pid of caller thread */


Maybe it makes more sense if this is tx_pid if I understand right this thread is in charge of join procedure and sending packets, so tx. Would be more inline with rx_pid (receiver thread)

tests/pkg_semtech-loramac/main.c

fjmolinas · 2019-05-23T15:17:40Z

pkg/semtech-loramac/doc.txt

@@ -95,12 +95,6 @@
 *         return 1;
 *     }
 *
- *     /* 5. wait for any potentially received data */


Document starting and usage of a receiving thread.

fjmolinas · 2019-05-23T15:59:15Z

Tested it does work and no deadlock, I confirm it fixes #11530. @ParksProjets can you confirm it fixes the issue on your side?

aabadie · 2019-05-24T07:32:31Z

@fjmolinas, I found a problem with the link_check feature. The current refactoring is breaking it. I know how to fix it simply but now that I'm thinking of it, I'd like to try a deeper refactoring of the interaction between the MAC events (MCPS confirm/indication, MLME confirm/indication) and the event loop. I hope this will simplify a bit more all this mess.

aabadie · 2019-05-24T09:08:49Z

@fjmolinas, I've reworked this the PR the way we discussed IRL and now everything is working (including link_check info and received ACKs).

fjmolinas

I tested that your changes regarding link_check and ack are working. A minor comment on a border case.

fjmolinas · 2019-05-28T15:10:56Z

pkg/semtech-loramac/doc.txt

 * }
 * ```
+ * - Finally, this thread can be started after the join procedure


Is this a requirement?

fjmolinas · 2019-05-28T16:01:31Z

pkg/semtech-loramac/contrib/semtech_loramac.c

+                        _semtech_loramac_send(mac, NULL, 0);
+                        mac->port = prev_port;
+                    }
+                    else if (indication->RxData) {


There is a border case where if a downlink is received when a cnf messaged is sent the indication-AckReceived doesn't go up to the application. Although this doesn't matter in class A, in class B or C this could be mixed. If using if instead of elseif this would work

fjmolinas

@aabadie Everyhting is looking good. I see all my comments have been addressed, can you squas?

tests/pkg_semtech-loramac/main.c

aabadie · 2019-05-29T07:14:04Z

I see all my comments have been addressed, can you squash?

Done!

aabadie · 2019-05-29T07:15:01Z

Note that this PR will conflicts with #8855 once merged but I prefer this one being merged first.

fjmolinas · 2019-05-29T07:24:38Z

@aabadie somehow before the squash I oversaw that waspmote doesn't fit anymore. I still think this is a good change but reducing supported boards doesn't seem great. Anyway for reducing stack-sizes for constrained boards? Or removing features from the test for constrained boards?

fjmolinas · 2019-05-29T07:28:29Z

Note that this PR will conflicts with #8855 once merged but I prefer this one being merged first.

Noted!

aabadie · 2019-05-29T07:38:23Z

Anyway for reducing stack-sizes for constrained boards? Or removing features from the test for constrained boards?

I think I already tried both:

reducing stack size was somehow crashing sometimes
removing features: I tried to make link_check optional but it was not enough.

For me it's a problem of missing RAM, and waspmote was already very short. Adding a second thread is too much for it.

aabadie · 2019-05-29T07:38:50Z

What about simply disabling RX thread ?

fjmolinas · 2019-05-29T07:42:08Z

What about simply disabling RX thread ?

I think that would be an acceptable compromise. The previous support had a bug so if fixing it adds some memory usage then I think it's okay to drop some feature support for waspmote, which was already buggy in master. :)

aabadie · 2019-05-29T08:31:55Z

@fjmolinas, I made all RX related code optional using a pseudo-module semtech_loramac_rx. Link check is also excluded because it relies on RX.

fjmolinas · 2019-05-29T14:21:10Z

@aabadie everything is good! This commit need to be discarded though tests/pkg_semtech-loramac: waspmote-pro doesn't fit anymore. Once that is done, we are all good!

Now TX notification messages are only sent after mcps confirm event. RX message notification message is sent after mcps indication

This can highly happen when no message is received from the network after a send

aabadie · 2019-05-29T14:26:30Z

This commit need to be discarded though

Done!

fjmolinas

ACK! Lets wait for murdock ot be green!

fjmolinas · 2019-05-29T14:43:24Z

Go!

aabadie · 2019-05-29T14:44:18Z

Great! Thanks a lot for reviewing @fjmolinas :)
Now I have to rebase the other ones ;)

- with RIOT-OS#11541 TX notification are only sent after mcps confirm event this will send a SEMTECH_LORAMAC_TX_DONE instaed of SEMTECH_LORAMAC_TX_OK.

- with RIOT-OS#11541 TX notification are only sent after mcps confirm event this will send a SEMTECH_LORAMAC_TX_DONE instead of SEMTECH_LORAMAC_TX_OK.

- with RIOT-OS#11541 TX notification are only sent after mcps confirm event this will send a SEMTECH_LORAMAC_TX_DONE instead of SEMTECH_LORAMAC_TX_OK. (cherry picked from commit cdf687c)

- with RIOT-OS#11541 TX notification are only sent after mcps confirm event this will send a SEMTECH_LORAMAC_TX_DONE instead of SEMTECH_LORAMAC_TX_OK.

aabadie added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: LoRa Area: LoRa radio support labels May 17, 2019

aabadie requested a review from jia200x May 17, 2019 10:08

aabadie mentioned this pull request May 17, 2019

pkg/semtech-loramac: fix deadlock when sending unconfirmed messages #11535

Closed

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch from 5344c0e to f6290f3 Compare May 20, 2019 13:07

aabadie mentioned this pull request May 20, 2019

pkg/semtech-loramac: provide basic persistence for MAC state #11552

Merged

aabadie requested a review from fjmolinas May 23, 2019 13:55

aabadie mentioned this pull request May 23, 2019

examples/lorawan: add the possibility to use ABP activation procedure #11237

Merged

fjmolinas requested changes May 23, 2019

View reviewed changes

aabadie changed the title ~~pkg/semtech-loramac: rework TX/RX interaction with the MAC~~ pkg/semtech-loramac: rework interaction with the MAC May 24, 2019

fjmolinas reviewed May 28, 2019

View reviewed changes

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch from 90d8087 to 79fca24 Compare May 28, 2019 16:13

fjmolinas mentioned this pull request May 28, 2019

sys/shell: add loramac shell command #8855

Merged

fjmolinas reviewed May 29, 2019

View reviewed changes

tests/pkg_semtech-loramac/main.c Outdated Show resolved Hide resolved

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch from 79fca24 to d7737a5 Compare May 29, 2019 07:08

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch 4 times, most recently from 79d5fac to 8003567 Compare May 29, 2019 08:28

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch from 8003567 to c721efe Compare May 29, 2019 08:29

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch from c721efe to 9da50dd Compare May 29, 2019 08:40

aabadie added 8 commits May 29, 2019 16:24

pkg/semtech-loramac: reduce MAC thread message queue

d179837

pkg/semtech-loramac: refactor send and recv interactions with the MAC

b7890b3

Now TX notification messages are only sent after mcps confirm event. RX message notification message is sent after mcps indication

tests/pkg_semtech-loramac: add RX thread for downlink messages

0838cf9

pkg-semtech-loramac: update/simplify doxygen documentation

032f891

examples/lorawan: remove recv call to avoid endless lock

f2c63c8

This can highly happen when no message is received from the network after a send

pkg/semtech-loramac: make RX optional

b5c48ef

tests/pkg_semtech-loramac: conditionally build RX related code

14a4db1

tests/pkg_semtech-loramac: use semtech_loramac_rx with supported boards

56fb257

aabadie force-pushed the pr/pkg/semtech-loramac-rx-thread branch from 9da50dd to 56fb257 Compare May 29, 2019 14:25

fjmolinas approved these changes May 29, 2019

View reviewed changes

fjmolinas merged commit ba84ad4 into RIOT-OS:master May 29, 2019

aabadie deleted the pr/pkg/semtech-loramac-rx-thread branch May 30, 2019 16:40

kYc0o mentioned this pull request Jun 5, 2019

tests/pkg_semtech-loramac: hardfault on lobaro-lorabox #11626

Closed

fjmolinas mentioned this pull request Jul 19, 2019

examples/lorawan: fix semtech_loramac_send TX ret code #11869

Merged

fjmolinas mentioned this pull request Jul 19, 2019

examples/lorawan: fix semtech_loramac_send TX ret code [backport 2019.07] #11873

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/semtech-loramac: rework interaction with the MAC #11541

pkg/semtech-loramac: rework interaction with the MAC #11541

aabadie commented May 17, 2019 •

edited

Loading

fjmolinas left a comment

fjmolinas May 23, 2019

fjmolinas May 23, 2019

fjmolinas May 23, 2019

aabadie May 23, 2019

fjmolinas commented May 23, 2019

aabadie commented May 24, 2019

aabadie commented May 24, 2019

fjmolinas left a comment

fjmolinas May 28, 2019

fjmolinas May 28, 2019

fjmolinas left a comment

aabadie commented May 29, 2019

aabadie commented May 29, 2019

fjmolinas commented May 29, 2019

fjmolinas commented May 29, 2019

aabadie commented May 29, 2019

aabadie commented May 29, 2019

fjmolinas commented May 29, 2019

aabadie commented May 29, 2019

fjmolinas commented May 29, 2019 •

edited

Loading

aabadie commented May 29, 2019

fjmolinas left a comment

fjmolinas commented May 29, 2019

aabadie commented May 29, 2019

pkg/semtech-loramac: rework interaction with the MAC #11541

pkg/semtech-loramac: rework interaction with the MAC #11541

Conversation

aabadie commented May 17, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas May 23, 2019

Choose a reason for hiding this comment

fjmolinas May 23, 2019

Choose a reason for hiding this comment

fjmolinas May 23, 2019

Choose a reason for hiding this comment

aabadie May 23, 2019

Choose a reason for hiding this comment

fjmolinas commented May 23, 2019

aabadie commented May 24, 2019

aabadie commented May 24, 2019

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas May 28, 2019

Choose a reason for hiding this comment

fjmolinas May 28, 2019

Choose a reason for hiding this comment

fjmolinas left a comment

Choose a reason for hiding this comment

aabadie commented May 29, 2019

aabadie commented May 29, 2019

fjmolinas commented May 29, 2019

fjmolinas commented May 29, 2019

aabadie commented May 29, 2019

aabadie commented May 29, 2019

fjmolinas commented May 29, 2019

aabadie commented May 29, 2019

fjmolinas commented May 29, 2019 • edited Loading

aabadie commented May 29, 2019

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas commented May 29, 2019

aabadie commented May 29, 2019

aabadie commented May 17, 2019 •

edited

Loading

fjmolinas commented May 29, 2019 •

edited

Loading