sparkfun / Arduino_Apollo3

Arduino core to support the Apollo3 microcontroller from Ambiq Micro
83 stars 39 forks source link

RTC Rolling Alarms Issue #398

Closed rserranosmith closed 2 years ago

rserranosmith commented 3 years ago

Hello,

In the RTC Rolling Alarms example, I have experienced an issue where the rtc.setAlarm() function may sometimes set the alarm for a time in the past, due to not taking into account an increment in the minute (or hour) time after the second (or minute) exceeds 60.

A suggested fix is shown below.

  int secs = (rtc.seconds + alarmSeconds);
  int mins = (rtc.minute + alarmMinutes + (int)(secs/60));
  int hours = (rtc.hour + alarmHours + (int)(mins/60));
  rtc.setAlarm(0,
                 secs % 60,
                 mins % 60,
                 hours % 24,
                 rtc.dayOfMonth,
                 rtc.month);
Wenn0101 commented 3 years ago

I agree this is a problem. It is being masked by the alarm repeat interval (alarmMode) in the example. An example of the failure can be seen in under a minute by changing the rtc.setAlarmMode to <5.

Fixed in this commit. I wanted to give you credit for the fix so I added you to the author list for the example.

adamgarbo commented 3 years ago

Hi @rserranosmith,

Thanks for the fix! I have also experienced this rollover issue in the past.

I'd be curious to know if you have any thoughts on how to handle day rollovers, which can't easily be addressed using a modulo operator of say: days % 30. My thoughts are that you'd need to make the program aware of the number of days in each month. Granted, this added complexity goes beyond the scope of this example.

Cheers, Adam

Wenn0101 commented 2 years ago

Closing this issue, as the main issue has been resolved.

As a side note I think a good way to answer your question to do this would be to have an array with the number of days in each month, and use this for your math. So month_days = [31, 28, 31, 30, ...] and then do, rtc.days % month_days[rtc.month-1]; This would still need something for a leap year, but I think a solution like this could work.