Feature - RTC alarm on every second #370
Conversation
- reorder functions to context - check parameters for alarm
- use enum for multi-state value - code formatting
rename tNew -> new_dt
-1 replaced with enum value
|
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) |
src/rp2_common/hardware_rtc/rtc.c
Outdated
| if (t->month >= 1) s0 |= RTC_IRQ_SETUP_0_MONTH_ENA_BITS | (((uint)t->month) << RTC_IRQ_SETUP_0_MONTH_LSB); | ||
| if (t->day >= 1) s0 |= RTC_IRQ_SETUP_0_DAY_ENA_BITS | (((uint)t->day) << RTC_IRQ_SETUP_0_DAY_LSB ); |
There was a problem hiding this comment.
I'm not sure if changing the month and day checks to 1 instead of 0 is wanted here? 🤷
There was a problem hiding this comment.
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
-with rtc_delete_alarm() not only the the alarm is disabled but also the according exclusive interrupt is removed and everything else cleaned up - renamed CONTINUOUS_REPEAT_ON_SEC to CONTINUOUS_REPEAT_EVERY_SEC -added some helper macros for RANGE_CHECK - added macro to modify irq_setup_1 with incremented seconds and wrap at 60 - removed _callback check in ISR because this is handled now in setup alarm function - increased readability by changing bitwise and to logical and
it is superfluous to check all other conditions if _alarm_repeats is already set to every second
now the trigger interval can be given with a negative sec value. Also updated docu for rtc_set_alarm()
| // Set this when setting an alarm | ||
| static rtc_callback_t _callback = NULL; | ||
| static bool _alarm_repeats = false; | ||
| static uint8_t _seconds_increment = 1; |
There was a problem hiding this comment.
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)
| // repeatable every second! All entries are -1 | ||
| datetime_t new_dt; | ||
| _rtc_get_datetime(&new_dt); | ||
| _seconds_increment = (-t->sec) % 60; |
There was a problem hiding this comment.
even more confused, because t->sec is -1 here always
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh maybe you're confused because _seconds_increment is processed within the macro ADD_AND_ENABLE_REPEATABLE_SECOND?
There was a problem hiding this comment.
I think @Reneg973 was intending that you could set e.g. "repeat every 5 seconds" by setting t->sec to -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! 😕
There was a problem hiding this comment.
From SW side you are right, this functionality could be implemented in a level above.
There are 2 solutions then
- set an alarm "repeat every second" -> count the number of IRQs arrived :: fast ISR but highly increased number of IRQs
- set an alarm at current_time + x -> get the IRQ -> query datetime_t -> increment by x -> set new alarm via rtc_set_alarm() :: slow ISR but not more IRQs than required.
So both methods produce more overhead than really required.
Anyway I don't know which HAL strategy you want to follow for Pico.
There was a problem hiding this comment.
3rd approach: repeat every second -> query datetime_t -> if t->sec % 5 == 0 do 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"?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
@kilograham:
Repeating timers are not repeating clock ticks. Anyway here is my proposal for a modification to be able to do this outside:
- generate a new interface function rtc_modify_alarm() which reduces the overhead of rtc_set_alarm() and therefore can be called within the ISR.
- in almost all use cases when the user ISR is called, the RTC time will be queried. Give it as a parameter (tell don't ask principle).
What do you think?
| } | ||
| } | ||
|
|
||
| static void __no_inline_not_in_flash_func(rtc_irq_handler)(void) { | ||
| // Always disable the alarm to clear the current IRQ. | ||
| // Even if it is a repeatable alarm, we don't want it to keep firing. | ||
| // If it matches on a second it can keep firing for that second. | ||
| rtc_disable_alarm(); | ||
|
|
||
| if (_alarm_repeats) { |
There was a problem hiding this comment.
Should all the if (_alarm_repeats) checks actually be if (_alarm_repeats != NO_REPEAT) ? 🤷
There was a problem hiding this comment.
a question of coding guidelines. Technically it's the same
There was a problem hiding this comment.
Fair enough, that's a Graham-decision then 🙂
| // If any value is set to -1 then we don't match on that value | ||
| // hence the alarm will eventually repeat | ||
| if (t->year < 0 && t->month < 0 && t->day < 0 && t->dotw < 0 | ||
| && t->hour < 0 && t->min < 0 && t->sec < 0) return CONTINUOUS_REPEAT_EVERY_SEC; |
There was a problem hiding this comment.
Probably worth adding a comment here saying that if all values are -1 then we repeat every second?
the function rtc_set_alarm() is used to wakeup the system. There a user_callback might not exist. Therefore removed the NULL comparison => it must be checked in ISR. - also fixed RANGE_CHECK_DAY - added a PICO_CONFIG entry
The PR includes 2 changes