STM32 RTC sub seconds support #61580

niedzwiecki-dawid · 2023-08-17T07:14:44Z

Implement support for handling the sub second registers.

gautierg-st

Some comments on new magic values that could be replaced. Otherwise LGTM

drivers/counter/counter_ll_stm32_rtc.c

niedzwiecki-dawid · 2023-08-21T07:03:18Z

I haven't payed attention to that earlier, but the clock source was checked incorrectly. I've fixed that in the new, first commit.

drivers/counter/counter_ll_stm32_rtc.c

gautierg-st

LGTM! Thanks

drivers/counter/Kconfig.stm32_rtc

erwango

Please add some (at least minimal) way to get this compiled in CI

drivers/counter/counter_ll_stm32_rtc.c

niedzwiecki-dawid · 2023-09-11T12:42:02Z

Sorry for the delay, I'm working on the comments now.

niedzwiecki-dawid · 2023-09-11T12:42:44Z

Please add some (at least minimal) way to get this compiled in CI

Do you have any suggestions how to do it?

erwango · 2023-09-13T16:17:24Z

Here is the log:

*** Booting Zephyr OS build zephyr-v3.4.0-2458-gcfb5f828b708 ***
Running TESTSUITE counter_basic
===================================================================
START - test_all_channels
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing rtc@40002800
PASS - test_all_channels in 4.022 seconds
===================================================================
START - test_cancelled_alarm_does_not_expire
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing rtc@40002800
PASS - test_cancelled_alarm_does_not_expire in 7.891 seconds
===================================================================
START - test_late_alarm
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Skipped for rtc@40002800
PASS - test_late_alarm in 0.813 seconds
===================================================================
START - test_late_alarm_error
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Skipped for rtc@40002800
PASS - test_late_alarm_error in 0.813 seconds
===================================================================
START - test_multiple_alarms
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Skipped for counter
Skipped for counter
Skipped for rtc@40002800
PASS - test_multiple_alarms in 1.083 seconds
===================================================================
START - test_set_top_value_with_alarm
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Skipped for rtc@40002800
PASS - test_set_top_value_with_alarm in 1.576 seconds
===================================================================
START - test_short_relative_alarm
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing rtc@40002800
Assertion failed at WEST_TOPDIR/zephyr/tests/drivers/counter/counter_basic_api/src/test_counter.c:801: test_short_relative_alarm_instance: (i + 1 not equal to cnt)
rtc@40002800: Expected 6 callbacks, got 5
FAIL - test_short_relative_alarm in 0.753 seconds
===================================================================
START - test_single_shot_alarm_notop
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing rtc@40002800
Assertion failed at WEST_TOPDIR/zephyr/tests/drivers/counter/counter_basic_api/src/test_counter.c:403: test_single_shot_alarm_instance: (0 not equal to err)
rtc@40002800: Counter set alarm failed (err: -16)
FAIL - test_single_shot_alarm_notop in 1.221 seconds
===================================================================
START - test_single_shot_alarm_top
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Skipped for rtc@40002800
PASS - test_single_shot_alarm_top in 1.303 seconds
===================================================================
TESTSUITE counter_basic failed.
Running TESTSUITE counter_no_callback
===================================================================
START - test_set_top_value_without_alarm
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Testing counter
Skipped for rtc@40002800
PASS - test_set_top_value_without_alarm in 0.848 seconds
===================================================================
TESTSUITE counter_no_callback succeeded
------ TESTSUITE SUMMARY START ------
SUITE FAIL -  77.78% [counter_basic]: pass = 7, fail = 2, skip = 0, total = 9 duration = 19.475 seconds
- PASS - [counter_basic.test_all_channels] duration = 4.022 seconds
- PASS - [counter_basic.test_cancelled_alarm_does_not_expire] duration = 7.891 seconds
- PASS - [counter_basic.test_late_alarm] duration = 0.813 seconds
- PASS - [counter_basic.test_late_alarm_error] duration = 0.813 seconds
- PASS - [counter_basic.test_multiple_alarms] duration = 1.083 seconds
- PASS - [counter_basic.test_set_top_value_with_alarm] duration = 1.576 seconds
- FAIL - [counter_basic.test_short_relative_alarm] duration = 0.753 seconds
- FAIL - [counter_basic.test_single_shot_alarm_notop] duration = 1.221 seconds
- PASS - [counter_basic.test_single_shot_alarm_top] duration = 1.303 seconds
SUITE PASS - 100.00% [counter_no_callback]: pass = 1, fail = 0, skip = 0, total = 1 duration = 0.848 seconds
- PASS - [counter_no_callback.test_set_top_value_without_alarm] duration = 0.848 seconds
------ TESTSUITE SUMMARY END ------
===================================================================
RunID: c68cf89ac73c546729e0323ac92016ff
PROJECT EXECUTION FAILED

niedzwiecki-dawid · 2023-09-14T08:38:07Z

Thanks for the logs. How do you run the tests? It's executed on the nucleo_f429zi board, right? I've ordered the board to do it on my side and make sure everything is working fine. I should be able to reproduce the tests in a few days.

niedzwiecki-dawid · 2023-09-19T11:25:03Z

I've managed to reproduce the tests on the nucleo_f429zi board. Working on the fix.

niedzwiecki-dawid · 2023-09-20T08:58:00Z

I've debugged the problem.

The test failures occur when 1 tick alarm is programmed. For example test_short_relative_alarm test checks the 1 tick alarm 100 times. The alarm is not triggered in ~20% cases.

I've noticed that the alarm doesn't occur when it is programmed one tick before deadline. It is a case when one tick occurs during the calculation of register values in the set_alarm function. IMO it still should work, but I started reading the reference manual one more time and I found the sentence Each change of the RTC_CR register is taken into account after 1 to 2 RTCCLK clock cycles due to clock synchronization..

I think it means 1 tick alarm won't work for RTC with subseconds and an additional check should be added that the relative minimum tick value is 2.

@erwango Please let me know what you think.

erwango · 2023-09-20T13:20:55Z

@erwango Please let me know what you think.

It seems there's a good amount of drivers dealing with such cases already. See STM32 timer based counter driver for instance.
Would you be able to apply the same in current case ?

niedzwiecki-dawid · 2023-09-21T08:28:36Z

@erwango Please let me know what you think.

It seems there's a good amount of drivers dealing with such cases already. See STM32 timer based counter driver for instance. Would you be able to apply the same in current case ?

I had to use a bit different detection method. It works fine and passes all testes.

The config values have been hardcoded as magic values. Introduce universal calculation based on the DTS entries. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

Synchronize reading two separate registers. In some edge cases the read registers could point different dates. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

erwango · 2023-09-21T08:42:21Z

I had to use a bit different detection method. It works fine and passes all testes.

Thanks I'll have a look once I have free time.

Just a side note, when updating a PR, please don't rebase at the same time. This prevents to easily see the diff using the compare button provided by github and we have to review the whole change again.
If rebase is really required, please proceed in 2 steps:
rebase and force push then update PR and force push (or in the order that better suit you).

If everyone would do the same, it would save a lot of time to reviewers.

niedzwiecki-dawid · 2023-09-21T08:45:07Z

Just a side note, when updating a PR, please don't rebase at the same time. This prevents to easily see the diff using the [compare](https://github.com/zephyrproject-rtos/zephyr/compare/6b87b0597c6b94fb6b4b25474f5e4c473c1ffa6d..6ce4d587861db359b7e36337b55e7f4669f70c69) button provided by github and we have to review the whole change again. If rebase is really required, please proceed in 2 steps: rebase and force push then update PR and force push (or in the order that better suit you).

If everyone would do the same, it would save a lot of time to reviewers.

Thanks for the tip. I just noticed how it works.

niedzwiecki-dawid · 2023-09-22T07:44:44Z

Sorry for the noise. I run the auto-formatter

drivers/counter/counter_ll_stm32_rtc.c

Fix indentations of the API definition. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

RTC drivers uses only seconds, so transition to microseconds is necessary. Change way of calculation tick<->time to avoid unnecessary conversations. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

Add support for using the sub second registers. It allows reading and setting alarm with the sub second tick resolution. The RTC module is configured to get as high frequency as possible, which equals the source clock (RTCCLK) divided by 2. To get such frequency, the asynchronous prescaler is set to 1. According to RM, setting the asynchronous prescaler to a high value minimize consumption, so the change increase the power consumption. Use a config to enable the sub second support. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

Split the read function into 2 versions (date and no date) since they don't have common code. It improves readability. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

niedzwiecki-dawid · 2023-09-26T07:16:41Z

Thanks for the comments. I should have done the requested changes by myself.

erwango

LGTM, but I'd like @gautierg-st to approve as well.

gautierg-st

LGTM

niedzwiecki-dawid requested a review from nordic-krch as a code owner August 17, 2023 07:14

zephyrbot added platform: STM32 ST Micro STM32 area: Counter labels Aug 17, 2023

zephyrbot requested review from ABOSTM, Desvauxm-st, erwango, FRASTM, gautierg-st and GeorgeCGV August 17, 2023 07:15

zephyrbot assigned nordic-krch Aug 17, 2023

niedzwiecki-dawid requested review from gautierg-st and removed request for gautierg-st August 17, 2023 07:51

gautierg-st requested changes Aug 17, 2023

View reviewed changes

niedzwiecki-dawid force-pushed the stm32_rtc branch 3 times, most recently from 1c45c28 to 376f885 Compare August 21, 2023 07:02

niedzwiecki-dawid requested a review from gautierg-st August 23, 2023 08:21

gautierg-st requested changes Aug 23, 2023

View reviewed changes

drivers/counter/counter_ll_stm32_rtc.c Outdated Show resolved Hide resolved

drivers/counter/counter_ll_stm32_rtc.c Show resolved Hide resolved

drivers/counter/counter_ll_stm32_rtc.c Outdated Show resolved Hide resolved

niedzwiecki-dawid force-pushed the stm32_rtc branch 2 times, most recently from 7f529f0 to 75dac8b Compare August 24, 2023 05:40

niedzwiecki-dawid requested a review from gautierg-st August 24, 2023 05:42

gautierg-st previously approved these changes Aug 24, 2023

View reviewed changes

erwango reviewed Aug 28, 2023

View reviewed changes

drivers/counter/Kconfig.stm32_rtc Outdated Show resolved Hide resolved

erwango requested changes Aug 28, 2023

View reviewed changes

gautierg-st mentioned this pull request Sep 7, 2023

drivers: rtc: stm32: add calibration feature and several fixes/improvements #62304

Merged

faloj reviewed Sep 11, 2023

View reviewed changes

drivers/counter/counter_ll_stm32_rtc.c Show resolved Hide resolved

niedzwiecki-dawid dismissed gautierg-st’s stale review via 08ac741 September 12, 2023 14:13

erwango mentioned this pull request Sep 20, 2023

drivers: counter: fix index of stm32 RTC source clock #62794

Merged

niedzwiecki-dawid force-pushed the stm32_rtc branch from cfb5f82 to 6b87b05 Compare September 21, 2023 08:25

niedzwiecki-dawid added 2 commits September 21, 2023 10:36

counter: stm32 rtc: improve assigning configs

c808530

The config values have been hardcoded as magic values. Introduce universal calculation based on the DTS entries. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

counter: stm32 rtc: improve reading registers

c3b38f4

Synchronize reading two separate registers. In some edge cases the read registers could point different dates. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

niedzwiecki-dawid force-pushed the stm32_rtc branch from 6b87b05 to 6ce4d58 Compare September 21, 2023 08:36

niedzwiecki-dawid force-pushed the stm32_rtc branch 2 times, most recently from 1254d33 to b0a118d Compare September 22, 2023 06:36

niedzwiecki-dawid force-pushed the stm32_rtc branch from b0a118d to ee8654f Compare September 25, 2023 05:09

erwango requested changes Sep 25, 2023

View reviewed changes

drivers/counter/counter_ll_stm32_rtc.c Show resolved Hide resolved

drivers/counter/counter_ll_stm32_rtc.c Outdated Show resolved Hide resolved

drivers/counter/counter_ll_stm32_rtc.c Outdated Show resolved Hide resolved

drivers/counter/counter_ll_stm32_rtc.c Show resolved Hide resolved

niedzwiecki-dawid force-pushed the stm32_rtc branch from ee8654f to 26fe61a Compare September 26, 2023 06:47

niedzwiecki-dawid added 3 commits September 26, 2023 08:56

counter: stm32 rtc: fix indentations

ec42a22

Fix indentations of the API definition. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

counter: stm32 rtc: simplify calculation

9d5b5b0

RTC drivers uses only seconds, so transition to microseconds is necessary. Change way of calculation tick<->time to avoid unnecessary conversations. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

niedzwiecki-dawid force-pushed the stm32_rtc branch from 26fe61a to b946027 Compare September 26, 2023 06:57

counter: stm32 rtc: improve readability

20e3123

Split the read function into 2 versions (date and no date) since they don't have common code. It improves readability. Signed-off-by: Dawid Niedzwiecki <dawidn@google.com>

niedzwiecki-dawid force-pushed the stm32_rtc branch from b946027 to 20e3123 Compare September 26, 2023 07:05

niedzwiecki-dawid requested a review from erwango September 26, 2023 07:50

erwango approved these changes Sep 26, 2023

View reviewed changes

gautierg-st approved these changes Sep 26, 2023

View reviewed changes

carlescufi merged commit 68292b0 into zephyrproject-rtos:main Sep 26, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STM32 RTC sub seconds support #61580

STM32 RTC sub seconds support #61580

niedzwiecki-dawid commented Aug 17, 2023

gautierg-st left a comment

niedzwiecki-dawid commented Aug 21, 2023

gautierg-st left a comment

erwango left a comment

niedzwiecki-dawid commented Sep 11, 2023

niedzwiecki-dawid commented Sep 11, 2023

erwango commented Sep 13, 2023

niedzwiecki-dawid commented Sep 14, 2023

niedzwiecki-dawid commented Sep 19, 2023

niedzwiecki-dawid commented Sep 20, 2023 •

edited

erwango commented Sep 20, 2023

niedzwiecki-dawid commented Sep 21, 2023

erwango commented Sep 21, 2023 •

edited

niedzwiecki-dawid commented Sep 21, 2023

niedzwiecki-dawid commented Sep 22, 2023

niedzwiecki-dawid commented Sep 26, 2023

erwango left a comment

gautierg-st left a comment

STM32 RTC sub seconds support #61580

STM32 RTC sub seconds support #61580

Conversation

niedzwiecki-dawid commented Aug 17, 2023

gautierg-st left a comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented Aug 21, 2023

gautierg-st left a comment

Choose a reason for hiding this comment

erwango left a comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented Sep 11, 2023

niedzwiecki-dawid commented Sep 11, 2023

erwango commented Sep 13, 2023

niedzwiecki-dawid commented Sep 14, 2023

niedzwiecki-dawid commented Sep 19, 2023

niedzwiecki-dawid commented Sep 20, 2023 • edited

erwango commented Sep 20, 2023

niedzwiecki-dawid commented Sep 21, 2023

erwango commented Sep 21, 2023 • edited

niedzwiecki-dawid commented Sep 21, 2023

niedzwiecki-dawid commented Sep 22, 2023

niedzwiecki-dawid commented Sep 26, 2023

erwango left a comment

Choose a reason for hiding this comment

gautierg-st left a comment

Choose a reason for hiding this comment

niedzwiecki-dawid commented Sep 20, 2023 •

edited

erwango commented Sep 21, 2023 •

edited