Skip to content
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/sx127x: pkg/semtech-loramac: replace xtimer by ztimer #14547

Merged
merged 5 commits into from Aug 20, 2020

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Jul 17, 2020

Contribution description

This PR replaces the xtimer used by the sx127x radio driver and package semtech-loramac by the new ztimer and especially its rtt based backend version.
This should allow lower power consumption, even between the TX and RX windows phases, but it will also allow the use of STOP mode (with RAM retention) on STM32 without the need to disable the duty-cycle check. I tested this particular point with #11237 and this PR and now it works.

Note that to be able to use the RTT backend one cannot rely on usec precision timers. msec precision delays cannot be used with radio reset delays but loramac uses ms precision delays all the time, so it can be used there. As a result, all timeouts that were provided in microseconds are now converted to milliseconds.

Testing procedure

  • verify that examples/lorawan and pkg/semtech_loramac are still functional

Issues/PRs references

Will be usefull for #11237

@aabadie aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support labels Jul 17, 2020
@aabadie aabadie requested a review from jia200x as a code owner July 17, 2020 15:00
@aabadie aabadie requested a review from fjmolinas July 17, 2020 15:02
@aabadie aabadie added this to Backlog / Proposals in RIOT Sprint Day via automation Jul 24, 2020
@fjmolinas
Copy link
Contributor

Would be nice if you start posting your test results ouput in the PR description.

@fjmolinas
Copy link
Contributor

I'll run the release tests for both applications :)

@fjmolinas fjmolinas self-assigned this Aug 11, 2020
@fjmolinas fjmolinas moved this from Backlog / Proposals to Under Review in RIOT Sprint Day Aug 11, 2020
@fjmolinas
Copy link
Contributor

All good with tests/pkg_semtech_loramac:

IOTLAB_NODE=st-lrwan1-10.saclay.iot-lab.info make -C tests/pkg_semtech-loramac/ flash test -j3
main(): This is RIOT! (Version: 2020.10-devel-474-g5b8be-pr-14547)
All up, running the shell now
> loramac get devaddr
loramac get devaddr
DEVADDR: 2601107E
> loramac get nwkskey
loramac get nwkskey
NWKSKEY: 6CCC73B2681EC60142C5D4933BFE8D34
> loramac get appskey
loramac get appskey
APPSKEY: 07683C44D66D201B557B0DF7EFEF8391
> TEST PASSED

@fjmolinas
Copy link
Contributor

examples/lorawan is not working for me:

main(): This is RIOT! (Version: 2020.10-devel-669-g2fb7d-HEAD)
LoRaWAN Class A low-power application
=====================================
Starting join procedure
Join procedure succeeded
Sending: This is RIOT!
# Nothing happens here ...

@fjmolinas
Copy link
Contributor

examples/lorawan is not working for me:

Hmm seems to be an issue with iotlab boards, works with locally connected boards:

main(): This is RIOT! (Version: 2020.10-devel-474-g5b8be-pr-14547)
LoRaWAN Class A low-power application
=====================================
Starting join procedure
Join procedure succeeded
Sending: This is RIOT!
Sending: This is RIOT!
Sending: This is RIOT!
Sending: This is RIOT!
Sending: This is RIOT!
TESST PASSED

@fjmolinas
Copy link
Contributor

@aabadie changes make sense and are working, once you take a look at my ztimer comment I'll ACK.

@fjmolinas
Copy link
Contributor

BTW works with st-lrwan1-19.saclay.iot-lab.info but not with st-lrwan1-10.saclay.iot-lab.info'/st-lrwan1-11.saclay.iot-lab.info'at least.

RIOT Sprint Day automation moved this from Under Review to Waiting For Murdock Aug 11, 2020
fjmolinas
fjmolinas previously approved these changes Aug 11, 2020
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 11, 2020
@fjmolinas
Copy link
Contributor

@aabadie murdock spotted many conflicts, can you take a look?

@aabadie
Copy link
Contributor Author

aabadie commented Aug 12, 2020

murdock spotted many conflicts, can you take a look?

There were several issues:

  • rtc and rtt features are conflicting on efm32 so I blacklisted these platform for examples/lorawan (since it uses both) => 61ab47e
  • the xtimer defines for lobaro-lorabox were in the wrong file, this is fixed by 1579614
  • for cpu with timer clocked at 250kHZ (Atmega), there was an issue in the ztimer auto_init code. I fixed it in 7c3e14e but I'm not 100% sure of the fix.

I can provide the last 2 commits in separate PRs to ease review if you want.

sys/ztimer/auto_init.c Outdated Show resolved Hide resolved
sys/ztimer/auto_init.c Outdated Show resolved Hide resolved
@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 12, 2020
@@ -6,6 +6,9 @@ USEMODULE += semtech_loramac_mac_region
USEMODULE += semtech_loramac_crypto
USEMODULE += semtech_loramac_arch

USEMODULE += ztimer_msec
USEMODULE += ztimer_periph_rtt
Copy link
Contributor

@fjmolinas fjmolinas Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
USEMODULE += ztimer_periph_rtt
# If RTT feature is available use the RTT backend of ztimer
FEATURES_OPTIONAL += periph_rtt
ifneq (,$(filter periph_rtt,$(FEATURES_USED)))
USEMODULE += ztimer_periph_rtt
endif

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer keeping the actual state for the loramac pkg: it's important to ensure that the rtt is used because of the duty-cycle timer. In a deep sleep mode with RAM retention, a regular timer will be stopped but not a rtt based timer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why enforce it? Low Power modes are not enabled by default, only examples/lorawan does that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ztimer is an internal implementation detail of the package adaption to RIOT. There's no need to enforce developers to add this dependency in their lorawan applications.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is a question of keeping flexibility or having things that work out of the box. I think you want the later, that is OK for me since you are maintaining this. Can you then add some documentation to the pkg on the implementation status, I think it would be good to say why rtt is required or when it is required. And in the same sense, what is needed for the pkg to work, what is needed for the pkg to work with sleep modes, etc... For me this is not very clear without having a knowledge of the code/implementation internals.

@fjmolinas
Copy link
Contributor

@aabadie The fact that rtc and rtt are required seems very stm32 specific since with stm32 only rtc can wake up the cpu. It seems like a shame to blacklist architectures because of this.

@fjmolinas fjmolinas dismissed their stale review August 13, 2020 07:25

Don't agree with latest changes.

RIOT Sprint Day automation moved this from Waiting For Murdock to Under Review Aug 13, 2020
@aabadie
Copy link
Contributor Author

aabadie commented Aug 13, 2020

with stm32 only rtc can wake up the cpu

only in standby mode. In stop mode, rtt can wakeup the cpu.

It's not stm32 specific, it's just that examples/lorawan is using the rtc as a periodic alarm to send to next message. It could be done differently of course.
But I prefer using the RTC alarm so there's support for standby mode (or any mode without RAM retention) with lorawan. This is an important use case.

@fjmolinas
Copy link
Contributor

It's not stm32 specific, it's just that examples/lorawan is using the rtc as a periodic alarm to send to next message. It could be done differently of course.
But I prefer using the RTC alarm so there's support for standby mode (or any mode without RAM retention) with lorawan. This is an important use case.

Ok I guess this is just an examples, and tests/pkg_semtech_loramac is more flexible.

@aabadie aabadie force-pushed the pr/lora/ztimer branch 2 times, most recently from b5efadd to 79eea38 Compare August 20, 2020 09:48
@fjmolinas
Copy link
Contributor

@aabadie only static tests did not pass, only #14547 (comment) has not yet been addressed

@aabadie
Copy link
Contributor Author

aabadie commented Aug 20, 2020

squashed (to fix static tests) and added documentation to address #14547 (comment)

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, lets wait for murdock

RIOT Sprint Day automation moved this from Under Review to Waiting For Murdock Aug 20, 2020
@aabadie aabadie merged commit 73b3d2a into RIOT-OS:master Aug 20, 2020
RIOT Sprint Day automation moved this from Waiting For Murdock to Done Aug 20, 2020
@aabadie aabadie deleted the pr/lora/ztimer branch August 20, 2020 13:14
@aabadie
Copy link
Contributor Author

aabadie commented Aug 20, 2020

Thanks for reviewing @fjmolinas !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants