Improved RTC Functionality

[adamgarbo]

Hi @nseidle,

Happy to finally submit my Artemis RTC functionality improvements! I've attempted to address all of your comments from the related SparkFun forum post.

Notable changes:

Added functions:

void setEpoch(uint32_t ts); // Set's the RTC's date and time using UNIX Epoch time
void getAlarm(); // Get's the RTC current alarm date/time
void setAlarm(uint8_t hund, uint8_t sec, uint8_t min, uint8_t hour, uint8_t dayOfMonth, uint8_t month); // Set's alarm time
void setAlarmMode(uint8_t mode); // Set's the RTC's alarm interrupt mode
void attachInterrupt(); // Enables the RTC alarm interrupt
void detachInterrupt(); // Disables the RTC alarm interrupt

Note that I've changed the order of date/time values for setTime() from:
hh, mm, ss, hund, dd, mm, yy
to:
hund, ss, mm, hh, dd, mm, yy

I believe this is a more intuitive way to set the RTC time, and is also in line with the majority of other SparkFun RTC libraries (e.g. RV1805, RV8803, DS1307, DS3234, etc.). However, glad to get your thoughts on this. As far as I’m aware, this will only break code in the OpenLog Artemis repositories (both OLA and Paul’s GNSS logger). However, these are a minor fixes that I can submit PRs for.

I also created a number of examples to demonstrate the added functionality. I attempted to keep them as simple as possible and included more explanatory information in the comment section.

Examples:

1. Example1_Get_Time
   • Not modified.
2. Example2_Set_Alarms
   • A simple example to set the RTC alarm, which replaces the existing example 2. Now makes use of am_isr_rtc() instead of am_stimer_cmpr6_isr().
3. Example3_Test_RTC
   • Not modified.
4. Example4_Set_Epoch
   • Demonstrates how to set the RTC using UNIX Epoch time.
5. Example5_Rolling_Alarms
   • Demonstrates how to read and set rolling RTC alarms.
6. Example6_LowPower_Alarm
   • Demonstrates how to set an RTC alarm and enter/wake from deep sleep.

All code was tested using a SparkFun Redboard Artemis, as well as an Edge 2 to confirm the sleep current draw in Example 6.

Happy to address any major/minor concerns!

Cheers,
Adam

[oclyke]

@adamgarbo I just want to chime in and let you know this PR is not forgotten - we will get to it as soon as we can :D

[stephenf7072]

Looks like good stuff @adamgarbo. Changing the parameters order of setTime() is not backwards-compatible with anyone (me!) who has written custom code, although it does make more sense. Can I suggest the new version of the function is called "setTimeA()" or something to differentiate it.

[adamgarbo]

Thanks @oclyke!

Let me know if there's anything I can do to help!

Cheers,
Adam

[oclyke]

@adamgarbo, @stephenf7072

I'm reviewing right now - about ready to merge. So far the only bother I have is @stephenf7072's comment about backward compatibility of the setTime() function. According to semantic versioning making this change should push us to v2.0.0 (which is planned for soon anyway) but I want to include this as part of a final patch to the v1.x.x lineage.

I do prefer the new order of setTime parameters. Perhaps we can keep the functional changes in this PR and separate the API changes into a new PR that I will bring in along with v2.0.0?

@adamgarbo would you be willing to commit changes that (somehow) revert the API back to its original state?

[oclyke]

Generally looks good. Formatting changes to examples are acceptable. Requesting that the setTime API change is undone in this PR and moved to a separate PR for v2.0.0

[oclyke]

Just a note - I'm not sure why the test compile jobs are failing. I tried compilation on my local machine and it worked fine. Now that I am more familiar with GitHub actions I may investigate further.

[adamgarbo]

Hi @oclyke,

Thanks for getting to this!

I'm happy to revert the order of the setTime parameters until the v2.0.0 release. I will try to get to this soon, as I'll need to change all the examples that make use of the new setTime parameter ordering.

What are your thoughts on the order of the setAlarm parameters? Should they be left as such (i.e. hund, ss, mm, hh, dd, mm), or changed to match setTime (hh, mm, ss, hund, dd, mm)?

Cheers,
Adam

[oclyke]

Let's match them for this release and then change both APIs at once in v2.0.0

It seems most intuitive that the two would match. I'll see if I can't help with the examples.

[oclyke]

I've changed setAlarm to match setTime and updated the examples. All compile. I'd appreciate a second pair of eyes for testing to ensure that the examples are accurate.

[adamgarbo]

Hi @oclyke,

Just reviewed the changes and they all look good!

Cheers,
Adam

[oclyke]

Just noticed some odd behavior - before my latest commit Example3 was broken. This turned out to be due to unclear logic but it was harder to diagnose because setTime does not appear to update the internal structures myRTC.dayOfMonth etc...

For example:

myRTC.setTime(23, 59, 59, 0, 1, 1, 20);
Serial.printf("weekday: %d\n", myRTC.weekday);

prints
weekday: 0

but

myRTC.setTime(23, 59, 59, 0, 1, 1, 20);
myRTC.getTime();
Serial.printf("weekday: %d\n", myRTC.weekday);

prints
weekday: 3

Is there a reason for this? Or can we change setTime to include a call to getTime internally? This would make a lot more sense to me - after all why would a clock ever have two different times?

[adamgarbo]

Hi Owen,

Based on my experiences with a number RTC libraries, I've found that it's actually a common behaviour to need to manually poll/update the RTC before executing a print statement. That being said, I don't see any reason why we couldn't include the bulk of the getTime() function in setTime(). This would basically just involve manually updating the time variables (e.g. hour, minute, second, etc.) when the time is set.

I think one of the reasons for this behaviour is due to the fact we're using the HAL as a middleman to write to the RTC registers.

Cheers,
Adam

[oclyke]

This is all great, thanks @adamgarbo!