pkg/semtech-loramac: refactor API and make it thread-safe by aabadie · Pull Request #8798 · RIOT-OS/RIOT

aabadie · 2018-03-19T08:00:06Z

Contribution description

This PR is a refactoring of the Semtech loramac package: the idea was initially to introduce a dedicated struct for handling the mac in a thread-safe way. The immediate consequence is a global API change of the package but thanks to this, any access to the internal mac can be protected using mutexes and data exchange is simplified between the upstream code and the RIOT adaption code (using ipc messages).
I also went a little further by refactoring other things:

remove useless static structures => saves a lot of memory
remove access from the thread handler function to static variables defined in the loramac upstream code
decouple the send from the receive => in the long run, it should be possible to adapt this package to the netif interface/thread
adapt the test application and the documentation

There might things I overlooked so comments and tests are very welcome.

cc @jia200x and @dylad who might be interested.

Issues/PRs references

None

jia200x · 2018-03-19T08:40:31Z

Hi @aabadie

Nice approach!
I only have some comments in the thread-safe part. I will do a quick review.

jia200x · 2018-03-19T08:56:13Z

tests/pkg_semtech-loramac/main.c

        if (strcmp("deveui", argv[2]) == 0) {
            uint8_t deveui[LORAMAC_DEVEUI_LEN];
-            semtech_loramac_get_deveui(deveui);
+            semtech_loramac_get_deveui(&loramac, deveui);


I would have preferred here the usage of IPC rather than direct calls + mutex. IMO:

If there's a single entry point for all commands, there shouldn't be needs for mutex (e.g here)

In can be integrated to netapi, since most of these commands are network options.

What do you think?

Basically, this means reusing MSG_TYPE_LORAMAC_CMD

I understand what you mean and I avoided it on purpose. That would have required other internal get/set functions, with switch/case depending on the parameters, very similar to netdev.

The actual approach is simpler and I think it ends up to the same result.

If the semtech-loramac API is later integrated into netif, then it will be possible to call it in the netapi. I need to rework my first attempt to achieve this.

I understand what you mean and I avoided it on purpose. That would have required other internal get/set functions, with switch/case depending on the parameters, very similar to netdev.

Yes, unless it's done via snippets (this is, writing integrated LoRaMAC code in a function and calling it via LORAMAC_CMD, but might confuse end users...)

Do you think it's possible to do the same with Join, etc?

jia200x · 2018-03-19T09:00:01Z

pkg/semtech-loramac/contrib/semtech_loramac.c

 kernel_pid_t semtech_loramac_pid;
-kernel_pid_t semtech_loramac_handler_pid;

+sx127x_t sx127x;


Do you think it could make sense to also isolate these kind of variables in a struct?

(e.g this could come from the netdev_t pointer)

The problem is that it is used by the radio adaption code as an extern variable. Using a get/set function for passing/retrieving the pointer as you initially did won't be better I think.
The best solution would be to pass it to all semtech-loramac function but then it will require massive patches in the upstream code.

yeah, you are right. I forgot that :)

dylad · 2018-03-25T16:03:29Z

pkg/semtech-loramac/contrib/semtech_loramac_getset.c

+    return datarate;
+}
+
+void semtech_loramac_set_rx2_params(semtech_loramac_t *mac,


I have a compilation error here because semtech_loramac_t *mac argument is not used.

dylad · 2018-03-25T17:16:44Z

pkg/semtech-loramac/contrib/semtech_loramac_getset.c

+    return datarate;
+}
+
+void semtech_loramac_set_rx2_params(semtech_loramac_channel_params_t params)


Can we declare it as static ? In the current state, this function is not threadsafe if user call it from its app.

Good idea, and it saves a few bytes (24) in ROM. Changed

dylad · 2018-03-26T09:53:29Z

I don't know if it is related to this PR but I'm encountering issue with the following command :

> loramac set tx_power 14

> loramac get tx_power

TX power index: 0
> loramac set tx_power 10

> loramac get tx_power

TX power index: 0

aabadie · 2018-03-26T10:47:29Z

I'm encountering issue with the following command

It is definitely a bug, and not only on this branch, I can reproduce it on master. ChannelsDefaultTxPower should be used to set the TX power.

aabadie · 2018-03-26T10:50:51Z

@dylad, see #8835

dylad

ACK
Tested work on custom hardware and a TTN gateway.
Join OTAA and data exchanges work as expected.
waiting for @jia200x's ACK before merging.

jia200x · 2018-03-26T11:26:45Z

@dylad unfortunately I don't have hardware to test. We bought a gateway by January and haven't received any news about it... codewise looks OK to me though

jia200x · 2018-04-09T12:19:53Z

@aabadie just tried it with a nucleo-l152 and SX1276, but I get:

Just confirmed this also happens with B-L072z-LRWAN1 (but this issue is not present in the master branch):

> loramac join abp
Join procedure succeeded!
> loramac tx asd
Failed: mac is busy

aabadie · 2018-04-09T12:23:09Z

Just confirmed this also happens with B-L072z-LRWAN1

Ok, with ABP join procedure. I know where it comes from and I'll fix it

jia200x · 2018-04-09T12:24:37Z

@aabadie it also happens in #8639

aabadie · 2018-04-09T15:41:44Z

it also happens in #8639

This is normal since it's based on this one ;)

jia200x · 2018-04-10T07:09:26Z

@aabadie the Mac busy msg vanished, but it hangs with ABP:

> loramac join abp
Join procedure succeeded!
> loramac tx asd
help
help
help
help
help
help
help
help

I will investigate

aabadie · 2018-04-10T07:20:45Z

Do you have the same problem on master ? (ABP activation is not my main use case, so I didn't test it)

aabadie · 2018-04-10T07:39:07Z

I will investigate

I think I know what is the problem: there's no fallback when the mcps_confirm event is not "status ok" and no message is sent from the MAC thread to the caller thread. I worked on that problem recently and have a branch that fixes this partially.

dylad · 2018-04-10T07:43:53Z

But I did not encounter this issue on my hardware, isn't it weird ?

aabadie · 2018-04-10T07:47:11Z

But I did not encounter this issue on my hardware, isn't it weird ?

More or less. That can happen if the server reply is not in the receiving time frame of the end device. In general it works.
It would be great if @jia200x could provide more information on his setup (iotlab node used, DR, etc) but also try the other activation method (OTAA).

dylad · 2018-04-19T09:33:20Z

Any news on this ?

jia200x · 2018-04-19T09:37:55Z

I will try to find some time between today and tomorrow

aabadie · 2018-04-19T10:02:20Z

@jia200x, I tested on iotlab and the problem you raised with ABP activation is also present in master. When using ABP activation, the TX confirmable mode doesn't work because no confirmation message is received by the end device. I know how to fix it (already worked on that in this branch)

aabadie · 2018-04-19T10:17:02Z

There's problem with receiving data when using ABP activation (this is also the case in master).

dylad · 2018-04-19T10:18:10Z

So now the question is : Should we fix it here ? or in another PR ?

aabadie · 2018-04-19T10:33:11Z

Should we fix it here ? or in another PR ?

I prefer in another PR: see #8982 (and mainly 99af4f4)

dylad · 2018-04-19T11:06:06Z

prefer in another PR: see #8982 (and mainly 99af4f4)

Great, I'll give it a try this afternoon if possible.
@jia200x should we merge this one ?

jia200x · 2018-04-19T11:12:34Z

@dylad I'm OK with merging this one since the bug is not related to this PR.

jia200x

ACK

jia200x · 2018-03-19T09:01:32Z

pkg/semtech-loramac/contrib/semtech_loramac.c

-
-static uint8_t _semtech_loramac_radio_payload[SX127X_RX_BUFFER_SIZE];
-static semtech_loramac_rx_data_t _semtech_loramac_rx_data;
+LoRaMacPrimitives_t semtech_loramac_primitives;


Same question as above here

jia200x · 2018-03-19T09:47:30Z

pkg/semtech-loramac/contrib/semtech_loramac.c

 kernel_pid_t semtech_loramac_pid;
-kernel_pid_t semtech_loramac_handler_pid;

+sx127x_t sx127x;


yeah, you are right. I forgot that :)

jia200x · 2018-03-19T09:56:48Z

tests/pkg_semtech-loramac/main.c

        if (strcmp("deveui", argv[2]) == 0) {
            uint8_t deveui[LORAMAC_DEVEUI_LEN];
-            semtech_loramac_get_deveui(deveui);
+            semtech_loramac_get_deveui(&loramac, deveui);


I understand what you mean and I avoided it on purpose. That would have required other internal get/set functions, with switch/case depending on the parameters, very similar to netdev.

Yes, unless it's done via snippets (this is, writing integrated LoRaMAC code in a function and calling it via LORAMAC_CMD, but might confuse end users...)

Do you think it's possible to do the same with Join, etc?

aabadie · 2018-04-19T11:21:10Z

Do you think it's possible to do the same with Join, etc?

I don't think so. Because the join/tx are triggering a complex workflow in the MAC with timers etc. Using the loramac_cmd and IPC messages makes more sense in this case.
Setting/Getting MAC parameters is just writing in a variables in the loramac-node code, that's why I think mutexes are more handy.

jia200x · 2018-04-19T11:22:48Z

I don't think so. Because the join/tx are triggering a complex workflow in the MAC with timers etc. Using the loramac_cmd and IPC messages makes more sense in this case.
Setting/Getting MAC parameters is just writing in a variables in the loramac-node code, that's why I think mutexes are more handy.

Sounds reasonable.

Ok, then GO.

pkg/semtech-loramac: refactor API and make it thread-safe

aabadie requested a review from jia200x March 19, 2018 08:00

This was referenced Mar 19, 2018

examples/lorawan: add autonomous lorawan class A device example #8789

Merged

pkg/semtech-loramac: add link check support #8639

Merged

aabadie force-pushed the hackathon_ietf branch 3 times, most recently from 4488dd0 to a82b048 Compare March 19, 2018 08:28

jia200x requested changes Mar 19, 2018

View reviewed changes

jia200x reviewed Mar 19, 2018

View reviewed changes

aabadie force-pushed the hackathon_ietf branch from a82b048 to 7d8b0fe Compare March 19, 2018 19:33

aabadie mentioned this pull request Mar 22, 2018

LoraMAC-node does not work #8813

Closed

aabadie force-pushed the hackathon_ietf branch 2 times, most recently from 9dda9fd to 27ad634 Compare March 24, 2018 21:24

dylad self-assigned this Mar 25, 2018

dylad reviewed Mar 25, 2018

View reviewed changes

aabadie force-pushed the hackathon_ietf branch from 27ad634 to f702aa5 Compare March 25, 2018 16:21

dylad reviewed Mar 25, 2018

View reviewed changes

aabadie force-pushed the hackathon_ietf branch 2 times, most recently from 4669665 to c53ece8 Compare March 26, 2018 05:35

aabadie mentioned this pull request Mar 26, 2018

pkg/semtech-loramac: fix tx_power get/set functions #8835

Closed

dylad approved these changes Mar 26, 2018

View reviewed changes

aabadie force-pushed the hackathon_ietf branch from c1abaa3 to f0b71c3 Compare April 9, 2018 15:43

aabadie added 3 commits April 19, 2018 11:44

pkg/semtech-loramac: refactor API to make it thread-safe

5b76fdf

tests/pkg_semtech-loramac: apply new API

378ec10

pkg/semtech-loramac: update documentation

f4efb5b

aabadie force-pushed the hackathon_ietf branch from f0b71c3 to f4efb5b Compare April 19, 2018 09:45

aabadie mentioned this pull request Apr 19, 2018

pkg/semtech_loramac: handle non OK mcps confirm messages #8982

Merged

jia200x approved these changes Apr 19, 2018

View reviewed changes

jia200x merged commit b8ac8c9 into RIOT-OS:master Apr 19, 2018

maxvankessel pushed a commit to maxvankessel/RIOT that referenced this pull request Apr 20, 2018

Merge pull request RIOT-OS#8798 from aabadie/hackathon_ietf

85ed8ab

pkg/semtech-loramac: refactor API and make it thread-safe

aabadie deleted the hackathon_ietf branch June 1, 2018 18:55

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

aabadie commented Mar 19, 2018

Contribution description

Issues/PRs references

Uh oh!

jia200x commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylad commented Mar 26, 2018

Uh oh!

aabadie commented Mar 26, 2018

Uh oh!

aabadie commented Mar 26, 2018

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Mar 26, 2018

Uh oh!

jia200x commented Apr 9, 2018

Uh oh!

aabadie commented Apr 9, 2018

Uh oh!

jia200x commented Apr 9, 2018

Uh oh!

aabadie commented Apr 9, 2018

Uh oh!

jia200x commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Apr 10, 2018

Uh oh!

aabadie commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylad commented Apr 10, 2018

Uh oh!

aabadie commented Apr 10, 2018

Uh oh!

dylad commented Apr 19, 2018

Uh oh!

jia200x commented Apr 19, 2018

Uh oh!

aabadie commented Apr 19, 2018

Uh oh!

aabadie commented Apr 19, 2018

Uh oh!

dylad commented Apr 19, 2018

Uh oh!

aabadie commented Apr 19, 2018

Uh oh!

dylad commented Apr 19, 2018

Uh oh!

jia200x commented Apr 19, 2018

Uh oh!

aabadie Mar 19, 2018 •

edited

Loading

jia200x commented Apr 10, 2018 •

edited

Loading

aabadie commented Apr 10, 2018 •

edited

Loading