Skip to content

Feature - RTC alarm on every second #370

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.

Sign up for GitHub

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

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Feature - RTC alarm on every second #370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b3fa3e6
- better const correctness
Reneg973 May 1, 2021
61309a6
new feature, alarm on every second
Reneg973 May 1, 2021
f945ab8
Update rtc.c
Reneg973 May 1, 2021
66ceabd
removed hungarian
Reneg973 May 1, 2021
eddfce4
overseen one comparison
Reneg973 May 1, 2021
3deda98
added rtc_delete_alarm
Reneg973 May 2, 2021
82a4681
check (_alarm_repeats == CONTINUOUS_REPEAT_EVERY_SEC) first
Reneg973 May 2, 2021
27cb80e
improved with seconds increment given as negative value
Reneg973 May 2, 2021
ecedcbe
removed user_callback check
Reneg973 May 2, 2021
1a6884b
fixed remove the correct IRQ handler
Reneg973 May 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/rp2_common/hardware_rtc/include/hardware/rtc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@
extern "C" {
#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_RTC
#define PARAM_ASSERTIONS_ENABLED_RTC 0
#endif

/*! Callback function type for RTC alarms
* \ingroup hardware_rtc
*
Expand All @@ -50,7 +55,7 @@ void rtc_init(void);
* \param t Pointer to a \ref datetime_t structure contains time to set
* \return true if set, false if the passed in datetime was invalid.
*/
bool rtc_set_datetime(datetime_t *t);
bool rtc_set_datetime(const datetime_t *t);

/*! \brief Get the current time from the RTC
* \ingroup hardware_rtc
Expand All @@ -69,10 +74,12 @@ bool rtc_running(void);
/*! \brief Set a time in the future for the RTC to call a user provided callback
* \ingroup hardware_rtc
*
* \param t Pointer to a \ref datetime_t structure containing a time in the future to fire the alarm. Any values set to -1 will not be matched on.
* \param t Pointer to a \ref datetime_t structure containing a time in the future to fire the alarm. Any values set to a negative value will not be matched on.
* With one exception: If all values are negative, it will be matched on every step of abs(datetime_t::sec).
* \param user_callback pointer to a \ref rtc_callback_t to call when the alarm fires
* \return false if parameters aren't valid
*/
void rtc_set_alarm(datetime_t *t, rtc_callback_t user_callback);
bool rtc_set_alarm(const datetime_t *t, rtc_callback_t user_callback);

/*! \brief Enable the RTC alarm (if inactive)
* \ingroup hardware_rtc
Expand All @@ -84,6 +91,11 @@ void rtc_enable_alarm(void);
*/
void rtc_disable_alarm(void);

/*! \brief Deletes the alarm previously set with \see rtc_set_alarm
* \ingroup hardware_rtc
*/
void rtc_delete_alarm(void);

#ifdef __cplusplus
}
#endif
Expand Down
187 changes: 124 additions & 63 deletions src/rp2_common/hardware_rtc/rtc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,28 @@
#include "hardware/resets.h"
#include "hardware/clocks.h"

#define RANGE_CHECK_YEAR(t) (t->year >= 0 && t->year <= 4095)
#define RANGE_CHECK_MONTH(t) (t->month >= 1 && t->month <= 12)
#define RANGE_CHECK_DAY(t) (t->day >= 1 && t->day <= 31)
#define RANGE_CHECK_DOTW(t) (t->dotw >= 0 && t->dotw <= 6)
#define RANGE_CHECK_HOUR(t) (t->hour >= 0 && t->hour <= 23)
#define RANGE_CHECK_MIN(t) (t->min >= 0 && t->min <= 59)
#define RANGE_CHECK_SEC(t) (t->sec >= 0 && t->sec <= 59)


// Set this when setting an alarm
static rtc_callback_t _callback = NULL;
static bool _alarm_repeats = false;
static uint8_t _seconds_increment = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


#define ADD_AND_ENABLE_REPEATABLE_SECOND(s) (RTC_IRQ_SETUP_1_SEC_ENA_BITS | ((((uint)s + _seconds_increment) % 60) << RTC_IRQ_SETUP_1_SEC_LSB))

typedef enum {
NO_REPEAT = 0,
CONTINUOUS_REPEAT = 1,
CONTINUOUS_REPEAT_EVERY_SEC = 2,
} repeat_type;

static repeat_type _alarm_repeats = NO_REPEAT;

bool rtc_running(void) {
return (rtc_hw->ctrl & RTC_CTRL_RTC_ACTIVE_BITS);
Expand All @@ -39,21 +58,38 @@ void rtc_init(void) {
rtc_hw->clkdiv_m1 = rtc_freq;
}

static bool valid_datetime(datetime_t *t) {
static bool is_valid_datetime(const datetime_t *t) {
// Valid ranges taken from RTC doc. Note when setting an RTC alarm
// these values are allowed to be -1 to say "don't match this value"
if (!(t->year >= 0 && t->year <= 4095)) return false;
if (!(t->month >= 1 && t->month <= 12)) return false;
if (!(t->day >= 1 && t->day <= 31)) return false;
if (!(t->dotw >= 0 && t->dotw <= 6)) return false;
if (!(t->hour >= 0 && t->hour <= 23)) return false;
if (!(t->min >= 0 && t->min <= 59)) return false;
if (!(t->sec >= 0 && t->sec <= 59)) return false;
return true;
return RANGE_CHECK_YEAR(t)
&& RANGE_CHECK_MONTH(t)
&& RANGE_CHECK_DAY(t)
&& RANGE_CHECK_DOTW(t)
&& RANGE_CHECK_HOUR(t)
&& RANGE_CHECK_MIN(t)
&& RANGE_CHECK_SEC(t);
}

bool rtc_set_datetime(datetime_t *t) {
if (!valid_datetime(t)) {
// small helper without check for running rtc
static inline datetime_t _rtc_get_datetime(datetime_t *t) {
// Note: RTC_0 should be read before RTC_1
uint32_t rtc_val = rtc_hw->rtc_0;
t->dotw = (rtc_val & RTC_RTC_0_DOTW_BITS) >> RTC_RTC_0_DOTW_LSB;
t->hour = (rtc_val & RTC_RTC_0_HOUR_BITS) >> RTC_RTC_0_HOUR_LSB;
t->min = (rtc_val & RTC_RTC_0_MIN_BITS) >> RTC_RTC_0_MIN_LSB;
t->sec = (rtc_val & RTC_RTC_0_SEC_BITS) >> RTC_RTC_0_SEC_LSB;

rtc_val = rtc_hw->rtc_1;
t->year = (rtc_val & RTC_RTC_1_YEAR_BITS) >> RTC_RTC_1_YEAR_LSB;
t->month = (rtc_val & RTC_RTC_1_MONTH_BITS) >> RTC_RTC_1_MONTH_LSB;
t->day = (rtc_val & RTC_RTC_1_DAY_BITS) >> RTC_RTC_1_DAY_LSB;
}

bool rtc_set_datetime(const datetime_t *t) {
bool check_params = is_valid_datetime(t);
valid_params_if(RTC, check_params);

if (!check_params) {
return false;
}

Expand Down Expand Up @@ -91,18 +127,7 @@ bool rtc_get_datetime(datetime_t *t) {
return false;
}

// Note: RTC_0 should be read before RTC_1
uint32_t rtc_0 = rtc_hw->rtc_0;
uint32_t rtc_1 = rtc_hw->rtc_1;

t->dotw = (rtc_0 & RTC_RTC_0_DOTW_BITS ) >> RTC_RTC_0_DOTW_LSB;
t->hour = (rtc_0 & RTC_RTC_0_HOUR_BITS ) >> RTC_RTC_0_HOUR_LSB;
t->min = (rtc_0 & RTC_RTC_0_MIN_BITS ) >> RTC_RTC_0_MIN_LSB;
t->sec = (rtc_0 & RTC_RTC_0_SEC_BITS ) >> RTC_RTC_0_SEC_LSB;
t->year = (rtc_1 & RTC_RTC_1_YEAR_BITS ) >> RTC_RTC_1_YEAR_LSB;
t->month = (rtc_1 & RTC_RTC_1_MONTH_BITS) >> RTC_RTC_1_MONTH_LSB;
t->day = (rtc_1 & RTC_RTC_1_DAY_BITS ) >> RTC_RTC_1_DAY_LSB;

_rtc_get_datetime(t);
return true;
}

Expand All @@ -114,60 +139,89 @@ void rtc_enable_alarm(void) {
}
}

static void rtc_irq_handler(void) {
void rtc_disable_alarm(void) {
// Disable matching and wait for it to stop being active
hw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);
while (rtc_hw->irq_setup_0 & RTC_IRQ_SETUP_0_MATCH_ACTIVE_BITS) {
tight_loop_contents();
}
}

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should all the if (_alarm_repeats) checks actually be if (_alarm_repeats != NO_REPEAT) ? 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a question of coding guidelines. Technically it's the same

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, that's a Graham-decision then 🙂

// If it is a repeatable alarm, re enable the alarm.
rtc_enable_alarm();
if(_alarm_repeats == CONTINUOUS_REPEAT_EVERY_SEC) {
// we need to modify the sec entry with the next valid RTC change and store it into irq_setup_1
datetime_t t;
rtc_get_datetime(&t);
rtc_hw->irq_setup_1 = ADD_AND_ENABLE_REPEATABLE_SECOND(t.sec);
}
}

// Call user callback function
if (_callback) {
if (_callback)
_callback();

// If it is a repeatable alarm, re enable the alarm.
if(_alarm_repeats) {
rtc_enable_alarm();
}
}

static bool rtc_alarm_repeats(datetime_t *t) {
// 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) return true;
if (t->month < 0) return true;
if (t->day < 0) return true;
if (t->dotw < 0) return true;
if (t->hour < 0) return true;
if (t->min < 0) return true;
if (t->sec < 0) return true;
return false;
static repeat_type rtc_alarm_repeats(const datetime_t *t) {
// 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;
Comment on lines +176 to +179
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth adding a comment here saying that if all values are -1 then we repeat every second?


return (t->year < 0 || t->month < 0 || t->day < 0 || t->dotw < 0
|| t->hour < 0 || t->min < 0 || t->sec < 0)
? CONTINUOUS_REPEAT : NO_REPEAT;
}

void rtc_set_alarm(datetime_t *t, rtc_callback_t user_callback) {
rtc_disable_alarm();
bool rtc_set_alarm(const datetime_t *t, rtc_callback_t user_callback) {
if (!rtc_running())
return false;

// Only add to setup if it isn't -1
rtc_hw->irq_setup_0 = ((t->year < 0) ? 0 : (((uint)t->year) << RTC_IRQ_SETUP_0_YEAR_LSB )) |
((t->month < 0) ? 0 : (((uint)t->month) << RTC_IRQ_SETUP_0_MONTH_LSB)) |
((t->day < 0) ? 0 : (((uint)t->day) << RTC_IRQ_SETUP_0_DAY_LSB ));
rtc_hw->irq_setup_1 = ((t->dotw < 0) ? 0 : (((uint)t->dotw) << RTC_IRQ_SETUP_1_DOTW_LSB)) |
((t->hour < 0) ? 0 : (((uint)t->hour) << RTC_IRQ_SETUP_1_HOUR_LSB)) |
((t->min < 0) ? 0 : (((uint)t->min) << RTC_IRQ_SETUP_1_MIN_LSB )) |
((t->sec < 0) ? 0 : (((uint)t->sec) << RTC_IRQ_SETUP_1_SEC_LSB ));
rtc_disable_alarm();

// Set the match enable bits for things we care about
if (t->year >= 0) hw_set_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_YEAR_ENA_BITS);
if (t->month >= 0) hw_set_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MONTH_ENA_BITS);
if (t->day >= 0) hw_set_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_DAY_ENA_BITS);
if (t->dotw >= 0) hw_set_bits(&rtc_hw->irq_setup_1, RTC_IRQ_SETUP_1_DOTW_ENA_BITS);
if (t->hour >= 0) hw_set_bits(&rtc_hw->irq_setup_1, RTC_IRQ_SETUP_1_HOUR_ENA_BITS);
if (t->min >= 0) hw_set_bits(&rtc_hw->irq_setup_1, RTC_IRQ_SETUP_1_MIN_ENA_BITS);
if (t->sec >= 0) hw_set_bits(&rtc_hw->irq_setup_1, RTC_IRQ_SETUP_1_SEC_ENA_BITS);
uint32_t s0 = 0, s1 = 0;

// Does it repeat? I.e. do we not match on any of the bits
_alarm_repeats = rtc_alarm_repeats(t);

bool check_params = (is_valid_datetime(t) || _alarm_repeats != NO_REPEAT);
valid_params_if(RTC, check_params);
if(!check_params) // none of the parameters is valid
return false;

// Set the match enable bits for things we care about
if(_alarm_repeats == CONTINUOUS_REPEAT_EVERY_SEC) {
// repeatable every second! All entries are -1
datetime_t new_dt;
_rtc_get_datetime(&new_dt);
_seconds_increment = (-t->sec) % 60;
Copy link
Contributor

Choose a reason for hiding this comment

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

even more confused, because t->sec is -1 here always

Copy link
Contributor Author

@Reneg973 Reneg973 May 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh maybe you're confused because _seconds_increment is processed within the macro ADD_AND_ENABLE_REPEATABLE_SECOND?

Copy link
Contributor

@lurch lurch May 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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! 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From SW side you are right, this functionality could be implemented in a level above.
There are 2 solutions then

  1. set an alarm "repeat every second" -> count the number of IRQs arrived :: fast ISR but highly increased number of IRQs
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor

Choose a reason for hiding this comment

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

You do need to worry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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_foo libraries 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:

  1. generate a new interface function rtc_modify_alarm() which reduces the overhead of rtc_set_alarm() and therefore can be called within the ISR.
  2. 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?

s1 = ADD_AND_ENABLE_REPEATABLE_SECOND(new_dt.sec);
}
else {
if (RANGE_CHECK_YEAR(t)) s0 |= RTC_IRQ_SETUP_0_YEAR_ENA_BITS | (((uint)t->year) << RTC_IRQ_SETUP_0_YEAR_LSB);
if (RANGE_CHECK_MONTH(t)) s0 |= RTC_IRQ_SETUP_0_MONTH_ENA_BITS | (((uint)t->month) << RTC_IRQ_SETUP_0_MONTH_LSB);
if (RANGE_CHECK_DAY(t)) s0 |= RTC_IRQ_SETUP_0_DAY_ENA_BITS | (((uint)t->day) << RTC_IRQ_SETUP_0_DAY_LSB);
if (RANGE_CHECK_DOTW(t)) s1 |= RTC_IRQ_SETUP_1_DOTW_ENA_BITS | (((uint)t->dotw) << RTC_IRQ_SETUP_1_DOTW_LSB);
if (RANGE_CHECK_HOUR(t)) s1 |= RTC_IRQ_SETUP_1_HOUR_ENA_BITS | (((uint)t->hour) << RTC_IRQ_SETUP_1_HOUR_LSB);
if (RANGE_CHECK_MIN(t)) s1 |= RTC_IRQ_SETUP_1_MIN_ENA_BITS | (((uint)t->min) << RTC_IRQ_SETUP_1_MIN_LSB);
if (RANGE_CHECK_SEC(t)) s1 |= RTC_IRQ_SETUP_1_SEC_ENA_BITS | (((uint)t->sec) << RTC_IRQ_SETUP_1_SEC_LSB);

if(!s0 && !s1) return false; // out of range datetime_t input
}

rtc_hw->irq_setup_0 = s0;
rtc_hw->irq_setup_1 = s1;

// Store function pointer we can call later
_callback = user_callback;

Expand All @@ -180,12 +234,19 @@ void rtc_set_alarm(datetime_t *t, rtc_callback_t user_callback) {
irq_set_enabled(RTC_IRQ, true);

rtc_enable_alarm();
return true;
}

void rtc_disable_alarm(void) {
// Disable matching and wait for it to stop being active
hw_clear_bits(&rtc_hw->irq_setup_0, RTC_IRQ_SETUP_0_MATCH_ENA_BITS);
while(rtc_hw->irq_setup_0 & RTC_IRQ_SETUP_0_MATCH_ACTIVE_BITS) {
tight_loop_contents();
}
void rtc_delete_alarm(void)
{
// first disable the alarm
rtc_disable_alarm();

// don't receive interrupts anymore
rtc_hw->inte = RTC_INTE_RESET;

// disable IRQ and remove handler
irq_remove_handler(RTC_IRQ, rtc_irq_handler);

_callback = NULL;
}