Feature - RTC alarm on every second #370
No reviewers
Labels
No labels
bazel
bluetooth
board settings
boot_stage2
bug
build
c++
can't reproduce
clang
clib-interface
cmsis
code cleanliness
code size
compiler version issues
cyw43_driver
debug
discussion
documentation
duplicate
dynamic_clock
elf2uf2
enhancement
errata
filesystem
flash layout
floating point
FreeRTOS
hardware_adc
hardware_claim
hardware_clocks
hardware_dma
hardware_exception
hardware_flash
hardware_gpio
hardware_i2c
hardware_irq
hardware_pio
hardware_pwm
hardware_rcp
hardware_rtc
hardware_spi
hardware_sync
hardware_uart
hardware_vreg
hardware_watchdog
hardware_xosc
help wanted
host
IAR
installation
invalid
linker scripts
lto
lwip
mbedtls
missing API
needs reporter detail
networking
newlib
newlib-multicore
non-c
non-cmake
non-gcc
openocd
orphan PR
OS integration
pico_async_context
pico_base
pico_binary_info
pico_flash
picolibc
pico_lwip
pico_malloc
pico_multicore
pico_printf
pico_runtime
pico_stdio
pico_stdio_usb
pico_sync
pico_time
pico_util
pioasm
port
question
review
review2
rtos interop
runtime init
static library
svd
test
tinyusb
vscode
wifi integration
wontfix
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gh-raspberrypi/pico-sdk!370
Loading…
Reference in a new issue
No description provided.
Delete branch "Reneg973/RTC-Changes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The PR includes 2 changes
@ -11,9 +11,28 @@#include "hardware/resets.h"#include "hardware/clocks.h"prefer to use an enum for 3 repeat types
@ -117,1 +142,3 @@static void rtc_irq_handler(void) {void rtc_disable_alarm(void) {// Disable matching and wait for it to stop being activehw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);This seems race prone; is using the RTC clock for an every second interrupt useful?
@ -117,1 +142,3 @@static void rtc_irq_handler(void) {void rtc_disable_alarm(void) {// Disable matching and wait for it to stop being activehw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);at least it works fine - updating my display every second while pushing the current time on UART.
But there is another more critical thing missing. ATM an attached exclusive interrupt can't be removed.
@ -11,9 +11,28 @@#include "hardware/resets.h"#include "hardware/clocks.h"I'll upate it
@ -117,1 +142,3 @@static void rtc_irq_handler(void) {void rtc_disable_alarm(void) {// Disable matching and wait for it to stop being activehw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);the current ISR still enables rtc_alarm before user callback has returned. Maybe we should change this too?
@ -117,1 +142,3 @@static void rtc_irq_handler(void) {void rtc_disable_alarm(void) {// Disable matching and wait for it to stop being activehw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);Another question, usually RTC uses 32.768kHz clock, here 46.875kHz is programmed. What is the cause for this?
You need to click the "Edit" button at the top of this page and change the base branch 😉 (which will clean up the messy commit history that GH is currently displaying)
@ -11,9 +11,28 @@#include "hardware/resets.h"#include "hardware/clocks.h"This looks like it might be a typo for
CONTINUOUS_REPEAT_ONE_SEC😉 PerhapsCONTINUOUS_REPEAT_EVERY_SECONDmight be less ambiguous?@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Graham might feel differently to me, but I wonder if explicitly doing
might be more readable than using bitwise-AND ?
(your version is also missing the space between the
ifand the(🙂 )@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.This comment should probably refer to the enum-values rather than -1 ?
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Should this be declared to return
repeat_typerather thanint8_t?@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Would an alarm be considered "valid" if it e.g. had a month of
14and a day of-1?@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.I'm not sure if changing the month and day checks to
1instead of0is wanted here? 🤷@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Should this be the first test in this chain-of-ifs, rather than only appearing in the
elseclause?Is this change needed because otherwise the RTC alarm would just fire continuously?
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Might be better to check
_alarm_repeats == NO_REPEATrather than!_alarm_repeats?@ -11,9 +11,28 @@#include "hardware/resets.h"#include "hardware/clocks.h"Not a typo, just a different point of view. An alarm is triggered ON change of x, where x may be SEC or MIN or HOUR, but all other possible alarms/triggers are already included in CONTINUOUS_REPEAT.
From SW pov you are right, I'll modify it
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Right, I already changed it, just not pushed - anyway it needs nearly twice the operations and therefore cycles to check
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Overseen to remove this comment, changed also return type of function
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.very good objection, needs a better breakdown
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Yes, that is a strange inconsistency in the original code. The RTC reports months from 1-12, but the input allowed a month of 0. If I assume the RTC follows the KISS principle, input and output will have the same ranges. Anyway I am not 100% sure
IRQ has been triggered on second x. This code is to reenable it for second (x+1)%60. If not incremented, it would delay until it reaches second x again
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.oh yes, it should be first case
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.right and done
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.done
also white space/formatting issues still
@ -15,2 +24,3 @@static rtc_callback_t _callback = NULL;static bool _alarm_repeats = false;static uint8_t _seconds_increment = 1;not sure why this is state; it seems like it should be a macro parameter as the usage on line 161 should always be 1 (not the last value)
even more confused, because t->sec is -1 here always
maybe best described with an example:
if sec input parameter for rtc_set_alarm() is in range 0<=s<=59, than we have absolute behavior. The alarm always appears when internal comparison of s with the RTC seconds is true.
If sec value is negative, it is interpreted as a relative value. Means if it is -1, _seconds_increment is set to 1, and the alarm appears whenever the seconds change. If now input sec is -2, the increment will be 2 and therefore the ISR is called every 2 seconds. As this input value is just local within rtc_set_alarm(), it must be remembered as a state, so that ISR can access it.
@ -33,2 +33,4 @@#endif// PICO_CONFIG: PARAM_ASSERTIONS_ENABLED_RTC, Enable/disable assertions in the RTC module, type=bool, default=0, group=hardware_rtc#ifndef PARAM_ASSERTIONS_ENABLED_RTCCould you add a PICO_CONFIG line as well please? See https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/hardware_adc/include/hardware/adc.h#L47 for an example.
(This allows the value to be set by https://github.com/raspberrypi/pico-project-generator )
@ -11,9 +11,28 @@#include "hardware/resets.h"Which months have 32 days? 😉
E.g. like snooze function on a clock. Set alarm to -30sec repeat, alarm appears, you press button to switch off the alarm, and after 30s it appears again.
@ -11,9 +11,28 @@#include "hardware/resets.h"Oh there are months that won't end at all :)
I was not sure whether <= or < has better readability, and so this slipped through.
@ -114,60 +139,89 @@ void rtc_enable_alarm(void) {}@kilograham Do we definitely want to just
return falsehere (and not set the alarm) if auser_callbackwasn't supplied?Oh maybe you're confused because _seconds_increment is processed within the macro ADD_AND_ENABLE_REPEATABLE_SECOND?
Feels like this function would be more robust if this check was done near the end rather than near the start? (or simply remove the check altogether, since
_callbacklater gets set to NULL anyway)I think @Reneg973 was intending that you could set e.g. "repeat every 5 seconds" by setting
t->secto -5 ?But I'm not sure if that's desirable behaviour, since e.g. there's no similar functionality to set "repeat every 5 minutes" (and we're not trying to re-implement
cron😉 ) so I think it might be more consistent to revert the "repeat every 5 seconds" functionality, and just stick to the "setting everything to -1 causes the alarm to repeat every second" behaviour. IMHO. For more sophisticated behaviour, the user can always add their own custom logic inside their callback function.EDIT: That's weird, I was replying to @kilograham 's comment and for some reason GH didn't show me @Reneg973 's comments until after I posted mine! 😕
no i was confused because I didn't notice you could now set this to negative values other than -1. I'm not sure that this functionality belongs here; there are plenty of other ways of getting repeating timers.
hardware_foolibraries are generally intended for exposing hardware functionality in a clean API, not using the hardware to provide additional functionality.I need to take a closer look at the RTC h/w, so I'll get back to you
@ -114,60 +139,89 @@ void rtc_enable_alarm(void) {}ok this one is wrong - I fix it
Datasheet 4.8.3. says this event is also used to wakeup the pico
@ -114,60 +139,89 @@ void rtc_enable_alarm(void) {}Anyway I think the HAL should differ between a user event and a wakeup from dormant mode, because if it is in dormant mode, the user callback may not be executed as clk_sys may be 0
Is this a condition that can actually happen? Won't it be caught by the earlier checks?
Should all the
if (_alarm_repeats)checks actually beif (_alarm_repeats != NO_REPEAT)? 🤷Probably worth adding a comment here saying that if all values are -1 then we repeat every second?
it can happen if one parameter is set to a value > range and all others are -1.
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.But with RP2040 running at 125MHz, and the RTC alarm only firing at most once per second, I don't think that should be a huge problem? 😉
yes, I removed the user_callback check and therefore it must be removed here too. But this leads to the next issue, see
irq_remove_handler()/irq_set_exclusive_handler(). Seems like a NULL handler is handled as a valid function ptr there.
a question of coding guidelines. Technically it's the same
Fair enough, that's a Graham-decision then 🙂
Perhaps it's worth adding an
is_valid_alarmtimefunction, and using that as part of thevalid_params_ifchecks?@ -117,1 +142,3 @@static void rtc_irq_handler(void) {void rtc_disable_alarm(void) {// Disable matching and wait for it to stop being activehw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);The RP2040 datasheet says: "XOSC @ 12MHz / 256 = 46875Hz. To get a 1Hz reference CLKDIV_M1 should be set to 46874."
As long as
CLKDIV_M1is set to an appropriate value, I don't think it matters what frequency the RTC clock runs at? (Looks like https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/hardware_clocks/clocks.c#L187 is where clk_rtc is actually configured)@ -180,12 +234,19 @@ void rtc_set_alarm(datetime_t *t, rtc_callback_t user_callback) {irq_set_enabled(RTC_IRQ, true);Should this be
irq_remove_handler(RTC_IRQ, rtc_irq_handler)? 🤷 (IRQ-related code isn't my strong point!)@ -117,1 +142,3 @@static void rtc_irq_handler(void) {void rtc_disable_alarm(void) {// Disable matching and wait for it to stop being activehw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);Exactly, 12MHz / 256... I found it. Would be nice if the comment you linked would say the same
@ -180,12 +234,19 @@ void rtc_set_alarm(datetime_t *t, rtc_callback_t user_callback) {irq_set_enabled(RTC_IRQ, true);nice, so I had in mind that I made too many changes with 1 commit ... and prompt forgot to fix this.
@ -180,12 +234,19 @@ void rtc_set_alarm(datetime_t *t, rtc_callback_t user_callback) {irq_set_enabled(RTC_IRQ, true);done
I don't think it's worth the effort as the function already returns false then. Just an assertion could appear - if they are enable for rtc.
I think the more critical question is, what should happen with a given user_callback, if pico is woken up from RTC event?
But I think this would successfully reject the "a month of 14 and a day of -1" that I mentioned earlier? Meh 🤷
Looking at https://github.com/raspberrypi/pico-extras/blob/master/src/rp2_common/pico_sleep/sleep.c#L106 (linked to from the RP2040 datasheet) it seems that whatever callback is passed to
sleep_goto_sleep_until(which of course might be NULL) overwrites any existing RTC user_callback.BTW I hope you don't feel that I'm being too picky with all my comments, I'm only trying to offer constructive feedback 🙂 And if any of my comments conflict with what @kilograham says, please ignore my comments and listen to Graham 😀
It's fine as your comments make sense. And now, good find! Yes, at least
sleep_goto_sleep_untiltries to overwrite the user callback. But the user callback is set withirq_set_exclusive_handler. And now we find ourselves within anhard_assert(current == __unhandled_user_irq || current == handler);So my freshly implemented rtc_delete_alarm() is directly required for
sleep_goto_sleep_until:-)Are you sure? I think you might be mixing up the irq_handler and the user_callback again? AFAICT (and I could easily be entirely wrong!) the
sleep_goto_sleep_untilonly changes the user_callback (which gets called by the irq_handler when the RTC alarm fires), and doesn't actually change the irq_handler? 🤷ok, second condition then is true (current==handler)
From SW side you are right, this functionality could be implemented in a level above.
There are 2 solutions then
So both methods produce more overhead than really required.
Anyway I don't know which HAL strategy you want to follow for Pico.
3rd approach: repeat every second -> query datetime_t -> if
t->sec % 5 == 0do something, else ignore 😉As I mentioned before, with RP2040 running with a CPU clock of 125MHz I don't think you really need to worry about "slow ISR"?
You do need to worry!
3rd approach = 1st :) - I meant the same! And t->sec % 5 is not correct, you need include the start seconds.
And 125MHz is just the default. I have improved my SPI performance by 22% (increase SPI freq 31.25MHz -> 40MHz), but for this I had to sacrifice 36% of clk_sys by setting it to 80MHz!
@ -114,60 +139,89 @@ void rtc_enable_alarm(void) {}ok no user callback in sleep because it's overwritten
@ -33,2 +33,4 @@#endif// PICO_CONFIG: PARAM_ASSERTIONS_ENABLED_RTC, Enable/disable assertions in the RTC module, type=bool, default=0, group=hardware_rtc#ifndef PARAM_ASSERTIONS_ENABLED_RTCdone
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.I am simply no friend of increasing the number of execution cycles just to increase readability. If it's not clear, it could be documented.
@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.Another Graham-decision I think 😉
EDIT: If "bitwise-ands and a single-check" is preferable to "separate-checks and logical-ands" for performance reasons, perhaps
(t->year < 0 || t->month < 0 || t->day < 0 || t->dotw < 0 || t->hour < 0 || t->min < 0 || t->sec < 0)could also be condensed into a "bitwise-or and a single-check"? 🤷♂️@ -118,1 +148,4 @@}static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) {// Always disable the alarm to clear the current IRQ.It could, I would be happy to change it if accepted. But of course it's a little bit bit "hacking" at a place where it is not really time critical. Just a few clock cycles and a few bytes binary code size reduction.
@kilograham:
Repeating timers are not repeating clock ticks. Anyway here is my proposal for a modification to be able to do this outside:
What do you think?
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.