Skip to content

drivers/periph/rtt: enhancements to make RTT_FREQUENCY easily configurable#13874

Closed
fjmolinas wants to merge 6 commits intoRIOT-OS:masterfrom
fjmolinas:pr_rtt_enhancement
Closed

drivers/periph/rtt: enhancements to make RTT_FREQUENCY easily configurable#13874
fjmolinas wants to merge 6 commits intoRIOT-OS:masterfrom
fjmolinas:pr_rtt_enhancement

Conversation

@fjmolinas
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas commented Apr 15, 2020

Contribution description

This PR wants to make RTT_FREQUENCY easily configurable. Currently RTT_FREQUENCY is for most platforms a fixed value, I would like to be easily configurable at compile time.

I looked at most rtt implementations and in most cases RTT_FREQUENCY would be a a prescaled value of the clock source . Therefore I think a min and max value should be enough to map all possible values for RTT_FREQUENCY.

I added a RTT_CLOCK_FREQUENCY to make the clock source transparent, also useful for deriving prescaler values from the desired RTT_FREQUENCY.

I also force RTT_MAX_VALUE to be defined for all platforms.

I migrated all stm32f1 to implement the required values. I'm willing to do the same for all other rtt configurations if the global change makes sense.

Testing procedure

  • tests/periph_rtt should still pass on stm32f1 boards with different frequencies

BOARD=iotlab-m3 make -C tests/periph_rtt flash test --no-print-directory -j3

main(): This is RIOT! (Version: 2020.07-devel-56-g42700-pr_rtt_enhancement)

RIOT RTT low-level driver test
This test will display 'Hello' every 5 seconds

Initializing the RTT driver
RTT now: 0
Setting initial alarm to now + 5 s (81920)
Done setting up the RTT, wait for many Hellos
Hello
Hello
Hello
Hello
Hello

CFLAGS+=-DRTT_FREQUENCY=1 BOARD=iotlab-m3 make -C tests/periph_rtt clean flash test --no-print-directory -j3

main(): This is RIOT! (Version: 2020.07-devel-56-g42700-pr_rtt_enhancement)

RIOT RTT low-level driver test
This test will display 'Hello' every 5 seconds

Initializing the RTT driver
RTT now: 0
Setting initial alarm to now + 5 s (5)
Done setting up the RTT, wait for many Hellos
Hello
Hello
Hello
Hello
Hello
  • Invalid RTT_FREQUENCY are checked at compile time:
CFLAGS=-DRTT_FREQUENCY=0 BOARD=iotlab-m3 make -C tests/periph_rtt/ --no-print-directory
Building application "tests_periph_rtt" for "iotlab-m3" with MCU "stm32f1".

In file included from /home/francisco/workspace/RIOT/tests/periph_rtt/main.c:30:
/home/francisco/workspace/RIOT/drivers/include/periph/rtt.h:107:2: error: #warning "RTT_FREQUENCY is out of the allowed range, " "RTT_MIN_FREQUENCY < RTT_FREQUENCY < RTT_MAX_FREQUENCY" [-Werror=cpp]
 #warning "RTT_FREQUENCY is out of the allowed range, " \
  ^~~~~~~
cc1: all warnings being treated as errors
/home/francisco/workspace/RIOT/Makefile.base:110: recipe for target '/home/francisco/workspace/RIOT/tests/periph_rtt/bin/iotlab-m3/application_tests_periph_rtt/main.o' failed
make[1]: *** [/home/francisco/workspace/RIOT/tests/periph_rtt/bin/iotlab-m3/application_tests_periph_rtt/main.o] Error 1
/home/francisco/workspace/RIOT/tests/periph_rtt/../../Makefile.include:538: recipe for target '/home/francisco/workspace/RIOT/tests/periph_rtt/bin/iotlab-m3/application_tests_periph_rtt.a' failed
make: *** [/home/francisco/workspace/RIOT/tests/periph_rtt/bin/iotlab-m3/application_tests_periph_rtt.a] Error 2

Issues/PRs references

@fjmolinas
drivers/rtt: enhance rtt api
dfab419 
This adds the following defines that must be present in all RTT
configurations:

- RTT_CLOCK_FREQUENCY
- RTT_MAX_FREQUENCY
- RTT_MIN_FREQUENCY

Since RTT_REQUENCY is based on a prescaled value of RTT_CLOCK_FREQUENCY
valid values RTT_CLOCK_FREQUENCY % RTT_FREQUENCY = 0.

It also enforces RTT_MAX_VALUE to be defined.
@fjmolinas
cpu/stm32f1: add unified rtt configuration
42700d2
@fjmolinas fjmolinas added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: drivers Area: Device drivers labels Apr 15, 2020
@fjmolinas fjmolinas requested review from benpicco and kaspar030 April 15, 2020 10:11
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 16, 2020
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Apr 16, 2020

I think that makes a lot of sense!
Less code duplication and more flexibility.

@fjmolinas fjmolinas added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Apr 17, 2020
@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Apr 17, 2020

I think that makes a lot of sense!
Less code duplication and more flexibility.

Should have tagged it as wip since I didn't adapt all rtt yet, will do now.

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Apr 17, 2020

Should have tagged it as wip since I didn't adapt all rtt yet, will do now.

I could also just remove the error messages for now.

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Apr 17, 2020

I don't think you have to update all RTT implementations in one go/PR.
There is no interdependency and smaller PRs are easier to review & merge.

@fjmolinas
Copy link
Copy Markdown
Contributor Author

fjmolinas commented Apr 17, 2020

I don't think you have to update all RTT implementations in one go/PR.
There is no interdependency and smaller PRs are easier to review & merge.

Ok, I think that makes sense. I had started looking into different rtt.c implementations and adapting them, so I'll use this one as a tracking PR, and open a PR that just adds the variables, without enforcing them.

From what I've seen from other rtt implementations two arch might need a little work:

  • Kinetis: it uses an RTC as the source for rtt.c which limits frequency to 1Hz, it should probably use the LPTMR for rtt.c and the RTC only for `rtc.c.
  • esp8266: because of the lack of documentation (pointed out in the initial PR), I'm not sure of what the limits would be :/

@fjmolinas fjmolinas added Type: tracking The issue tracks and organizes the sub-tasks of a larger effort and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 17, 2020
This was referenced Apr 20, 2020
Merged
Merged
@fjmolinas fjmolinas mentioned this pull request May 5, 2020
Merged
@fjmolinas fjmolinas mentioned this pull request Jun 17, 2020
Merged
4 tasks
@fjmolinas fjmolinas mentioned this pull request Aug 11, 2020
Merged
@fjmolinas fjmolinas deleted the pr_rtt_enhancement branch July 30, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco Awaiting requested review from benpicco

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@DipSwitch DipSwitch Awaiting requested review from DipSwitch

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@smlng smlng Awaiting requested review from smlng

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: tracking The issue tracks and organizes the sub-tasks of a larger effort

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fjmolinas @benpicco