Optional clear flag in checkIfAlarm() / #9

[jnuernberg]

This PR tackles the challanges brought up in issue #9 by adding an addition method checkIfAlarm to the DS3231 class, that makes clearing the alarm flags optional.

Summary:

• added new method bool DS3231::checkIfAlarm(byte Alarm, bool clearflag)
• added polling example AlarmPolling.ino to test said method and provide an example for polling alarm code

With this PR, we may close #9.

Best regards,
Jacob

[IowaDave]

So... you're overloading the checkIfAlarm() function? Now there would be two versions, one with the boolean parameter and the other without it?

I don't oppose this change. I'm asking the question to gather information for future documentation, because if we go this way then both of these similarly-named methods would need to be disclosed and their differences explained.

Observations without any intent to criticize:

1. Overloading appears to be redundant for fetching the value of the specified alarm flag (1 or 2).
2. How likely is the use case for checking the status of the alarm flag while leaving unchanged?

I mention the following solely for conversational purposes.

I've thought about this same issue. The approach I considered was to separate and distinguish the two tasks that the function presently performs.

• Add a void clearAlarm(n) method which would simply clear the specified alarm flag without bothering to return its value.
• Remove the flag-clearing side effect of checkIfAlarm(n) and even rename it to checkAlarm(n).

Then the two functions would be clearly different and more or less self-documenting:

• bool checkAlarm(n) returns the state of the specified flag but does not alter it.
• void clearAlarm(n) forces the flag to the "clear" status but does not return the state it was in before clearing it.

[jnuernberg]

Yes, that is absolutely correct. I overload the checkIfAlarm with a second input parameter. This was a design decision with respect to backwards compatibility and consistency.

First, I wouldn't want to alter the existing method too much for it could break existing code. Your suggestion to separate the alarm check and the clear flag is surely an option, but this would break code, that relies on clearing the alarm flag in checkIfAlarm (for example the AlarmInterrupt example).

Second, checking the alarm without clearing the flag is actually not that uncommon. In particular, in interrupting code, you usually want to respond to one alarm uninterrupted until all your alarm logic is completed and then clear the flag. In fact, the AlarmInterrupt example does exactly that, however, it uses the interrupt to check the alarm.
Let me give a specific example: Say you use both alarms on the clock with interrupting code and you want to complete your alarm logic without being interrupted. Then you need checkIfAlarm to figure out, which alarm has fired, without clearing the flag as otherwise, the other alarm could interrupt your logic. Once your logic has run, you can then clear the flag.

I hope this explains my design decisions.

Best regards,
Jacob

[IowaDave]

I understand. As you wish. Thank you.

We could avoid breaking old code while adding the two, distinctly named functions to the Library for the benefit of clarity in future code. Leave the checkIfAlarm() function in there, including the overloading you propose, but deprecate it.

[awickert]

Overloading seems to me a good solution to keep backwards compatibility while adding the requested more limited scope of the function.

[awickert]

A note to @jnuernberg: It seems that your local commits are not linked with your GitHub account, and therefore the system is not noting your contributions to the repository. If this is important to you, please link these, and I will try to retroactively apply this. In this way, your name will appear when generating doi-labeled releases via GitHub + Zenodo.

[jnuernberg]

Hi @awickert, thanks for pointing that out. I had my global configs changed lately for demonstration purposes an apparantly didn't change them back.
The only way to relink the commits (that I know of) is a rebase. So I went ahead and did that in the branch issue9. We can remerge it into master. It's not really important for me, but as we are on it, why not. If it's too much of an effort, we can also just stall/delete this branch.
Again, thanks for pointing it out, and thanks for the power entrusted 👍