bug in the timealarm library

hello all, I am new to arduino and doing a sprinkler system for my home as part as a more global homeautomation project

I think there are some bugs in timealarms.cpp inside the updateNextTrigger function

  • the check between weekly and global alarm is made with SECS_PER_WEEK instead of SECS_PER_WEEK+SECS_PER_DAY
    example an alarm made weekly on dowsaturday=7 at 22 the value is 7SECS_PER_DAY+22SECS_PER_HOUR ie more than SECS_PER_WEEK

  • when using dow,(SECS_PER_DAY < value < SECS_PER_WEEK+SECS_PER_DAY) the line

nextTrigger = value + previousMidnight(time);

is not correct it schould be

nextTrigger = value + previousfirstdowMidnight(time);

assuming the function previousfirstdowMidnight is properly created
furthermore the test made for daily alarm is not done:

if( value + previousMidnight(now()) <= time) 
		      {
            nextTrigger = value + nextMidnight(time); // if time has passed then set for tomorrow 
		      }
		      else
	        {
            nextTrigger = value + previousMidnight(time);  // set the date to today and add the time given in value	
		      }

this test need to be made for weekly alarm albeit a bit differently using again the previousfirstdowMidnight() function

I am available to make the correction in the library (need to modify timealarm and datetime) if everybody agree with these corrections.

I also think that there is space as already requested by others (such as newhobby the 25.02.2010) for some mean of identification of the called alarm if an handler could determine its ID making the i global (obviously using another name and this might not be the neatest because not multithread safe) in the serviceAlarms private function would be enough.

Best regards to everybody and congrats to those who created arduino

What is the source of the time library you refer to?

I think I might have written it too small :slight_smile: but the source concerned is
library/timealarms/timealarms.cpp

regards

I've been using this library for months, I must never have tried an alarm on Saturday. Did you change the macro for day of week?

#define dayOfWeek(time) ((( time / SECS_PER_DAY + 4) % DAYS_PER_WEEK)+1) // 1 = Sunday

This one did give me fits for a while before I found it in an old post. And, you did know that the library returns the identifier for the alarm and that can be checked in the callback to see if it's the one you want. That identifier can be used for other stuff also.

unfortunately, it's not the only issue when using Dow:

1st the 'value' of the alarm is calculated using this function
(7 + DOW - dayOfWeek(now())) %7) * SECS_PER_DAY + AlarmHMS(H,M,S)
this value is only valid for the next alarm to be set and works well with alarmonce for exemple
if you are on monday morning and set an alarm for (and each) tuesday 12:00AM, the 'value' is roughly 1 day and a half.
This is OK for the first alarm but the next one is also set 1 day and a half after midnight of the first ie: for wednesday (which is not the goal)

Concerning the id returned by the alarm.alarmrepeat, it works very well and it's very usefull from outside the alarmhandling, but sometimes you want the alarmhandling funtion to do different things base on which alarmid you are handling, and this is not doable, (or I missed something)

best regards

Viknet

Wow, it looks like you're right. The reason I haven't seen it is that I use the firing of an alarm to set another one for next time. That way I only use alarmOnce and just keep repeating that. I did that for debugging purposes when I was bringing the initial code up and never changed it. The author didn't notice that he needed to calculate for same time next week, not same time span from now.

Regarding which timer, can't you grab the timer id and the use it in the callback routine to tell which one you are responding to? Maybe I'm not understanding this problem.

thanks for aknowledging the issue,

concerning your question

can't you grab the timer id and the use it in the callback routine to tell which one you are responding to?

that's exactly what I would like to do, the code calling the callback is this one (simplified for everybody's understanding):

for(uint8_t i = 0; i < dtNBR_ALARMS; i++)
    {
      if( Alarm[i].Mode.isEnabled && (now() >= Alarm[i].nextTrigger)  )
      {
        OnTick_t TickHandler = Alarm[i].onTickHandler;
          (*TickHandler)();     // call the handler  
      }
    }

what I ould like to get is the 'i' variable, but because it is not global neither public I am stuck (unless once again I missed something)

By the way, I manage to find the issue because for testing purpose I put adjustTime(55); within the loop, it is very efficient for testing issue specialy if yur debug output is timestamped.

My code has runed 12 year (from 1970 to 1982) yesterday without visible issues or memory leak apart from this DOW things.

Best regards

Viknet

This post addresses the request for access to the alarm ID, I will follow up on 'day of week' in a later post.

The first version of this library was posted three years ago and did include the alarm ID in the callback.
Mellis from the Arduino team suggested that the API be simplified and I subsequently posted the current version that does not pass the alarm ID. If you are interested in the history, see this thread: http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1217881285

I had considered adding a method that could be invoked from the callback that would return the alarm id but had not seen a request for this up until now.

If you guys think it would be useful I can add the following method to the library:

AlarmID_t  getTriggeredAlarmId();  //returns the currently triggered  alarm id
// returns  dtINVALID_ALARM_ID if not invoked from within an alarm handler

This would enable a sketch along the following lines:

#include <Time.h>
#include <TimeAlarms.h>

AlarmID_t morningAlarmID, afternoonAlarmID, eveningAlarmID;

void setup()
{
  morningAlarmID   = Alarm.alarmRepeat(8,30,0, onAlarm); 
  afternoonAlarmID = Alarm.alarmRepeat(13,25,0,onAlarm); 
  eveningAlarmID   = Alarm.alarmRepeat(17,45,0,onAlarm);   
}

void onAlarm()
{
  // alarm callback function
  AlarmId id = Alarm. getTriggeredAlarmId();

  if(id == morningAlarmID) {
    Serial.println("Morning Alarm"); 
  } 
  else if (id == afternoonAlarmID) {  
    Serial.println("Afternoon Alarm"); 
  } 
  else if (id == eveningAlarmID) {  
    Serial.println("Evening Alarm"); 
  }  
  else {  
    Serial.println("Invalid Alarm ID"); 
  }
}

void loop()
{
  Alarm.delay(1000);
}

The implementation would required the following new method :

AlarmID_t getTriggeredAlarmId()();  //returns the currently triggered  alarm id
// returns  dtINVALID_ALARM_ID if not invoked from within an alarm handler
{
  if(isServicing)
       return  servicedAlarmId;  // new private data member used instead of local loop variable i in serviceAlarms();
  else
     return dtINVALID_ALARM_ID; // valid ids only available when servicing a callback
}

If something like this would be generally useful then I can add it into the next release

Michael Margolis

viknet, re the DOW issue can you post your revised updateNextTrigger method here so we can give it a try.

Also post your macro for calculating the time at midnight of the previous day of week

Thanks,

Michael

I will :=)

but I have to code it first (my main job is testing not coding :=) )

give me a few days and I will do it.

I wanted to make sure first that

  • The bug was a real one
  • The way to correct it was ok

it seems so, so I will correct it ASAP and post it on the forum.

Regards

Viknet

Viknet, support for recurring weekly alarms is a problem but I am not sure how easy it will be to fix. This functionality was not considered when the library was designed and it will be interested to see if there is a simple solution to this.

Michael

mem,
concerning your proposal for the

AlarmID_t  getTriggeredAlarmId();

it is EXACTLY what I wanted, you made it a lot nicer than I would have done it (probably using a full global variables) I should have rememebered school class with getter and setter.........

thanks a lot if you can add it to the library (If not I will do it on for my project)

best regards

Viknet

Viknet, why not add the method I posted above to your local copy of the library and give it a try.

If it tests out for you ok then I will add it to the next release.

Michael

edit: don't forget to add the new private data member to the header file:

uint8_t servicedAlarmId; // the alarm currently being serviced

and in the cpp file, change the serviceAlarms method so it uses this instead of the local variable i as follows:

void TimeAlarmsClass::serviceAlarms()
{
  if(! isServicing)
  {
    isServicing = true;
    for( servicedAlarmId = 0; servicedAlarmId < dtNBR_ALARMS; servicedAlarmId++)
    {
      if( Alarm[servicedAlarmId].Mode.isEnabled && (now() >= Alarm[servicedAlarmId].nextTrigger)  )
      {
        OnTick_t TickHandler = Alarm[i].onTickHandler;
        if(Alarm[servicedAlarmId].Mode.isOneShot)
          Alarm[servicedAlarmId].Mode.isEnabled = Alarm[servicedAlarmId].Mode.isAllocated = false;  // free the ID if mode is OnShot		
        else   
           Alarm[servicedAlarmId].updateNextTrigger();
        if( TickHandler != NULL) {        
          (*TickHandler)();     // call the handler  
        }
      }
    }
    isServicing = false;
  }
}

I just tested the changes for getting the alarm id. There were a few items I had to change to compile it and then I ran it with Vicknet's trick of updating the seconds in every loop for a couple of (virtual) years. It worked fine. Of course since I was setting the seconds (or in a few high speed tests, minutes) it didn't hit right on the second, but it ran fine.

The code I used to test it with:

#include <Time.h>
#include <TimeAlarms.h>

AlarmID_t morningAlarmID, afternoonAlarmID, eveningAlarmID;
char buf[100];

void setup()
{
  Serial.begin(57600);
  Serial.println("Initializing..");
  morningAlarmID   = Alarm.alarmRepeat(8,30,0, onAlarm); 
  afternoonAlarmID = Alarm.alarmRepeat(13,25,0,onAlarm); 
  eveningAlarmID   = Alarm.alarmRepeat(17,45,0,onAlarm);   
}

void onAlarm()
{
  // alarm callback function
  AlarmId id = Alarm. getTriggeredAlarmId();

  if(id == morningAlarmID) {
    Serial.println("Morning Alarm"); 
  } 
  else if (id == afternoonAlarmID) {  
    Serial.println("Afternoon Alarm"); 
  } 
  else if (id == eveningAlarmID) {  
    Serial.println("Evening Alarm"); 
  }  
  else {  
    Serial.println("Invalid Alarm ID"); 
  }
  sprintf(buf,"Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),day(),month(),year());
  Serial.println(buf);
}

void loop()
{
  Alarm.delay(0);
  adjustTime(55);
//  sprintf(buf,"Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),day(),month(),year());
//  Serial.println(buf);
  
}

TimeAlarms.h

#ifndef TimeAlarms_h
#define TimeAlarms_h

#include <inttypes.h>

#include "Time.h"

#define dtNBR_ALARMS 20

typedef enum { dtMillisecond, dtSecond, dtMinute, dtHour, dtDay } dtUnits_t;

typedef struct  {
    uint8_t isAllocated            :1 ;  // the alarm is avialable for allocation if false
    uint8_t isEnabled              :1 ;  // the timer is only actioned if isEnabled is true 
    uint8_t isOneShot              :1 ;  // the timer will be de-allocated after trigger is processed 
    uint8_t isAlarm                :1 ;  // time of day alarm if true, period timer if false  
 }
    AlarmMode_t   ;

typedef uint8_t AlarmID_t;
typedef AlarmID_t AlarmId;  // Arduino friendly name
#define dtINVALID_ALARM_ID 255


class AlarmClass;  // forward reference
typedef void (*OnTick_t)();  // alarm callback function typedef 

// class defining an alarm instance, only used by dtAlarmsClass
class AlarmClass
{  
private:
public:
  AlarmClass();
  OnTick_t onTickHandler;  
  void updateNextTrigger();
  time_t value;
  time_t nextTrigger;
  AlarmMode_t Mode;
};

// class containing the collection of alarms
class TimeAlarmsClass
{
private:
   AlarmClass Alarm[dtNBR_ALARMS];
   void serviceAlarms();
   uint8_t isServicing;
   uint8_t servicedAlarmId; // the alarm currently being serviced
   AlarmID_t create( time_t value, OnTick_t onTickHandler, uint8_t isAlarm, uint8_t isOneShot, uint8_t isEnabled=true);
   
public:
  TimeAlarmsClass();
  // functions to create alarms and timers
  AlarmID_t alarmRepeat(time_t value, OnTick_t onTickHandler);                    // trigger daily at given time of day
  AlarmID_t alarmRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler); // as above, with hms arguments
  AlarmID_t alarmRepeat(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler); // as above, with day of week 
 
  AlarmID_t alarmOnce(time_t value, OnTick_t onTickHandler);                     // trigger once at given time of day
  AlarmID_t alarmOnce( const int H,  const int M,  const int S, OnTick_t onTickHandler);  // as above, with hms arguments
  AlarmID_t alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler); // as above, with day of week 
  
  AlarmID_t timerOnce(time_t value, OnTick_t onTickHandler);   // trigger once after the given number of seconds 
  AlarmID_t timerOnce(const int H,  const int M,  const int S, OnTick_t onTickHandler);   // As above with HMS arguments
  
  AlarmID_t timerRepeat(time_t value, OnTick_t onTickHandler); // trigger after the given number of seconds continuously
  AlarmID_t timerRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler);   // As above with HMS arguments
  
  void delay(unsigned long ms);
   
  // utility methods
  uint8_t getDigitsNow( dtUnits_t Units);         // returns the current digit value for the given time unit
  void waitForDigits( uint8_t Digits, dtUnits_t Units);  
  void waitForRollover(dtUnits_t Units);
  void clear();						 // disable and clear all alarms from memory
  time_t readAlarm(AlarmID_t ID);			 // return the value for given alarm
  time_t readTimer(AlarmID_t ID);			 // return the value for given timer 
  uint8_t count();
  AlarmID_t getTriggeredAlarmId();
  // low level methods
  void enable(AlarmID_t ID);
  void disable(AlarmID_t ID);
  void write(AlarmID_t ID, time_t value);    // write the value (and enable) the alarm with the given ID
  time_t read(AlarmID_t ID);                 // return the value for the given timer 
};

extern TimeAlarmsClass Alarm;  // make an instance for the user

/*==============================================================================
 * MACROS
 *============================================================================*/

/* public */
#define waitUntilThisSecond(_val_) waitForDigits( _val_, dtSecond)
#define waitUntilThisMinute(_val_) waitForDigits( _val_, dtMinute)
#define waitUntilThisHour(_val_)   waitForDigits( _val_, dtHour)
#define waitUntilThisDay(_val_)    waitForDigits( _val_, dtDay)
#define waitMinuteRollover() waitForRollover(dtSecond)
#define waitHourRollover()   waitForRollover(dtMinute)
#define waitDayRollover()    waitForRollover(dtHour)

#define AlarmHMS(_hr_, _min_, _sec_) (_hr_ * SECS_PER_HOUR + _min_ * SECS_PER_MIN + _sec_)

#endif /* TimeAlarms_h */

Thanks draythomp , I will make that addition to the next release

and pieces of TimeAlarms.cpp, It wouldn't all fit in the allowed size:

// return the value for alarms only
time_t TimeAlarmsClass::readAlarm(AlarmID_t ID)
{
  if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated && Alarm[ID].Mode.isAlarm)
    return Alarm[ID].value ;
  else
    return 0l;  
}

// return the value for timers only
time_t TimeAlarmsClass::readTimer(AlarmID_t ID)
{
  if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated && Alarm[ID].Mode.isAlarm == false && Alarm[ID].Mode.isOneShot)
    return Alarm[ID].value ;
  else
    return 0l;  
}

void TimeAlarmsClass::clear()  // clear and disable all alarms and make them avialable for allocation  
{
  for(uint8_t id = 0; id < dtNBR_ALARMS; id++){
     Alarm[id].Mode.isEnabled = false;  
     Alarm[id].Mode.isAllocated = false;
  }
} 

uint8_t TimeAlarmsClass::count()  // return the number of alarms active 
{
  uint8_t count = 0;
  for(uint8_t id = 0; id < dtNBR_ALARMS; id++){
    if (Alarm[id].Mode.isAllocated)
	count++;
  }
  return(count);
}

AlarmID_t TimeAlarmsClass::getTriggeredAlarmId() //returns the currently triggered  alarm id
// returns  dtINVALID_ALARM_ID if not invoked from within an alarm handler
{
  if(isServicing)
       return  servicedAlarmId;  // new private data member used instead of local loop variable i in serviceAlarms();
  else
     return dtINVALID_ALARM_ID; // valid ids only available when servicing a callback
}



// and under the private stuff

 void TimeAlarmsClass::serviceAlarms()
{
  if(! isServicing)
  {
    isServicing = true;
    for( servicedAlarmId = 0; servicedAlarmId < dtNBR_ALARMS; servicedAlarmId++)
    {
      if( Alarm[servicedAlarmId].Mode.isEnabled && (now() >= Alarm[servicedAlarmId].nextTrigger)  )
      {
        OnTick_t TickHandler = Alarm[servicedAlarmId].onTickHandler;
        if(Alarm[servicedAlarmId].Mode.isOneShot)
          Alarm[servicedAlarmId].Mode.isEnabled = Alarm[servicedAlarmId].Mode.isAllocated = false;  // free the ID if mode is OnShot		
        else   
           Alarm[servicedAlarmId].updateNextTrigger();
        if( TickHandler != NULL) {        
          (*TickHandler)();     // call the handler  
        }
      }
    }
    isServicing = false;
  }
}

Sorry, I was posting stuff at the same time you were.

However, you'll notice that I included stuff from other people that have commented. The count function is especially useful if you're putting stuff in on the fly for dynamic timers. It helps keep from running out of them! Ok, so I get a little carried away from time to time.

draythomp, are any of the other methods you added needed to get functionality that is otherwise not available?

I am reluctant to add methods to the library that can be implemented in a few line of code in the sketch. But I am happy to hear cogent reasons for doing otherwise. It looks to me that most users of the library would not need that functionality and those that do can easily add it into the sketch.

Michael

Actually, that is a really good question; I hadn't thought about that.

The count function is probably the only function that I actually need. That is, unless there is a way I can do it in some other fashion; in which case, I certainly don't mind having code in the sketch to handle it. The others I stuck in were to handle some silly thing I was working on at that particular time and I'm not really sure they make much sense looking back.

On that note, a weekly alarm based on day of week can be handled in the fashion I accidentally found: Set an alarmonce for Tuesday at 10AM and when it fires, set one for Tuesday at 10AM. I know, it's not as elegant, but it seems to work.

Frankly though, this is such a cool library. I use it a lot, even for things like turning off an LED. When an event happens, I turn on an LED and set a timer for a second or so from now to turn it off. That way my code can blissfully wander off doing other things and the timer takes care of the details for me. So, you can see why I need the count function ! Heck, updating displays, sounding a bell when the soups ready, testing fire alarms, there's just no end to it.