-
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
examples/lorawan: add autonomous lorawan class A device example #8789
Conversation
6d93a79
to
9f95963
Compare
Side comment: I'm unsure if the pm management is used like it should, same for the RTC. Comments are very welcome on those specific parts. |
366977d
to
8367a4a
Compare
Thanks @aabadie! I will give it a closer look |
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 interesting ! But I don't see any call to pm_set(). You should put the device to sleep after the init I guess.
examples/lorawan/main.c
Outdated
time.tm_sec -= 60; | ||
} | ||
while (time.tm_min > 60) { | ||
time.tm_hour++; |
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.
You should also add a guard to ensure tm_hour doesn't reach 25 hours.
examples/lorawan/main.c
Outdated
{ | ||
#ifdef MODULE_PM_LAYERED | ||
/* Ensure the low-power mode is blocked during initialization phase */ | ||
pm_block(PM_MODE); |
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.
no call to pm_unblock after initialization.
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.
it is not needed because it's done at the end of the send function.
8367a4a
to
dfe5caf
Compare
This is the part I'm unsure. I thought that when all threads are blocked, the system falls in the idle thread that itself calls pm_set_lowest(). Normally, the |
Actually, you should not call any |
46eaae8
to
13373c9
Compare
3bba77c
to
0038928
Compare
semtech_loramac_channel_params_t params; | ||
params.frequency = freq; | ||
params.datarate = p.Datarate; | ||
semtech_loramac_set_rx2_params(mac, params); |
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.
Just wondering, isn't it a problem to call this function which will re-call mutex_lock(&mac->lock)
before we unlock it ?
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.
Yes it is. Good finding. I admit I didn't test this function. If you try semtech_loramac_set_rx2_freq
or semtech_loramac_set_rx2_dr
from the shell application, I think the command will just hang.
0038928
to
03d0847
Compare
@@ -60,7 +60,7 @@ void TimerSetValue(TimerEvent_t *obj, uint32_t value) | |||
xtimer_remove(&(obj->dev)); | |||
} | |||
|
|||
obj->timeout = (value - 50) * 1000; | |||
obj->timeout = (value - 20) * 1000; |
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.
BTW what sorcery is this number?
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.
This comes from the specs: the RX windows start RX_DELAY seconds after the TX with +/-20ms error, so anticipating the RX window with 20ms should ensure that a message from the gateway is not missed.
The initial port was not very accurate : the RX windows were starting is little too late - probably because of thread switching overheads - and then were missing the gateway message preamble, that's why I anticipated them a little more (hence the arbitrary 50ms value). But with this PR, it behaves as expected, and it works with 20ms.
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.
Could you add a little comment above this line to explain it please ? I don't think it is a good idea to have hardcoded number in the wild without any tips.
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.
Could you add a little comment above this line to explain it please ?
that's a good idea (and practice), I'll change that.
aa422c8
to
6a85c3d
Compare
I took a look at RIOT's PM and I think @vincent-d is right. You must not call any pm_* function here. The idle must select the best PM mode by itself. |
I know but the problem is that in this case it doesn't stop the MCU as I would expect. I'm testing with the b-l072z-lrwan1 board which is STM32L0 based. |
6a85c3d
to
c8b702e
Compare
c8b702e
to
563518b
Compare
I understand what you means but this is something that must be handle at driver and board level not at application level. There is still a lot of work that need to be done in order to have a working and wonderful PM management ;) |
I removed the use of |
examples/lorawan/main.c
Outdated
#include "semtech_loramac.h" | ||
|
||
/* Use the STOP mode to ensure memory retention between each send */ | ||
#define PM_MODE (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.
Not needed anymore
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.
Indeed, fixed !
Although this doesn't have to do with the general discussion here, @aabadie in my experience working with pm modes for L0 and L1, there's a lot of stuff you have to do with peripherals and gpio's to reach the datasheet power modes consumption. Whats is implemented right now for L0 isn't enough, the biggest part is switching unused GPIO's to AIN and the driver part @dylad mentioned (I had a PR on this a long time ago, you can see #8403 to have an idea of what I mean). |
+1 , I had to hack so much part of RIOT to get a sleep consumption around 20 uA with my SAML21 family. |
Thanks @dylad and @fjmolinas for the information. I'll look at #8403 more closely to see how the low power consumption can be reached with the L0 board I have. |
b933367
to
c9f6013
Compare
@dylad are still changes requested? |
@tcschmidt I don't think so, the current state looks good to me and it works. |
@dylad great. Then hit merge? |
This PR broke murdock ;-) https://ci.riot-os.org/RIOT-OS/RIOT/9390/0c9d7aeedce747885e83de340a177c04ce21e307/output.html#error0 |
This is why we don't merge with builds that are ages old.. |
See #9393 for fix. Guess someone owes the community a box of beer ;-) |
@miri64 you'll get free beer on Saturday! |
Sorry @miri64, I was not aware we need to re-triggers Murdock for this :( |
No problem. It was an easy fix after all ;-) |
@dylad we all live and learn - you'll get free beer at the RIOT summit. |
Contribution description
This PR adds a simple example of use of RIOT with LoRaWAN. The example shows several concepts:
I had to slightly change the loramac pkg code to better handle caller thread pid: in this application each send is done by a sender thread which is different from the main thread, used for the join procedure.
Issues/PRs references
Now based on #8798