search

JChristensen / DS3232RTC

Arduino Library for Maxim Integrated DS3232 and DS3231 Real-Time Clocks
GNU General Public License v3.0
392 stars 135 forks source link

DS3232RTC::alarmInterrupt issue #102

Open amazed-2022 opened 1 year ago

amazed-2022 commented 1 year ago

alarmInterrupt(ALARM_NBR_t alarmNumber, bool alarmEnabled) Description

Enable or disable an alarm "interrupt". Note that this "interrupt" just causes the RTC's INT pin to be asserted. To use this signal as an actual interrupt to (for example) a microcontroller, the RTC "interrupt" pin needs to be connected to the microcontroller and the microcontroller application firmware must properly configure the interrupt and also provide for handling it.

In case of alarmEnabled = true is requested for any of the alarms, the INTCN bit in the control register could be also set, to make sure that the INT pin is really asserted. (calling this method will not result the described behavior, in case of INTCN was set to 0 previously).

JChristensen commented 1 year ago

Is there no scenario where I want to set A2IE and/or A1IE and not set INTCN, or perhaps not set it simultaneously?

It may be better to control INTCN independently of A1IE and A2IE. Calling squareWave(DS3232RTC::SQWAVE_NONE) will set INTCN. Calling it with any other valid argument will clear INTCN.

I can see where the documentation for alarmInterrupt() and also for squareWave() could be improved.

amazed-2022 commented 1 year ago

Is there no scenario where I want to set A2IE and/or A1IE and not set INTCN, or perhaps not set it simultaneously?

I can't think of such scenario. Setting A1IE and/or A2IE without INTCN will do nothing.

JChristensen commented 1 year ago

I can't think of such scenario.

Off the top, I am not sure that I can either, but it could be coded that way. There may be a small risk that if alarmInterrupt() is changed to always set INTCN, it could break something for someone.

The suggestion here is a convenience feature; it would avoid a call to squareWave(). Perhaps the best approach would be to implement a new method that is like alarmInterrupt() but that sets INTCN as well as AxIE. That way, backwards compatibility is ensured.

amazed-2022 commented 1 year ago

There may be a small risk that if alarmInterrupt() is changed to always set INTCN, it could break something for someone.

I understand your concern. However if anyone used alarmInterrupt() for arm an interrupt before, he/she already had INTCN bit set to 1 (the default value when power is first applied), otherwise their code wouldn't work, at least not as they intended.

The suggestion here is a convenience feature; it would avoid a call to squareWave().

I don't think it's convenience, it would just make more sense that alarmInterrupt() can really arm the interrupt. Currently, viewing from application level, it's like squareWave(SQWAVE_NONE) has a hidden feature, enabling/preparing INT pin toggling.

Anyway, I thought this minor code/documentation (as you mentioned earlier) issue is worth mentioning. To anyone who is familiar with the device's documentation and knows the how to set/reset register bits, this wouldn't be a problem.