Add pcf8523 interrupts

[simzes]

Summary

This change adds support for programming timers and their interrupts to the library code for the PCF8523. No other RTCs have timer support with this change.

Impact and Overlap

No existing code was modified for this change. If the user does not use any of the added timer or interrupt functions, nothing on the hardware will change. There is an overlap in functionality between the timers and the square wave function: because the INT1/CLKOUT functions share a pin, enabling an interrupt for a timer will disable CLKOUT in favor of INT1, and will set the SqwPinMode to "OFF." There is a note to this effect in the write_timer function. This is visible from the readSqwPinMode function, and rewriting it with writeSqwPinMode will restore the CLKOUT function without any impact.

Added Functionality

This change adds several register addresses, enumerations, and structures for manipulating the timer and interrupt features of the pcf. A pair of functions for reading and writing its timers and interrupts has been added to the driver class. For the timers, the timer is selected from an enumeration, and its enable state, value, count frequency, and associated interrupt are read or written from a struct. A timer's interrupt can be read or written separately, giving access to its flag and output enable fields.

Examples and Test Programs

Two example programs, pcf8523_set_timer and pcf8523_monitor_timer, have been added. The set_timer program starts a timer, and periodically checks and prints out its status. The monitor_timer program starts a timer, and is similar to set_timer but also monitors a pin; this is pulled-up and connected to INT, and shows whether the interrupt pin is active. Runs of these programs show that all three timers are working correctly.

Documentation

Finally, a theory of operation section has been added to the main README, holding information about how the timers work, how interrupt pins should be wired to a microcontroller, and how they correspond to pins on adafruit pcf boards.

[drak7]

Already defined in Arduino.h but using a larger data type, might cause problems with other libraries that depend on that one. Can either use that one or choose a different name if the data type matters for this case.

[drak7]

These should go in RTClib.h

[drak7]

Please check the build details for the items that need Doxygen fixes, mostly macros and new class variables. Check out the Doxygen guide and tips: https://learn.adafruit.com/the-well-automated-arduino-library/doxygen

[czuvich]

Edit... everything works fine.

[czuvich]

I would love to see this merged into trunk! I've been using this PR for several months without any issues (using hour, second, and minute timers in deep sleep + wakeup).

[edgar-bonet]

@czuvich: It seems to me the functionality implemented here is roughly the same as in PR #163, which has already been merged into master and published as part of release 1.6.0. See the documentation of RTC_PCF8523 and check the methods that have “Timer” in their name.

[czuvich]

@edgar-bonet Sorry for the delay, but I just wanted to follow-up and confirm that everything is working great with the new alarm functionality! The API is much easier to use, too. Thanks for the great work.

[simzes]

Hi,

@colindgrant I am sorry for the delay. I didn't see any of this conversation.

I will work on addressing the feedback about code comments; it also looks like I have some merging to do.

I disagree that this functionality is the same as the current API. This is because:

• this interface supports all three timers available on the hardware
• the interrupt struct provides low-level access to the interrupt's drive vs. flag pin
• the interface provides a read feature as well; this is potentially important for applications that need to check how much time is left
• the interrupt struct is important because alarms share these contents

I understand that the interface is a little prickly; full-featured low-level control is important to establish and expose first. I will look at adding a more straightforward API.

[simzes]

Hi Matt (@drak7),

I added a string of commits to the pull request. I also have a question about the build.

The commits do a few things:

• The first three commits are almost the same as the original pull request: there was a merge conflict with register macro definitions, and I added a few updates to the readme.

• I added a string of commits to address your feedback:remove bit(x) macro, move look-up tables and a macro, and fix comments for doxygen.

• I added a few commits to re-use macro definitions that have since been added.

• Because there is other work with the pcf's second timer and interrupt pulse, and potential work on adding alarm support, I made the TimerState struct into a class. This will support modifications without breaking changes in the future.

For the broken build: I'm unsure what's causing it. Things are working fine with the arduino IDE over here. There isn't an informative error status in the clang step--it prints out a differently formatted source file, and then leaves with an error at the bottom.

Any ideas?

[edgar-bonet]

@simzes wrote:

For the broken build: I'm unsure what's causing it.

The CI script enforces some very strict code formatting policies through the “clang-format” tool. You can check the changes it is requesting by looking at the build log and clicking on the “clang” line. Alternatively, you can run clang-format on your local repo with its default formatting settings and commit the modified sources.

[edgar-bonet]

Suggested change

	while (1);
	Serial.flush();
	abort();

This is for consistency with the other examples and also, more importantly, in order to avoid undefined behavior.

[edgar-bonet]

Other than the infinite loop in the example, I have three other comments:

1. The example pcf8523_monitor_timer seems to be a slightly expanded version of pcf8523_set_timer. I then see no use in keeping the latter.

2. What is the rationale for turning Pcf8523TimerState into a class with constructors? Since this is nothing but a container for some state, with no associated behavior, it would seem more idiomatic to leave it as a plain struct, just like Pcf8523TimerDetails. An explicit copy constructor seems useless to me, and defining it violates the rule of three. The other constructor can be replaced by aggregate initialization:

Pcf8523TimerState state {
    true,   // timer set to run
    10,     // timer value
    PCF8523_FrequencySecond, // timer frequency
    false,  // timer interrupt flag (when timer has gone off)
    true    // timer flag -> signal enable
};

with an optional = before the opening brace.

3. I would avoid defining timer_details_table in the header file. If this file is included in more than one compilation unit, we end up with multiple definitions of the array. I would rather follow the standard practice of putting the definition in the .cpp file and the extern declaration in the header. Or maybe remove it from the header altogether, as this is not something the user of the library needs to have access to.