Add PCF8523 alarm functionality #264
Conversation
There was a problem hiding this comment.
This looks to me like a useful and well written contribution! I have a few comments below, which are only my personal opinions (I am not a maintainer of this repo).
You may have noticed that a continuous integration test failed. This is a script that runs clang-format in order to enforce some coding conventions. You may fix this by applying the patch that appears in the page of the failed test. Alternatively, you could run clang-format -i locally, as suggested in the README.
|
|
||
| RTC_PCF8523 rtc; | ||
|
|
||
| bool alarm_triggered = false; |
There was a problem hiding this comment.
This flag should be qualified as volatile.
|
|
||
| bool alarm_triggered = false; | ||
|
|
||
| void setAlarmExample(); |
There was a problem hiding this comment.
There is no function with this name. You probably mean doAlarmExample.
There was a problem hiding this comment.
Oh shoot! I had renamed it at some point. Thanks for pointing it out. :) As you mentioned in another comment, I'll probably just get rid of forward declarations entirely.
| void setAlarmExample(); | ||
| void alarmISR(); | ||
| void handleAlarmTriggered(); |
There was a problem hiding this comment.
You do not need to declare these functions, as the Arduino IDE handles those declarations for you.
I personally do prefer adding the declarations explicitly, rather that relying on this feature of the IDE. However, for the sake of consistency with the other provided examples, it think it would be better to omit them.
There was a problem hiding this comment.
Strangely I've had my Arduino IDE complain on and off about having forward declarations but I'm not exactly sure why. I usually have to restart the IDE or reboot to get it to stop complaining, so I think I just got in the habit of adding them, but the other examples in the library don't include them so I should probably remove them to be consistent.
| rtc.enableAlarmTimer(alarm_time, PCF8523_AlarmDate); | ||
|
|
||
| // Print the current time for demonstration purposes. | ||
| char current_time_buf[] = "YYYY, MMM, DD, DDD, hh:mm:ss AP"; |
There was a problem hiding this comment.
I think there are too many commas here. The date format also seems unusual.
For reference, ISO 8601 defines the big-endian format 2022-08-02 whereas RFC 2822 / RFC 5322 (internet mail) uses the little-endian format Tue, 02 Aug 2022. In the USA, a middle-endian format is quite common.
There was a problem hiding this comment.
Good idea, I should probably stick to a common format like ISO 8601.
| attachInterrupt(digitalPinToInterrupt(INTERRUPT_PIN), alarmISR, LOW); | ||
| } | ||
|
|
||
| void handleAlarmTriggered() |
There was a problem hiding this comment.
This function could be inlined in loop(), which is tiny.
| rtc.disableAlarmTimer(); | ||
| Serial.println("Alarm triggered.\n"); | ||
| doAlarmExample(); | ||
|
|
src/RTClib.h
Outdated
| void enableAlarmTimer(const DateTime &dt, const Pcf8523AlarmMode alarmMode, | ||
| uint8_t alarmWeekday); |
There was a problem hiding this comment.
This three-parameter version of enableAlarmTimer() has not been defined. It is not needed either.
There was a problem hiding this comment.
Thanks for noticing! I must have missed removing this. I was was originally going to make the weekday an explicit argument but pivoted and felt it might be easier to just grab it from the passed DateTime object like another alarm for the DS3231 RTCs in the library was already doing. I'll review and fix. Thanks!!! :)
Great suggestions! I was wondering about the build failing so I'll implement those changes and give clang-format a try when I'm at my workstation next (which will probably be tomorrow). Thanks a bunch for taking a look! |
|
I have implemented the suggestions from @edgar-bonet and ran clang-format, the build appears to have been successful this time. |
|
Any word on whether this PR will be merged? |
Summary
This commit adds basic alarm functionality to the PCF8523.
This addresses Issue #241.
Details
The follow methods have been added:
An example sketch has been added as well: pcf8523Alarm.ino
I have not added functionality to retrieve the alarm, although would be open to adding it in a similar fashion to #257
This is my first open source contribution on GitHub, please be kind. :)