Arduino Forum

Development => Other Software Development => Topic started by: viknet on Jun 28, 2011, 04:00 pm

Title: bug in the timealarm library
Post by: viknet on Jun 28, 2011, 04:00 pm
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 7*SECS_PER_DAY+22*SECS_PER_HOUR ie more than SECS_PER_WEEK

- when using dow,(SECS_PER_DAY < value < SECS_PER_WEEK+SECS_PER_DAY) the line
Code: [Select]
nextTrigger = value + previousMidnight(time);
is not correct it schould be
Code: [Select]
nextTrigger = value + previousfirstdowMidnight(time);
assuming the function previousfirstdowMidnight is properly created
furthermore the test made for daily alarm is not done:
Code: [Select]
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
Title: Re: bug in the timealarm library
Post by: robtillaart on Jun 28, 2011, 04:04 pm

What is the source of the time library you refer to?
Title: Re: bug in the timealarm library
Post by: viknet on Jun 28, 2011, 05:51 pm
I think I might have written it too small :-) but the source concerned is
library/timealarms/timealarms.cpp

regards
Title: Re: bug in the timealarm library
Post by: draythomp on Jun 28, 2011, 08:25 pm
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.
Title: Re: bug in the timealarm library
Post by: viknet on Jun 29, 2011, 12:17 am
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
Title: Re: bug in the timealarm library
Post by: draythomp on Jun 29, 2011, 12:47 am
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.
Title: Re: bug in the timealarm library
Post by: viknet on Jun 29, 2011, 08:08 am
thanks for aknowledging the issue,

concerning your question
Quote
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):
Code: [Select]
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
Code: [Select]
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
Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 10:54 am
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:

Code: [Select]
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:

Code: [Select]
#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 :

Code: [Select]
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
Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 11:56 am
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

Title: Re: bug in the timealarm library
Post by: viknet on Jun 29, 2011, 12:21 pm
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
Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 01:32 pm
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
Title: Re: bug in the timealarm library
Post by: viknet on Jun 29, 2011, 02:21 pm
mem,
concerning your proposal for the
Code: [Select]
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
Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 02:57 pm
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:
Code: [Select]
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:

Code: [Select]
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;
  }
}


Title: Re: bug in the timealarm library
Post by: draythomp on Jun 29, 2011, 09:11 pm
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:
Code: [Select]
#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);
 
}



Title: Re: bug in the timealarm library
Post by: draythomp on Jun 29, 2011, 09:12 pm
TimeAlarms.h
Code: [Select]

#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 */


Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 09:23 pm
Thanks draythomp , I will make that addition to the next release
Title: Re: bug in the timealarm library
Post by: draythomp on Jun 29, 2011, 09:25 pm
and  pieces of TimeAlarms.cpp, It wouldn't all fit in the allowed size:
Code: [Select]

// 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;
  }
}



Title: Re: bug in the timealarm library
Post by: draythomp on Jun 29, 2011, 09:29 pm
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.
Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 09:47 pm
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

Title: Re: bug in the timealarm library
Post by: draythomp on Jun 29, 2011, 10:04 pm
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.
Title: Re: bug in the timealarm library
Post by: mem on Jun 29, 2011, 11:08 pm
Quote
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;


Can you post an example of how you use the count method?
Title: Re: bug in the timealarm library
Post by: viknet on Jun 29, 2011, 11:52 pm
OK I made the corrections:

in time.h add these macro
Code: [Select]
#define previousSunday(_time_)  ((( _time_ / SECS_PER_DAY) * SECS_PER_DAY)  - (dayOfWeek(_time_)-1)*SECS_PER_DAY)  // time at the start of the given week
#define nextSunday(_time_) ( previousSunday(time)+SECS_PER_WEEK)  // time at the end of the given week


in timealarms.cpp
Code: [Select]
void AlarmClass::updateNextTrigger()
{
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time )   // update alarm if next trigger is not yet in the future
   {
       // value is less than a day for daily alarm
       // value is between a day and a week+day for a weekly alarm
       // value is more than SECS_PER_WEEK+SECS_PER_DAY to represent an alarm in the future
       // in this case value is a LOT more than SECS_PER_WEEK+SECS_PER_DAY
       // beware you cannot set an alarm for alarm in 1970 (you should'nt need unless you are Marty McFly)
       //
       // All this issues may need to be addressed using
       // a second private field instead of relaying on the value
       //
     
     if( value > (SECS_PER_YEAR) ) // is the value a specific date and time in the future
       {
         nextTrigger = value;  // yes, trigger on this value  
       }
     else    // this value is only an hour or a weekday and an hour
     {
       if ( value <= SECS_PER_DAY) //if the value is a daily alarm
       {
         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  
         }
       }
       else //else the value is a weekly alarm (or monthly if somebody want to code this)
       {
         if ( (value - SECS_PER_DAY) <= SECS_PER_WEEK )
         {
           if( value - SECS_PER_DAY + previousSunday(now()) <= time)
           {
             nextTrigger = value - SECS_PER_DAY + nextSunday(time); // if day has passed then set for the next week. because weekly start at  SECS_PER_DAY  withdraw this amount
           }
           else
           {
             nextTrigger = value - SECS_PER_DAY + previousSunday(time);  // set the date to this week today and add the time given in value
           }
         }
         else
         {
           Mode.isEnabled = 0; // values less than a year but neither a daily nor a weekly alarm
         }
       }
     }
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;  // add the value to previous time (this ensures delay always at least Value seconds)
   }
 }
 else
 {
   Mode.isEnabled = 0;  // Disable if the value is 0
 }
}


and

Code: [Select]
AlarmID_t TimeAlarmsClass::alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
  return create( (DOW * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );  
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
  return create( (DOW * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT );      
}



the code has been unit tested and is working AFAIK


Concerning the count function, I have to agree with mem, can't you loop through all the alarmID, and check the return of alarm.read ?
I tested this code with success.
Code: [Select]
int count()
{
 int c=0;
for (int i=0;i<dtNBR_ALARMS;i++)
 if (Alarm.read(i)>=2) c++;
 return c;
}


the >=2 is because the output of an unset alarm seems to be 0 at start and 01 after an alarm has been deleted (this part is a bit strange for me)


Title: Re: bug in the timealarm library
Post by: draythomp on Jun 30, 2011, 12:28 am
Mem,

heh, It's not much, just something on the order of,

If (Alarm.count == dtNBR_ALARMS){
   don't set the alarm.
}

I then do something else.  In one case, where I really need the particular alarm, I set a flag to try again later (without using an alarm).  I also use the count to tell me if I need to up the max number of alarms the next time I compile.  An example of this is a controller I have for various functions around the house.  Things like turn off the A/C recirculators, guarantee a pump is shut off before going to bed, that kind of thing.  I get the count of alarms active, save the max over time and display it.  This way when I get close to the compiled max, I bump the max, recompile and put the device back in service.  It's not an elegant, sophisticated solution, but it keeps me from running out of timers.  On this device I have dtNBR_ALARMS  set to 20 and at run time I max out around 14 over a couple of days operation. 

See, when you dynamically create alarms to handle something like retrying a failed communication (such as Pachube) or time a response from a remote device that may be slow responding (my neighbors chicken feeder that may fail because a chicken is asleep on it), it's easy to loose track of how many are possible at a given instant.   Additionally, it may not be possible to actually know the max used because seemingly random hardware events happen.  Yes, I could possibly count up all the instances of creation and set the max for that number, but that is wasteful of the limited memory on the Arduino.  The twenty I already have eat up 240 bytes and I'm not close to being done with the eventual capabilities of the device and I must have 25 or thirty timerOnce calls scattered around in the code doing various things.

Obviously, it would be nice to be able to allocate a new timer whenever the number hit the max, but that would likely require malloc and I'm reasonably convinced that would introduce unexpected problems over the long term on a little device.  If you want to see the controller where I display the timer count it's at this page (http://draythomp.blogspot.com/p/house-controller.html).  Scroll down a little and there is a short video that illustrates what happens.

And, reviewing the thread you pointed out, I disagree with Dave Mellis, returning the alarm id is such a tiny complication that it wouldn't be worth mentioning.  At least IMHO.

viknet,

Yes, I could use the ID like you illustrate, but suppose the ID isn't just an index into the array?  In some libraries the token is a pointer, offset into a buffer, structure or something even more exotic.  Relying on what it appears to be (index) seems a little scary.


Title: Re: bug in the timealarm library
Post by: viknet on Jun 30, 2011, 01:38 am
It is an index no more than that no pointer or fluffy logic :-)


when you look at mem's code, it is that simply:

Code: [Select]
for(uint8_t servicedAlarmId = 0; servicedAlarmId < dtNBR_ALARMS; servicedAlarmId++)
{
.......
}


by the way, I probably fucked up somewher in my previous code, while tring to use mems new getTriggeredAlarmId It look's like some functions are called too many times, I go to bed for now, and will try to understand if it is a bug or not.

See you later
Title: Re: bug in the timealarm library
Post by: mem on Jun 30, 2011, 05:16 am
Draythomp,  if the primary usage of count is to indicate if it is possible to set another alarm then I think there is a better way to do this.

If you check the return value from the create methods you can determine  if you have exceeded the capacity. The value will be dtINVALID_ALARM_ID if  dtNBR_ALARMS is not large enough to cope with the number of alarms you want to set.
I think this is simpler and more robust than trying to keep track of the allocation count as you get positive confirmation of the success or failure of the alarm creation.
I agree with your point that dynamic allocation using malloc would introduce unexpected problems that would be unacceptable in the Arduino environment.  So given that the maximum number of timers is fixed at compile time, I believe that reporting the success or failure of each allocation provides the required information about capacity without the need for a function to check the current allocation count.
If the create function returns without error then you are good to go, if it returns dtINVALID_ALARM_ID then you would do the same thing as you would if you had called a method to check if the alarm count was equal to dtNBR_ALARMS

so instead of:

If (Alarm.count == dtNBR_ALARMS){
   //don't set the alarm
   Serial.println("Unable to create alarm!")
}
else{
   Alarm.timerOnce(10, OnceOnlyTask); 
}

you can do this:

   if(Alarm.timerOnce(10, OnceOnlyTask) ) ==  dtINVALID_ALARM_ID){
      Serial.println("Unable to create alarm!")     
   }


Michael
Title: Re: bug in the timealarm library
Post by: mem on Jun 30, 2011, 05:36 am
Viknet, I see that you have extended the functionality in updateNextTrigger to allow for recurring alarms at intervals greater than a week. This complicates the testing and unless there is a strong case for adding this functionality I suggest we keep things simpler by minimising changes to those necessary to fix the recurring DOW alarm issue.
Title: Re: bug in the timealarm library
Post by: draythomp on Jun 30, 2011, 06:15 am
Mem, you didn't look closely enough at my response.  To try and create an alarm to see if you can create an alarm just uses up alarms if you're going to check to see if you're getting close to the limit; especially if you're going to have to let it expire to get rid of it.  Since it's a compile time limit, and there is no way to extend it, monitoring in advance makes it more robust.

But, it doesn't really matter.  Leave the count function out if it in some way conflicts with some philosophical point I don't understand.  I am perfectly capable of putting it back in the library I use having already done so once.  That's one of the good things about open source.
Title: Re: bug in the timealarm library
Post by: mem on Jun 30, 2011, 07:13 am
I did look at your response but I don't think most people would see an advantage in testing if the code was close to the limit. But as you say, its open source so if the count function is useful in your code then enjoy.
Title: Re: bug in the timealarm library
Post by: viknet on Jun 30, 2011, 09:45 am

Viknet, I see that you have extended the functionality in updateNextTrigger to allow for recurring alarms at intervals greater than a week. This complicates the testing and unless there is a strong case for adding this functionality I suggest we keep things simpler by minimising changes to those necessary to fix the recurring DOW alarm issue.


there is no strong case for this, but I made the change because I did not really understood your code.

see below the original code with correct indentation, usefull braces (for me at least) and two additional comments:

Code: [Select]
void AlarmClass::updateNextTrigger()
{
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time )   // update alarm if next trigger is not yet in the future
   {
     if( value > SECS_PER_WEEK )
     { // is the value a specific data and time in the future
       nextTrigger = value;  // yes, trigger on this value
     }
     else //VIK: value <= SECS_PER_WEEK
     {
       if ( value <= SECS_PER_DAY)
       {
         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
         }
       }
       else //VIK: value > SECS_PER_DAY
       {
         if ( value <= SECS_PER_WEEK)
         {
           nextTrigger = value + previousMidnight(time); // set the date to today and add the time given in value
         }
         else
       {
           Mode.isEnabled = 0; // values more than a year but less than today have expired so the alarm is disabled
         }
       }
     }
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;  // add the value to previous time (this ensures delay always at least Value seconds)
   }
 }
 else
 {
   Mode.isEnabled = 0;  // Disable if the value is 0
 }
}



It took me a strong time to understand this line
Code: [Select]
Mode.isEnabled = 0; // values more than a year but less than today have expired so the alarm is disabled
(I am sure something is wrong because this line is dead code and your comment does not match)
And that's why I made some changes.

I can go back and replace SECS_PER_YEAR by SECS_PER_DAY+SECS_PER_WEEK but the previous line code would became dead code again (I have nothing against this and we can go this way to limit testing, but we shall change the comment to explain that this code should never be run)


regards

Viknet
Title: Re: bug in the timealarm library
Post by: mem on Jun 30, 2011, 12:06 pm

Code: [Select]
Mode.isEnabled = 0; // values more than a year but less than today have expired so the alarm is disabled

Sorry about the confusion , there is a typo in the comment. It should say:
// values more than a week but less than today have expired so the alarm is disabled

It catches times that are less then one year but more than one week - this handles the error case for alarms set in 1970 after midnight on Jan 1.

The revised structure you posted looks functionally similar to the version in the playground so this would be a good basis to add your fix for DOW if you are happy to do that.
Title: Re: bug in the timealarm library
Post by: viknet on Jun 30, 2011, 07:59 pm
sorry mem, not that I want to argue with you, but I think you missed my point :-)
I continue to think that the
Code: [Select]
Mode.isEnabled = 0; // values more than a week but less than today have expired so the alarm is disabled

will never be run (and that the previous 'if' statement is useless) could you check the simplification of the code made below ?

 
Code: [Select]
   if( value > SECS_PER_WEEK )
     { ... }
     else //VIK: value <= SECS_PER_WEEK for all the command within the else statement (MARKED as an X)
  X   {
  X     if ( value <= SECS_PER_DAY)
  X     { ... }
  X     else //VIK: value > SECS_PER_DAY
  X     {
  X       if ( value <= SECS_PER_WEEK)  //VIK: this condition is always true and useless (IMHO)
  X       {  ...  }
  X     }
  X   }


Best regards.

P.S.

I will upgrade the code in the playground once we sort this out :-)
Title: Re: bug in the timealarm library
Post by: mem on Jun 30, 2011, 08:59 pm
vikent, we are not arguing, we are clarifying  ;)

My post was referring to the   Mode.isEnabled = 0 code at the end of the method. As you say, the earlier test for value <= SECS_PER_WEEK is redundant.

Please post your revised code here in this thread so it can be reviewed before going live. I will update the playground with these changes (along with the new method to get the alarm id) when we are sure that the fix hasn't broken any of the other functionality.

Many thanks for working in this, I will note your contribution in the code when I update it in the playground.

I hope draythomp is still around so he can try it out. I think he wins the prize for sketchs with the most alarms.


Michael


Title: Re: bug in the timealarm library
Post by: viknet on Jul 01, 2011, 12:24 am
hello, mem,
do you agree with this code :

Code: [Select]
void AlarmClass::updateNextTrigger()
{  
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time )   // update alarm if next trigger is not yet in the future
   {
       // value is less than a day for daily alarm
       // value is between a day and a week+day for a weekly alarm
       // value is more than SECS_PER_WEEK+SECS_PER_DAY to represent an alarm in the future
       // in this case value is a LOT more than SECS_PER_WEEK+SECS_PER_DAY
       // beware you cannot set an alarm for the first 8 days in 1970 (you should'nt need unless you are Marty McFly)
       //
       // All this issues may need to be addressed using
       // a second private field instead of relaying on the value
       //
     
     if( value > (SECS_PER_WEEK +SECS_PER_DAY) ) // is the value a specific date and time in the future
       {
         nextTrigger = value;  // yes, trigger on this value  
       }
     else    // this value is only an hour or a weekday and an hour
     {
       if ( value <= SECS_PER_DAY) //if the value is a daily alarm
       {
         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  
         }
       }
       else //else the value is a weekly alarm
       {
         if( (value - SECS_PER_DAY + previousSunday(now())) <= time)
         {
           nextTrigger = value - SECS_PER_DAY + nextSunday(time); // if day has passed then set for the next week. because weekly start at  SECS_PER_DAY  withdraw this amount
         }
         else
         {
           nextTrigger = value - SECS_PER_DAY + previousSunday(time);  // set the date to this week today and add the time given in value
         }
       }
     }
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;  // add the value to previous time (this ensures delay always at least Value seconds)
   }
 }
 else
 {
   Mode.isEnabled = 0;  // Disable if the value is 0
 }
}


I also find a small but not so easy to correct bug:

the previousSunday(_time_) macro is not safe when used with time beeing the first few days of 1970. this is not a big issue because this time are not usefull,
but sometimes you might initialized your alarm before setting the correct time and in this case the bug is triggered and the nexttrigger is undefined.

I don't know how to nicely correct the issue, in the previousSunday(_time_) we could add a "max(old macro,0 )" to be sure it never goes beyond 0.
I have done it and it is not working work probably because time is an unsigned integer.
so I have changed my macro to :
Code: [Select]
#define previousSunday(_time_)  ((( max(_time_,SECS_PER_WEEK) / SECS_PER_DAY) * SECS_PER_DAY)  - (dayOfWeek(_time_)-1)*SECS_PER_DAY
It is much more failsafe but a bit cumbersome :-)

I also tested your AlarmID_t TimeAlarmsClass::getTriggeredAlarmId() and it is working as advertised :-)

So we are probably ready to update the playground now that the argument is over  ;)

regards

Viknet (Vincent Valdy in real life if you want to quote my contribution)
Title: Re: bug in the timealarm library
Post by: mem on Jul 01, 2011, 10:52 am
Vincent, thanks for that.

I wonder why your test for weekly alarms is SECS_PER_WEEK +SECS_PER_DAY and not  just SECS_PER_WEEK?

I simplified the previousSunday macro by using the existing elapsedSecsThisWeek macro in Time.h
Code: [Select]

#define previousSunday(_time_)  (_time_ - elapsedSecsThisWeek(_time_))  // time at the start of the week for the given time
#define nextSunday(_time_) ( previousSunday(time)+SECS_PER_WEEK)  // time at the end of the week for the given time


I think we can address potential macro underflow problems in documentation. I will add the following to the FAQ:
Q: Can the clock and alarms be set for times in the past?
A: The algorithm to handle alarms works for times after Jan 1 1971 (i.e. at least one year after time 0)

Here is the updateNextTrigger method for testing, I have added some private macros to determine the alarm type that should make the code easier to enhance in future. This adds some redundancy in the tests but is easier to read and enhance. I have not tested these, feel free to fix them if necessary.
Code: [Select]
//**************************************************************
//* Private Methods

// private macros to determine the alarm type
// these may be replaced with the use of a private field if more alarm types are needed
#define isDailyAlarm    (value <= SECS_PER_DAY) //  value is less than a day for daily alarm
#define isWeeklyAlarm   (value > SECS_PER_DAY && value <= SECS_PER_WEEK)  // more than a day and up to one week
#define isExplicitAlarm (value > SECS_PER_WEEK) // alarm is at the given time and date      

void AlarmClass::updateNextTrigger()
{  
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time )   // update alarm if next trigger is not yet in the future
   {      
     if(isExplicitAlarm ) // is the value a specific date and time in the future
     {
       nextTrigger = value;  // yes, trigger on this value  
     }
     else if(isDailyAlarm)  //if this is a daily alarm
     {
       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  
       }
     }
     else if(isWeeklyAlarm)  // if this is a weekly alarm
     {
       if( (value - SECS_PER_DAY + previousSunday(now())) <= time)
       {
         nextTrigger = value - SECS_PER_DAY + nextSunday(time); // if day has passed then set for the next week. because weekly start at  SECS_PER_DAY  withdraw this amount
       }
       else
       {
         nextTrigger = value - SECS_PER_DAY + previousSunday(time);  // set the date to this week today and add the time given in value
       }
     }
     else  // its none of the above (a time that is
     {
       Mode.isEnabled = 0;  // Disable the alarm
     }  
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;  // add the value to previous time (this ensures delay always at least Value seconds)
   }
 }
 else
 {
   Mode.isEnabled = 0;  // Disable if the value is 0
 }
}


Title: Re: bug in the timealarm library
Post by: viknet on Jul 01, 2011, 11:32 am
hello mem,
Quote
I wonder why your test for weekly alarms is SECS_PER_WEEK +SECS_PER_DAY and not  just SECS_PER_WEEK?


day does not start from 0 but from 1 so the highest "value" of a valid weekly alarm is dowSaturday*SECS_PER_DAY+AlarmHMS(23,59,59)
dowSaturday*SECS_PER_DAY=SECS_PER_WEEK and AlarmHMS(23,59,59) is SECS_PER_DAY.

This would not work if day were starting at 0, because for value between 0 and  SECS_PER_DAY you would'nt know if it was a daily alarm or a weekly sunday alarm.

I will check your macro this evening, concerning the underflow issue, the problem is that it is quite common to start sketch and assign a weekly repeating alarm before having the time correctly set (through serial in my case) so you have to manually issue during setup a setTime (SECS_PER_YEAR) for exemple to start in 1971, I did that before correcting the macro but it is not "end-programmer" friendly.
if you don't modify the macro nor issue the setTime the the nextrigger is wrongly calculated way way in the future and the alarm is never triggered (or will be but in many many years)


regards

Viknet
Title: Re: bug in the timealarm library
Post by: mem on Jul 01, 2011, 01:15 pm
I forgot that I changed the start of week to day 1, the first version of this library started the week at day 0.

Bear in mind that the library defines time_t  as unsigned, so the max function may not be the right solution to underflow.  Setting alarms before setting the time creates problems other than just the underflow so one solution is to modify the create method so it will only create recurring day/week alarms if the current time has been set to a value  on or after Jan 1 1971. This assumes its better to prevent an alarm than create one that is likely to go off on the wrong time or day because the clock was set after the alarm was created.

The duration Timers could ignore this check as they are not susceptible to underflow. 

Thoughts?

Michael
Title: Re: bug in the timealarm library
Post by: viknet on Jul 01, 2011, 02:56 pm
Quote
Bear in mind that the library defines time_t  as unsigned, so the max function may not be the right solution to underflow

I know, that was my first approach, but it did not work, so I move the max to
max(_time_,SECS_PER_WEEK), this is always more than SECS_PER_WEEK so within the calculation a negative value could never append.
it just not really accurate because it give the next sunday instead of the previous during the first 3 days of 1970 (because the fourth day is a sunday and the macro become accurate again).
Any weekly alarm for the (thursday friday or saturday) set before the fourth of january 1970 will only trigger the next week everything else is OK

beware that
Code: [Select]
#define elapsedSecsThisWeek(_time_)  (elapsedSecsToday(_time_) +  (dayOfWeek(_time_) * SECS_PER_DAY) )   

Is not accurate, because DayOfWeek Starts at 1. the correct macro needs to be:
Code: [Select]
#define elapsedSecsThisWeek(_time_)  (elapsedSecsToday(_time_) +  ((dayOfWeek(_time_)-1) * SECS_PER_DAY) )   

you could then define
Code: [Select]
#define previousSunday(_time_)  (max(_time_,SECS_PER_WEEK) - elapsedSecsThisWeek(_time_))

Concerning the idea of checking time is after Jan 1 1971 it is also very acceptable.

Best regards

Viknet
Title: Re: bug in the timealarm library
Post by: mem on Jul 01, 2011, 09:48 pm
I have not had a chance to run these but here is the revised code for testing. I hope draythomp  and others will give this a good workout.

Here is the TimeAlarms.h file:

Code: [Select]

#ifndef TimeAlarms_h
#define TimeAlarms_h
//  TimeAlarms.h - Arduino Time alarms header for use with Time library 

#include <inttypes.h>

#include "Time.h"

#define dtNBR_ALARMS 6

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);

  // 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
  AlarmID_t getTriggeredAlarmId();  //returns the currently triggered  alarm id
};

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 */








Title: Re: bug in the timealarm library
Post by: mem on Jul 01, 2011, 09:49 pm
The Time.h File:
Code: [Select]
/*
 time.h - low level time and date functions
*/

#ifndef _Time_h
#define _Time_h

#include <inttypes.h>

typedef unsigned long time_t;

typedef enum {timeNotSet, timeNeedsSync, timeSet
}  timeStatus_t ;

typedef enum {
   dowInvalid, dowSunday, dowMonday, dowTuesday, dowWednesday, dowThursday, dowFriday, dowSaturday
} timeDayOfWeek_t;

typedef enum {
   tmSecond, tmMinute, tmHour, tmWday, tmDay,tmMonth, tmYear, tmNbrFields
} tmByteFields;  

typedef struct  {
 uint8_t Second;
 uint8_t Minute;
 uint8_t Hour;
 uint8_t Wday;   // day of week, sunday is day 1
 uint8_t Day;
 uint8_t Month;
 uint8_t Year;   // offset from 1970;
} tmElements_t, TimeElements, *tmElementsPtr_t;

//convenience macros to convert to and from tm years
#define  tmYearToCalendar(Y) ((Y) + 1970)  // full four digit year
#define  CalendarYrToTm(Y)   ((Y) - 1970)
#define  tmYearToY2k(Y)      ((Y) - 30)    // offset is from 2000
#define  y2kYearToTm(Y)      ((Y) + 30)  

typedef time_t(*getExternalTime)();
//typedef void  (*setExternalTime)(const time_t); // not used in this version


/*==============================================================================*/
/* Useful Constants */
#define SECS_PER_MIN  (60UL)
#define SECS_PER_HOUR (3600UL)
#define SECS_PER_DAY  (SECS_PER_HOUR * 24UL)
#define DAYS_PER_WEEK (7UL)
#define SECS_PER_WEEK (SECS_PER_DAY * DAYS_PER_WEEK)
#define SECS_PER_YEAR (SECS_PER_WEEK * 52UL)
#define SECS_YR_2000  (946684800UL) // the time at the start of y2k

/* Useful Macros for getting elapsed time */
#define numberOfSeconds(_time_) (_time_ % SECS_PER_MIN)  
#define numberOfMinutes(_time_) ((_time_ / SECS_PER_MIN) % SECS_PER_MIN)
#define numberOfHours(_time_) (( _time_% SECS_PER_DAY) / SECS_PER_HOUR)
#define dayOfWeek(_time_)  ((( _time_ / SECS_PER_DAY + 4)  % DAYS_PER_WEEK)+1) // 1 = Sunday
#define elapsedDays(_time_) ( _time_ / SECS_PER_DAY)  // this is number of days since Jan 1 1970
#define elapsedSecsToday(_time_)  (_time_ % SECS_PER_DAY)   // the number of seconds since last midnight
// The following macros are used in calculating alarms and assume the clock will not underflow
// Always set the correct time before settting alarms
#define previousMidnight(_time_) (( _time_ / SECS_PER_DAY) * SECS_PER_DAY)  // time at the start of the given day
#define nextMidnight(_time_) ( previousMidnight(_time_)  + SECS_PER_DAY ) // time at the end of the given day
#define elapsedSecsThisWeek(_time_)  (elapsedSecsToday(_time_) +  ((dayOfWeek(_time_)-1) * SECS_PER_DAY) )   // note that week starts on day 1
#define previousSunday(_time_)  (_time_ - elapsedSecsThisWeek(_time_))  // time at the start of the week for the given time
#define nextSunday(_time_) ( previousSunday(time)+SECS_PER_WEEK)  // time at the end of the week for the given time


/* Useful Macros for converting elapsed time to a time_t */
#define minutesToTime_t ((M)) ( (M) * SECS_PER_MIN)  
#define hoursToTime_t   ((H)) ( (H) * SECS_PER_HOUR)  
#define daysToTime_t    ((H)) ( (D) * SECS_PER_DAY)
#define weeksToTime_t   ((W)) ( (W) * SECS_PER_WEEK)  

/*============================================================================*/
/*  time and date functions   */
int     hour();            // the hour now
int     hour(time_t t);    // the hour for the given time
int     hourFormat12();    // the hour now in 12 hour format
int     hourFormat12(time_t t); // the hour for the given time in 12 hour format
uint8_t isAM();            // returns true if time now is AM
uint8_t isAM(time_t t);    // returns true the given time is AM
uint8_t isPM();            // returns true if time now is PM
uint8_t isPM(time_t t);    // returns true the given time is PM
int     minute();          // the minute now
int     minute(time_t t);  // the minute for the given time
int     second();          // the second now
int     second(time_t t);  // the second for the given time
int     day();             // the day now
int     day(time_t t);     // the day for the given time
int     weekday();         // the weekday now (Sunday is day 1)
int     weekday(time_t t); // the weekday for the given time
int     month();           // the month now  (Jan is month 1)
int     month(time_t t);   // the month for the given time
int     year();            // the full four digit year: (2009, 2010 etc)
int     year(time_t t);    // the year for the given time

time_t now();              // return the current time as seconds since Jan 1 1970
void    setTime(time_t t);
void    setTime(int hr,int min,int sec,int day, int month, int yr);
void    adjustTime(long adjustment);

/* date strings */
#define dt_MAX_STRING_LEN 9 // length of longest date string (excluding terminating null)
char* monthStr(uint8_t month);
char* dayStr(uint8_t day);
char* monthShortStr(uint8_t month);
char* dayShortStr(uint8_t day);

/* time sync functions */
timeStatus_t timeStatus(); // indicates if time has been set and recently synchronized
void    setSyncProvider( getExternalTime getTimeFunction); // identify the external time provider
void    setSyncInterval(time_t interval); // set the number of seconds between re-sync

/* low level functions to convert to and from system time                     */
void breakTime(time_t time, tmElements_t &tm);  // break time_t into elements
time_t makeTime(tmElements_t &tm);  // convert time elements into time_t


#endif /* _Time_h */



the TimeAlarms.cpp file is in the next post
Title: Re: bug in the timealarm library
Post by: mem on Jul 01, 2011, 09:59 pm
the TimeAlarms.cpp file:
Code: [Select]

//  TimeAlarms.cpp - Arduino Time alarms for use with Time library  
//  1 July 2011 - added fix for weekly alarms contributed by Vincent Valdy

extern "C" {
#include <string.h> // for memset
}

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

#define IS_ONESHOT  true
#define IS_REPEAT   false
#define IS_ALARM    true
#define IS_TIMER    false

AlarmClass::AlarmClass()
{
 Mode.isAlarm =  Mode.isEnabled = Mode.isOneShot = 0;
 value = nextTrigger = 0;
 onTickHandler = NULL;  
}


// private macros to determine the alarm type
// these may be replaced with the use of a private field if more alarm types are needed
#define isDailyAlarm    (value <= SECS_PER_DAY) //  value is less than a day for daily alarm
#define isWeeklyAlarm   (value > SECS_PER_DAY && value <= SECS_PER_WEEK+SECS_PER_DAY )  // more than a day and up to one week
#define isExplicitAlarm (value > SECS_PER_WEEK) // alarm is at the given time and date      

void AlarmClass::updateNextTrigger()
{  
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time )   // update alarm if next trigger is not yet in the future
   {      
     if(isExplicitAlarm ) // is the value a specific date and time in the future
     {
       nextTrigger = value;  // yes, trigger on this value  
     }
     else if(isDailyAlarm)  //if this is a daily alarm
     {
       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  
       }
     }
     else if(isWeeklyAlarm)  // if this is a weekly alarm
     {
       if( (value - SECS_PER_DAY + previousSunday(now())) <= time)
       {
         nextTrigger = value - SECS_PER_DAY + nextSunday(time);
       }
       else
       {
         nextTrigger = value - SECS_PER_DAY + previousSunday(time);  
       }
     }
     else  // its none of the above (a time that is
     {
       Mode.isEnabled = 0;  // Disable the alarm
     }  
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;  
   }
 }
 else
 {
   Mode.isEnabled = 0;  // Disable if the value is 0
 }
}

//**************************************************************
//* Time Alarms Public Methods

TimeAlarmsClass::TimeAlarmsClass()
{
 isServicing = false;
 for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
    Alarm[id].Mode.isAllocated = false;
}

AlarmID_t TimeAlarmsClass::alarmOnce(time_t value, OnTick_t onTickHandler){   // trigger once at the given time of day
  return create( value, onTickHandler, IS_ALARM, IS_ONESHOT );
}

AlarmID_t TimeAlarmsClass::alarmOnce(const int H,  const int M,  const int S,OnTick_t onTickHandler){  
  return create( AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );
}

AlarmID_t TimeAlarmsClass::alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
  unsigned long dayOffset =  ( 7 + DOW - dayOfWeek(now())) %7;
  return create( (dayOffset * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );  
}
 
AlarmID_t TimeAlarmsClass::alarmRepeat(time_t value, OnTick_t onTickHandler){
    return create( value, onTickHandler, IS_ALARM, IS_REPEAT );
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler){
    return create( AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT );
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
  unsigned long dayOffset =  ( 7 + DOW - dayOfWeek(now())) %7;
  return create( (dayOffset * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT );      
}
 
AlarmID_t TimeAlarmsClass::timerOnce(time_t value, OnTick_t onTickHandler){  
    return create( value, onTickHandler, IS_TIMER, IS_ONESHOT );
}

AlarmID_t TimeAlarmsClass::timerOnce(const int H,  const int M,  const int S, OnTick_t onTickHandler){   // As above with HMS arguments
 return create( AlarmHMS(H,M,S), onTickHandler, IS_TIMER, IS_ONESHOT );
}
 
AlarmID_t TimeAlarmsClass::timerRepeat(time_t value, OnTick_t onTickHandler){ // trigger after the given number of seconds continuously
    return create( value, onTickHandler, IS_TIMER, IS_REPEAT);
}

AlarmID_t TimeAlarmsClass::timerRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler){
    return create( AlarmHMS(H,M,S), onTickHandler, IS_TIMER, IS_REPEAT);
}

void TimeAlarmsClass::enable(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
   Alarm[ID].Mode.isEnabled = (Alarm[ID].value != 0) && (Alarm[ID].onTickHandler != 0) ;  
   Alarm[ID].updateNextTrigger(); // trigger is updated whenever  this is called, even if already enabled
 }
}

void TimeAlarmsClass::disable(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated)
   Alarm[ID].Mode.isEnabled = false;
}

// write the given value to the given alarm
void TimeAlarmsClass::write(AlarmID_t ID, time_t value)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
   Alarm[ID].value = value;
   enable(ID);
 }
}

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

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
}

// following functions are not Alarm ID specific.
void TimeAlarmsClass::delay(unsigned long ms)
{
 unsigned long start = millis();
 while( millis() - start  <= ms)
   serviceAlarms();
}

void TimeAlarmsClass::waitForDigits( uint8_t Digits, dtUnits_t Units)
{
 while(Digits != getDigitsNow(Units) )
 {
   serviceAlarms();
 }
}

void TimeAlarmsClass::waitForRollover( dtUnits_t Units)
{
 while(getDigitsNow(Units) == 0  ) // if its just rolled over than wait for another rollover                            
   serviceAlarms();
 waitForDigits(0, Units);
}

uint8_t TimeAlarmsClass::getDigitsNow( dtUnits_t Units)
{
 time_t time = now();
 if(Units == dtSecond) return numberOfSeconds(time);
 if(Units == dtMinute) return numberOfMinutes(time);
 if(Units == dtHour) return numberOfHours(time);
 if(Units == dtDay) return dayOfWeek(time);
 return 255;  // This should never happen
}


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;
 }
}

AlarmID_t TimeAlarmsClass::create( time_t value, OnTick_t onTickHandler,boolean isAlarm, boolean isOneShot, boolean isEnabled ){  // returns true if has been registerd ok
 if( ! (isAlarm && now() < SECS_PER_YEAR)) // only create alarm ids if the time is at least Jan 1 1971
 {  
for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
   {
       if(Alarm[id].Mode.isAllocated == false)
    {
    // here if there is an Alarm id is available
     Alarm[id].Mode.isAllocated = true;
     Alarm[id].onTickHandler = onTickHandler;
     Alarm[id].Mode.isAlarm = isAlarm;
     Alarm[id].Mode.isOneShot = isOneShot;
     Alarm[id].value = value;
     isEnabled ?  enable(id) : disable(id);  
     return id;  // alarm created ok
    }  
   }
 }
 return dtINVALID_ALARM_ID; // no IDs available or time is invalid
}

// make one instance for the user to use
TimeAlarmsClass Alarm = TimeAlarmsClass() ;



Lets see how we get on with testing this

Michael
Title: Re: bug in the timealarm library
Post by: viknet on Jul 01, 2011, 10:29 pm
macro previousSunday and isExplicitAlarm  have errors
previoussunday need to be
Code: [Select]
#define previousSunday(_time_)  (max(_time_,SECS_PER_WEEK) - elapsedSecsThisWeek(_time_))  // time at the start of the week for the given time

(you could remove the max, but in this case keep _time_ not SECS_PER_WEEK) ;)


the isExplicitAlarm macro need to be
Code: [Select]
#define isExplicitAlarm (value > (SECS_PER_WEEK+SECS_PER_DAY)) // alarm is at the given time and date     


sorry but I have spent wayyyyyy to much time with your code during the last days ;)


regards


Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 12:02 am
There are problems.  I incorporated viknet's macros from post right above because the day of week alarms were firing every second.  After they were incorporated, the DOW alarms are firing on the wrong day and apparently doing a call(0) causing the board to reboot on the second try.  I started the tests at 1/1/1971 to avoid the various problems with the first year.

code used:
Code: [Select]
#include <Time.h>
#include <TimeAlarms.h>

AlarmID_t morningAlarmID, afternoonAlarmID, eveningAlarmID;
AlarmID_t tuesdayAlarmID, saturdayAlarmID;
char buf[200];

void setup()
{
  Serial.begin(57600);
  Serial.println("Initializing...");
  setTime(0,0,0,1,1,1971);
  morningAlarmID   = Alarm.alarmRepeat(8,30,0, onAlarm);
  afternoonAlarmID = Alarm.alarmRepeat(13,25,0,onAlarm);
  eveningAlarmID   = Alarm.alarmRepeat(17,45,0,onAlarm);
  tuesdayAlarmID   = Alarm.alarmRepeat(dowTuesday,12,31,0,onAlarm);
//  saturdayAlarmID  = Alarm.alarmRepeat(dowSaturday,1,45,0,onAlarm);
  pinMode(13, OUTPUT);     
  Serial.println("Begin test...");
}

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

  if(id == morningAlarmID) {
    if(hour() != 8 || minute() != 30){
      sprintf(buf,"Morning Alarm Failure: Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),month(),day(),year());
      Serial.println(buf);
    }
  }
  else if (id == afternoonAlarmID) { 
    if(hour() != 13 || minute() != 25){
      sprintf(buf,"Afternoon Alarm Failure: Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),month(),day(),year());
      Serial.println(buf);
    }
  }
  else if (id == eveningAlarmID) { 
    if(hour() != 17 || minute() != 45){
      sprintf(buf,"Evening Alarm Failure: Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),month(),day(),year());
      Serial.println(buf);
    }
  } 
  else if (id == tuesdayAlarmID) { 
    if(hour() != 1 || minute() != 45 || weekday() != dowTuesday){
      sprintf(buf,"Tuesday Alarm Failure: Time %s %d:%d:%d %d/%d/%d", dayStr(day()),hour(),minute(),second(),month(),day(),year());
      Serial.println(buf);
    }
  } 
  else if (id == saturdayAlarmID) { 
    if(hour() != 17 || minute() != 45 || weekday() != dowSaturday){
      sprintf(buf,"Saturday Alarm Failure: Time %s %d:%d:%d %d/%d/%d", dayStr(day()),hour(),minute(),second(),month(),day(),year());
      Serial.println(buf);
    }
  } 
  else { 
    Serial.println("Invalid Alarm ID");
    sprintf(buf,"Time %s %d:%d:%d %d/%d/%d", dayStr(day()),hour(),minute(),second(),month(),day(),year());
    Serial.println(buf);
  }
}

int savedMonth = 0;
int savedHour = 0;

void loop()
{
  if(savedHour != hour()){  //just to let me know it's alive
    savedHour = hour();
    digitalWrite(13, digitalRead(13)==HIGH?LOW:HIGH);   // set the LED on
  }
  if(savedMonth != month()){  // how far it's gotten
    savedMonth = month();
    sprintf(buf,"Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),day(),month(),year());
    Serial.println(buf);
  }

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


results:
Code: [Select]
Initializing...
Begin test...
Time 0:0:0 1/1/1971
Tuesday Alarm Failure: Time Friday 12:31:18 1/6/1971
Initializing...
Begin test...
Time 0:0:0 1/1/1971
Tuesday Alarm Failure: Time Friday 12:31:18 1/6/1971
Initializing...
Begin test...
Time 0:0:0 1/1/1971
Tuesday Alarm Failure: Time Friday 12:31:18 1/6/1971
Initializing...


The daily alarms seem to be working fine.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 02, 2011, 12:17 am
the
Code: [Select]
AlarmID_t TimeAlarmsClass::alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
   unsigned long dayOffset =  ( 7 + DOW - dayOfWeek(now())) %7;
   return create( (dayOffset * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );   
}


is also wrong, for alarmonce and alarmrepeat it should be

Code: [Select]
AlarmID_t TimeAlarmsClass::alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
   return create( (DOW * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );   
}



regards
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 02:23 am
vicnet's latest changes to DOW alarm don't help much.

tuesdayAlarmID   = Alarm.alarmOnce(dowTuesday,12,31,0,onAlarm);

fires on Thursday 12:31:12 1/5/1971  using the same test program shown above.

And alarmRepeat still cause the arduino to reboot, apparently on the second time it triggers.  Is everyone working from the same source?
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 03:16 am
I think I have the source mixed up somehow.  Regrouping.....
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 05:27 am
OK, testing again.  Daily alarms seem to work except for Saturday

the call  - saturdayAlarmID  = Alarm.alarmRepeat(dowSaturday,17,45,0,onAlarm);  will fire every time through the loop.  I have all the latest changes in place.  The next time you post a change, try to put the entire module in the box, my editing can cause errors.

There are a lot more tests to run though.  I haven't tried jumping by hours to see how far it will run and I haven't made sure it doesn't miss any.  Additionally, tests on a future date, like Feb 4, 2013 need to be tested.  However, right now the saturday thing is messing me up.
Title: Re: bug in the timealarm library
Post by: mem on Jul 02, 2011, 08:54 am
Hi draythomp, good to see you back.
I think there is a better way to fix the DOW problem then the approach we have been taking. Using 7 days plus one in weekly calculations just does not feel right. I am looking into  changing the code so that weekly alarms are calculated on periods of 7 days instead of 8 and the adjust for the fact that Sunday is day one by subtracting one from the DOW index on the call to create DOW alarms.

The alarm type macros would then be:
#define isDailyAlarm    (value <= SECS_PER_DAY) //  value is less than a day for daily alarm
#define isWeeklyAlarm   (value > SECS_PER_DAY && value <= SECS_PER_WEEK )  // more than a day and up to one week
#define isExplicitAlarm (value > SECS_PER_WEEK) // alarm is at the given time and date  

The alarm duration macros would be  as in the Time.h  post above
#define elapsedSecsThisWeek(_time_)  (elapsedSecsToday(_time_) +  ((dayOfWeek(_time_)-1) * SECS_PER_DAY)
#define previousSunday(_time_)  (_time_ - elapsedSecsThisWeek(_time_))  
#define nextSunday(_time_) ( previousSunday(time)+SECS_PER_WEEK)  

And the calls to the create methods for alarmOnce and alarmRepeat that take a DOW argument would be modified to use the macros above to subtract one from the DOW index.

Both you and viknet have done a lot of work on this so far so I wanted to get feedback before making these changes.

Michael

edit: on reflection, that approach doesn't work for weekly alarms set for sunday (the values look the same as daily alarms). I am sure we can fix the code along the lines suggested by viknet but perhaps this is the time to replace the implicit calculations for determining the alarm period with an explicit index.  There is room to store 16 index values in the AlarmMode_t structure so it would not consume any more RAM.  Values could be: DAILY_ALARM, WEEKLY_ALARM and EXPLICIT_ALARM_TIME ( Monthly and Annual alarms could be added later if someone really needs these and wants to code them). This would simplify the code in updateNextTrigger but it is  a significant change so I am interested in hearing views on this.
Title: Re: bug in the timealarm library
Post by: mem on Jul 02, 2011, 12:05 pm

Further thoughts on library changes to better support setting alarm periods.

The proposal above uses an explicit variable to hold the alarm period type, this would privately indicate a daily or weekly alarms to the trigger logic and could be extended for biweekly, monthly or annual alarms if needed.
This could be implemented without changing the public API calls but the behaviour of code that calls the create methods would be as follows:

Calls to these existing methods would create daily alarms as now:
  alarmRepeat( H, M, S, onTickHandler);
  alarmOnce(H, M, S, onTickHandler);


Calls to this method also create daily alarms if the given value is not greater than a day.  In the new functionality, values greater than a day would return an error
   alarmRepeat(time_t value,  onTickHndler);

Calls to this method also create a daily alarm if the given value is not greater than a day (for legacy support, the alarmOnce(H,M.S) method is preferred).  In the new functionality values greater than a year will create an alarm at the specific time and date.  Values greater than a day and less than a year would return an error. The general rule will be to set the clock to the current time before seting time based alarms, although the logic will work as long as the clock is set at a time later than Jan1 1971.

   alarmOnce(time_t value,  onTickHandler);

Calls to the following existing methods create alarms that trigger at the given day of week and time as now:
  alarmRepeat(timeDayOfWeek_t  DOW,  H, M, S, onTickHandler);
  alarmOnce( timeDayOfWeek_t   DOW, H, M, S, onTickHandler);

In the new functionality these would be the only way to create weekly alarms.

In summary, all timer and daily alarm functionality should remain the same.  The method using time_t  argument for setting day of week alarms would no longer be supported (it will return an error).
The create method will only create time of day or weekly alarms if the  clock time is on or after  Jan 1 1971. (duration timers will work as now even if the clock is not set)

Does anyone see a problem with any of this?
Title: Re: bug in the timealarm library
Post by: viknet on Jul 02, 2011, 03:37 pm
draythomp

sorry to ask you this but could you check this, it is working on my side, and instead of asking you to do modification here are the full file in the three following post.


best regards:

time.h:
Code: [Select]
/*
  time.h - low level time and date functions
*/

#ifndef _Time_h
#define _Time_h

#include <inttypes.h>

typedef unsigned long time_t;

typedef enum {timeNotSet, timeNeedsSync, timeSet
}  timeStatus_t ;

typedef enum {
    dowInvalid, dowSunday, dowMonday, dowTuesday, dowWednesday, dowThursday, dowFriday, dowSaturday
} timeDayOfWeek_t;

typedef enum {
    tmSecond, tmMinute, tmHour, tmWday, tmDay,tmMonth, tmYear, tmNbrFields
} tmByteFields;    

typedef struct  {
  uint8_t Second;
  uint8_t Minute;
  uint8_t Hour;
  uint8_t Wday;   // day of week, sunday is day 1
  uint8_t Day;
  uint8_t Month;
  uint8_t Year;   // offset from 1970;
} tmElements_t, TimeElements, *tmElementsPtr_t;

//convenience macros to convert to and from tm years
#define  tmYearToCalendar(Y) ((Y) + 1970)  // full four digit year
#define  CalendarYrToTm(Y)   ((Y) - 1970)
#define  tmYearToY2k(Y)      ((Y) - 30)    // offset is from 2000
#define  y2kYearToTm(Y)      ((Y) + 30)   

typedef time_t(*getExternalTime)();
//typedef void  (*setExternalTime)(const time_t); // not used in this version


/*==============================================================================*/
/* Useful Constants */
#define SECS_PER_MIN  (60UL)
#define SECS_PER_HOUR (3600UL)
#define SECS_PER_DAY  (SECS_PER_HOUR * 24UL)
#define DAYS_PER_WEEK (7UL)
#define SECS_PER_WEEK (SECS_PER_DAY * DAYS_PER_WEEK)
#define SECS_PER_YEAR (SECS_PER_WEEK * 52UL)
#define SECS_YR_2000  (946684800UL) // the time at the start of y2k

/* Useful Macros for getting elapsed time */
#define numberOfSeconds(_time_) (_time_ % SECS_PER_MIN) 
#define numberOfMinutes(_time_) ((_time_ / SECS_PER_MIN) % SECS_PER_MIN)
#define numberOfHours(_time_) (( _time_% SECS_PER_DAY) / SECS_PER_HOUR)
#define dayOfWeek(_time_)  ((( _time_ / SECS_PER_DAY + 4)  % DAYS_PER_WEEK)+1) // 1 = Sunday
#define elapsedDays(_time_) ( _time_ / SECS_PER_DAY)  // this is number of days since Jan 1 1970
#define elapsedSecsToday(_time_)  (_time_ % SECS_PER_DAY)   // the number of seconds since last midnight
#define previousMidnight(_time_) (( _time_ / SECS_PER_DAY) * SECS_PER_DAY)  // time at the start of the given day
#define previousSunday(_time_)  ((( max(_time_,SECS_PER_WEEK) / SECS_PER_DAY) * SECS_PER_DAY)  - (dayOfWeek(_time_)-1)*SECS_PER_DAY)  // time at the start of the given week
#define nextSunday(_time_) ( previousSunday(time)+SECS_PER_WEEK)  // time at the end of the given week
#define nextMidnight(_time_) ( previousMidnight(_time_)  + SECS_PER_DAY ) // time at the end of the given day
#define elapsedSecsThisWeek(_time_)  (elapsedSecsToday(_time_) +  (dayOfWeek(_time_) * SECS_PER_DAY) )   

/* Useful Macros for converting elapsed time to a time_t */
#define minutesToTime_t ((M)) ( (M) * SECS_PER_MIN) 
#define hoursToTime_t   ((H)) ( (H) * SECS_PER_HOUR) 
#define daysToTime_t    ((H)) ( (D) * SECS_PER_DAY)
#define weeksToTime_t   ((W)) ( (W) * SECS_PER_WEEK)   

/*============================================================================*/
/*  time and date functions   */
int     hour();            // the hour now
int     hour(time_t t);    // the hour for the given time
int     hourFormat12();    // the hour now in 12 hour format
int     hourFormat12(time_t t); // the hour for the given time in 12 hour format
uint8_t isAM();            // returns true if time now is AM
uint8_t isAM(time_t t);    // returns true the given time is AM
uint8_t isPM();            // returns true if time now is PM
uint8_t isPM(time_t t);    // returns true the given time is PM
int     minute();          // the minute now
int     minute(time_t t);  // the minute for the given time
int     second();          // the second now
int     second(time_t t);  // the second for the given time
int     day();             // the day now
int     day(time_t t);     // the day for the given time
int     weekday();         // the weekday now (Sunday is day 1)
int     weekday(time_t t); // the weekday for the given time
int     month();           // the month now  (Jan is month 1)
int     month(time_t t);   // the month for the given time
int     year();            // the full four digit year: (2009, 2010 etc)
int     year(time_t t);    // the year for the given time

time_t now();              // return the current time as seconds since Jan 1 1970
void    setTime(time_t t);
void    setTime(int hr,int min,int sec,int day, int month, int yr);
void    adjustTime(long adjustment);

/* date strings */
#define dt_MAX_STRING_LEN 9 // length of longest date string (excluding terminating null)
char* monthStr(uint8_t month);
char* dayStr(uint8_t day);
char* monthShortStr(uint8_t month);
char* dayShortStr(uint8_t day);

/* time sync functions */
timeStatus_t timeStatus(); // indicates if time has been set and recently synchronized
void    setSyncProvider( getExternalTime getTimeFunction); // identify the external time provider
void    setSyncInterval(time_t interval); // set the number of seconds between re-sync

/* low level functions to convert to and from system time                     */
void breakTime(time_t time, tmElements_t &tm);  // break time_t into elements
time_t makeTime(tmElements_t &tm);  // convert time elements into time_t


#endif /* _Time_h */

Title: Re: bug in the timealarm library
Post by: viknet on Jul 02, 2011, 03:38 pm
TimeAlarms.h
Code: [Select]

#ifndef TimeAlarms_h
#define TimeAlarms_h

#include <inttypes.h>

#include "Time.h"

#define dtNBR_ALARMS 8

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);

  // low level methods
  void enable(AlarmID_t ID);
  void disable(AlarmID_t ID);
  void reset(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
  AlarmID_t  getTriggeredAlarmId();  //returns the currently triggered  alarm id
                                      // returns  dtINVALID_ALARM_ID if not invoked from within an alarm handler
};

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 */

Title: Re: bug in the timealarm library
Post by: viknet on Jul 02, 2011, 03:43 pm
TimeAlarms.cpp

Code: [Select]

//(c) Michael Margolis
//This is free software under the terms of the GNU LGPL

extern "C" {
#include <string.h> // for memset
}

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

#define IS_ONESHOT  true
#define IS_REPEAT   false
#define IS_ALARM    true
#define IS_TIMER    false


//* Alarm Class Constructor

AlarmClass::AlarmClass()
{
 Mode.isAlarm =  Mode.isEnabled = Mode.isOneShot = 0;
 value = nextTrigger = 0;
 onTickHandler = NULL; // prevent a callback until this pointer is explicitly set
}


//* Private Methods
#define isExplicitAlarm (value > (SECS_PER_WEEK+SECS_PER_DAY)) // alarm is at the given time and date
#define isDailyAlarm    (value <= SECS_PER_DAY) //  value is less than a day for daily alarm
#define isWeeklyAlarm   ((value > SECS_PER_DAY) && (value <= (SECS_PER_WEEK+SECS_PER_DAY)) )  // more than a day and up to one week
 
void AlarmClass::updateNextTrigger()
{  
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time ) // update alarm if next trigger is not yet in the future
   {
     if( isExplicitAlarm ) // is the value a specific date and time in the future
     {
       nextTrigger = value;// yes, trigger on this value  
     }
     else  // this value is only an hour or a weekday and an hour
     {
       if ( isDailyAlarm) //if the value is a daily alarm
       {
         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  
         }
       }
       else if(isWeeklyAlarm)  // if this is a weekly alarm
       {
         if( (value - SECS_PER_DAY + previousSunday(now())) <= time)
         {
           nextTrigger = value - SECS_PER_DAY + nextSunday(time); // if day has passed then set for the next week. because weekly start at  SECS_PER_DAY  withdraw this amount
         }
         else
         {
           nextTrigger = value - SECS_PER_DAY + previousSunday(time);  // set the date to this week today and add the time given in value
         }
       }
       else
       {
         Mode.isEnabled = 0;  // Disable the alarm
       }
     }
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;  // add the value to previous time (this ensures delay always at least Value seconds)
   }
 }
 else
 {
   Mode.isEnabled = 0;  // Disable if the value is 0
 }
}

//* Time Alarms Public Methods

TimeAlarmsClass::TimeAlarmsClass()
{
 isServicing = false;
 for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
    Alarm[id].Mode.isAllocated = false;  // ensure  all Alarms are avialable for allocation  
}

AlarmID_t TimeAlarmsClass::alarmOnce(time_t value, OnTick_t onTickHandler){   // trigger once at the given time of day
  return create( value, onTickHandler, IS_ALARM, IS_ONESHOT );
}

AlarmID_t TimeAlarmsClass::alarmOnce(const int H,  const int M,  const int S,OnTick_t onTickHandler){   // as above with HMS arguments
  return create( AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );
}
 
AlarmID_t TimeAlarmsClass::alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
  return create( (DOW * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT );  
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){  // as above, with day of week
  return create( (DOW * SECS_PER_DAY) + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT );      
}
 
AlarmID_t TimeAlarmsClass::alarmRepeat(time_t value, OnTick_t onTickHandler){ // trigger daily at the given time
    return create( value, onTickHandler, IS_ALARM, IS_REPEAT );
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler){ // as above with HMS arguments
    return create( AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT );
}



AlarmID_t TimeAlarmsClass::timerOnce(time_t value, OnTick_t onTickHandler){   // trigger once after the given number of seconds
    return create( value, onTickHandler, IS_TIMER, IS_ONESHOT );
}

AlarmID_t TimeAlarmsClass::timerOnce(const int H,  const int M,  const int S, OnTick_t onTickHandler){   // As above with HMS arguments
 return create( AlarmHMS(H,M,S), onTickHandler, IS_TIMER, IS_ONESHOT );
}
 
AlarmID_t TimeAlarmsClass::timerRepeat(time_t value, OnTick_t onTickHandler){ // trigger after the given number of seconds continuously
    return create( value, onTickHandler, IS_TIMER, IS_REPEAT);
}

AlarmID_t TimeAlarmsClass::timerRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler){ // trigger after the given number of seconds continuously
    return create( AlarmHMS(H,M,S), onTickHandler, IS_TIMER, IS_REPEAT);
}

void TimeAlarmsClass::enable(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
   Alarm[ID].Mode.isEnabled = (Alarm[ID].value != 0) && (Alarm[ID].onTickHandler != 0) ;  // only enable if value is non zero and a tick handler has been set
   Alarm[ID].updateNextTrigger(); // trigger is updated whenever  this is called, even if already enabled
 }
}

void TimeAlarmsClass::disable(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated)
   Alarm[ID].Mode.isEnabled = false;
}

void TimeAlarmsClass::reset(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated)
 {
   Alarm[ID].Mode.isAllocated = false;
   Alarm[ID].onTickHandler = 0;
   Alarm[ID].value = 0;
   Alarm[ID].nextTrigger = 0;
 }
}

// write the given value to the given alarm
void TimeAlarmsClass::write(AlarmID_t ID, time_t value)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
   Alarm[ID].value = value;
   enable(ID);
 }
}

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

// following functions are not Alarm ID specific.
void TimeAlarmsClass::delay(unsigned long ms)
{
 unsigned long start = millis();
 while( millis() - start  <= ms)
   serviceAlarms();
}

void TimeAlarmsClass::waitForDigits( uint8_t Digits, dtUnits_t Units)
{
 while(Digits != getDigitsNow(Units) )
 {
   serviceAlarms();
 }
}

void TimeAlarmsClass::waitForRollover( dtUnits_t Units)
{
 while(getDigitsNow(Units) == 0 ) // if its just rolled over than wait for another rollover
   serviceAlarms();
 waitForDigits(0, Units);
}

uint8_t TimeAlarmsClass::getDigitsNow( dtUnits_t Units)
{
 time_t time = now();
 if(Units == dtSecond) return numberOfSeconds(time);
 if(Units == dtMinute) return numberOfMinutes(time);
 if(Units == dtHour) return numberOfHours(time);
 if(Units == dtDay) return dayOfWeek(time);
 return 255;  // This should never happen
}


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
 }
}


//***********************************************************
//* Private Methods

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  
       }
       else      Serial.println("null handler !!!");
     }
   }
   isServicing = false;
 }
}

AlarmID_t TimeAlarmsClass::create( time_t value, OnTick_t onTickHandler,boolean isAlarm, boolean isOneShot, boolean isEnabled )
{  // returns true if has been registerd ok
 for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
 {
   if(Alarm[id].Mode.isAllocated == false)
   {
     // here if there is an Alarm id is available
   Alarm[id].Mode.isAllocated = true;
     Alarm[id].onTickHandler = onTickHandler;
     Alarm[id].Mode.isAlarm = isAlarm;
     Alarm[id].Mode.isOneShot = isOneShot;
     Alarm[id].value = value;
     isEnabled ?  enable(id) : disable(id);  
   return id;
 }  
 }
 return dtINVALID_ALARM_ID; // no IDs available
}

// make one instance for the user to use
TimeAlarmsClass Alarm = TimeAlarmsClass() ;

Title: Re: bug in the timealarm library
Post by: mem on Jul 02, 2011, 04:23 pm
To avoid spinning our wheels on this, I would appreciate a response to post #46 and #47. These changes would make the code in posts #48-#50 redundant.
I value fresh ideas and open discussion on this library but want to avoid creating new issues when we fix the old ones.  Using max to prevent overflow still can result in an alarm set at the wrong time. Indeed, any time of day alarms created before the clock is set is likely to result in alarms at the wrong times.

I would appreciate a response to the proposal in posts 46 and 47 before we spend more time testing. The issue raised in this thread has existed for 18 months or so I don't think we need to rush a fix through.

Michael
Title: Re: bug in the timealarm library
Post by: viknet on Jul 02, 2011, 05:02 pm
mem,
sorry but I did not saw your edit on post 46 so I did not really understood the next one I prefered to post my lib as this to enable test by draythomp.

Concerning your proposal

Quote
I am sure we can fix the code along the lines suggested by viknet but perhaps this is the time to replace the implicit calculations for determining the alarm period with an explicit index.  There is room to store 16 index values in the AlarmMode_t structure so it would not consume any more RAM.  Values could be: DAILY_ALARM, WEEKLY_ALARM and EXPLICIT_ALARM_TIME ( Monthly and Annual alarms could be added later if someone really needs these and wants to code them). This would simplify the code in updateNextTrigger but it is  a significant change so I am interested in hearing views on this.


I fully agree with this, I thought I was going to do it I spoke about it in another thread:
http://arduino.cc/forum/index.php/topic,63583.15.html
but in order not to break I prefer to first correct the issue with weekly alarm because everything is already done for this (and prefectly working on my side)


following your second post I agree with you but there is the need to expose an alarm.create method that would work for all alarm type (whatever the new parameter we have to supply)

your proposal would also need another alarm.read() method that would reply the value and the frequency (without breaking the old one)

that is a lot of work and test, and I am not sure we have the manpower/testing power/utility for this,

you will also see that I add an alarm.reset() method to fully disable an alarm (this is needed for alarm management).

The project I am working on need alarm to be added/set/read/destroyed using the serial port and that may explain a bit more my needs.

best regards

Viknet
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 05:22 pm
vicnet, I'll be glad to give your pieces a try.  Get to it in a bit.

mem, first, let me bring up that the usefulness of daily alarms is huge.  I'm not sure why people aren't using the heck out of them and why this particular problem hasn't cropped up before except in my case where I reset the daily alarm each time the previous one triggered.  But household timers: alarm clocks, thermostats, pool controllers, chicken feeders, light controllers, etc all want daily alarms.  So, given that, I think a small change of philosophy is a good thing.  

And, frankly, it doesn't make much sense to me to set a daily alarm in any fashion other than alarmRepeat( everyDarnMonday, 10:00AM,etc)
because that reads well in the code except in the instances where the daily time is calculated.  In my experience there are extremely few instances where a daily time is calculated, sensors for sunrise, sunset, ph, drainage valves etc, prevail.

Net, having to use the calls with the day of week to set a daily alarm is not a problem, and not being able to use time_t shouldn't bother anyone.  I can't imagine how one would use that reasonably anyway.

Also, I don't see a problem having to set the time before using the alarms.  It's one silly call with easy to type parameters and one already has to have the time library included.  No big deal.
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 05:34 pm
vicnet, your modules update worked, at least so far.  Saturday no longer fires every time through the loop.  I'll run some tests now that include saturday and such to see how it goes.
Title: Re: bug in the timealarm library
Post by: mem on Jul 02, 2011, 05:59 pm
thanks draythomp,


viknet, If the code you posted above is working for you then by all means use it but I am not yet ready to update the playground with this because it does not fully address underflow and does not address potential issues with setting alarms before the clock is set.

I realize that neither of these is likely to be an issue that most people will come across, but the bug I introduced when I added the weekly alarms indicates to me that that the trigger algorithm I implemented in the current version does not make it easy to add more alarm types.

The public alarm creation methods do not need to be changed to support the current alarm types.  The private create method would take an additional argument for the alarm period enumeration.

The read method is not documented in the playground or in the distributed readme file so its only used by experienced developers. The existing read method would still return the value but if developers are using the value returned to infer the alarm period then it would be better  if an explicit method was provided.

I had not noticed that you had added a method to free up an alarm. This could be added if useful although I would name it clear or free rather than reset.
I have already coded the changes needed to implement the proposal in #46 and #47 so the only work is in testing.  If we think it useful to fully address the underflow and  'clock not yet set' issues as well as make the code easier to enhance then would it not be best to do this now?

Michael
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 06:34 pm
I want my count function.  Of course, it doesn't need to be called count, but for me and others that will do something similar I think it will be useful.

C'mon, trying to set a timer and having it fail because you have dynamic code that can't track of how many are used kinda sucks.  if you create a timer and it returns BAD, what do you do?  Hang up in a loop until one comes free to the exclusion of draining the septic tank?  The count function could be tied to a bell that rings because you added one too many timers and overdid it.  It would also be useful to measure how loaded your device is becoming.

With the clear, reset, whatever routine one could keep assigning timers until one ran out and then clear them all out to get the count, but that's the only way outside the library to tell how many are currently being used.  That seems a little silly when it is so easy to do in the library.  Looking at the index value doesn't meet with the idea of libraries controlling their data, otherwise, once could just get the id and start poking at the alarm itself.

How many of us have implemented something to tell us how close we've gotten to the end of memory because our arrays and such have put us a hundred bytes from oblivion.  Heck, if you're using streams you can run out and have exactly zero indication that there's a problem except for the burst of garbage on the serial output as you over write the stack.

If vicnet hadn't put in the free, he would have wanted a count, especially if he couldn't use the id as an index indicator.

You see, I envision a controller with sixty or seventy timers or alarms.  They check on devices periodically, take actions, then retry in a few seconds if it didn't happen, measure ph in the pool and time HCl injectors over a period of days, pan cameras toward the neighbor lady that sunbathes in the afternoon on Tuesdays when the sun is out.  It would be very bad if that alarm failed because it ran out.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 02, 2011, 06:58 pm
draythomp ;)

why did'nt you tried the following code ?

Code: [Select]
int count()
{
  int c=0;
for (int i=0;i<dtNBR_ALARMS;i++)
  if (Alarm.read(i)>=2) c++;
  return c;
}


AFAIK it is perfectly correct, I know it is usefull to know how many slot are left and this is working well, that's the reason why I see no use of putting this inside the library code ;)
Title: Re: bug in the timealarm library
Post by: mem on Jul 02, 2011, 07:08 pm
draythomp, although I find it difficult to imagine an application that is allocating and freeing so many timers, I certainly don't want to be the one that causes your septic tank to overflow because you couldn't get one more  timer.

If you really are unable to determine the maximum number of timers you need until the system is running then I can see why you want the count.  Am I right in thinking that it would be better to return the  number of free timers rather than the count of used timers. Obviously you can derive one from the other using dtNBR_ALARMS,  but would you prefer count to return the number used or free?

Michael
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 08:00 pm
That's actually a pretty good question, but like you said one is easy from the other.  So, it would be a teeny bit better to return the remaining number IMHO.  This is not a hard and fast recommendation at all, just a guess.  Perhaps it would be better to name it something like countersleft, remaining, not used or something.  Count implies used and blows my argument about using the remaining number.

flip a coin???

Just to help you understand how I and one neighbor use the timers dynamically.  He has a drip watering system that senses the moisture level in the soil a couple of inches below grade.  When it gets dry enough, he turns on the drips, but he wants to limit how long they run because he doesn't want to drain the water tank and have the wives (I share the tank, he's not Mormon) mad because they can't make iced tea.  So he turns on the drip and sets a timer for 1 hour from now to turn it off then sets another timer to check tomorrow for dryness.  You can't sample the tank because it's buried up to the very top to keep it from getting hot.  Maybe an ultra sonic from the lid, but that's another project.  But meanwhile, the little arduino is doing a ton of other stuff of a similar nature.  The ability to set a timer then just forget about it until it fires leaving the arduino free to do something else makes really cool control systems easy.  There's hardware out there that is so complex that you really can't predict what it will do next so, you check it; an alarm that fires daily and then decides when to fire again based on the data is really useful.  I don't have a clue why people don't do more of this.  Heck a standing light that turns on when a coyote comes in range of the chicken coop then sets a timer to turn off in five minutes is dynamic, multiply that times one light at each corner and you start to see something of the situation.  Of course, you want the lights independent, it confused the coyotes more.

Oh, so far, the testing has been working perfectly.  I still have a few things to try out, but it has stepped through a few years of daily alarms with them all firing at the right time.  I have not checked to see if any of them have missed, just that when they fire they fire on time.  More later.  And don't worry, if you change something and it needs to be tried, I'll crank up the little piece of code I'm using and do it again.

Besides, I have a vested interest in this because I use this library a lot for drip watering, pool control, blinking lights, etc.  The arduino makes an incredible timer when you need one. 
Title: Re: bug in the timealarm library
Post by: mem on Jul 02, 2011, 08:59 pm
draythomp and viknet, do you have test sketches that you can post here that exercise the alarms, particularly the daily and weekly alarms?


Michael
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 02, 2011, 09:24 pm
I threw this one together and modify it as necessary to try various items.

Code: [Select]
#include <Time.h>
#include <TimeAlarms.h>

AlarmID_t morningAlarmID=99, afternoonAlarmID=99, eveningAlarmID=99;
AlarmID_t tuesdayAlarmID=99, wednesdayAlarmID=99, thursdayAlarmID=99,
          fridayAlarmID=99, saturdayAlarmID=99, sundayAlarmID=99;
char buf[100];

void setup()
{
  Serial.begin(57600);
  Serial.println("Initializing...");
  delay(2000);
  setTime(0,0,0,1,1,1971);
//  morningAlarmID   = Alarm.alarmRepeat(8,30,0, onAlarm);
//  afternoonAlarmID = Alarm.alarmRepeat(13,25,0,onAlarm);
//  eveningAlarmID   = Alarm.alarmRepeat(17,45,0,onAlarm);
//  tuesdayAlarmID   = Alarm.alarmRepeat(dowTuesday,17,45,0,onAlarm);
//  wednesdayAlarmID = Alarm.alarmRepeat(dowWednesday,17,45,0,onAlarm);
//  thursdayAlarmID = Alarm.alarmRepeat(dowThursday,17,45,0,onAlarm);
//  fridayAlarmID    = Alarm.alarmRepeat(dowFriday,17,45,0,onAlarm);
//  sundayAlarmID    = Alarm.alarmRepeat(dowSunday,17,45,0,onAlarm);
  saturdayAlarmID  = Alarm.alarmRepeat(dowSaturday,17,45,0,onAlarm);
  pinMode(13, OUTPUT);     
  Serial.println("Begin test...");
}

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

  if (id == tuesdayAlarmID){
    if(weekday(tnow) != dowTuesday || hour(tnow) != 17 || minute(tnow) != 45){
      showMe("Tuesday error", tnow);
    }
    else{
//     showMe("Tuesday OK", tnow);
    }
  }
  if (id == wednesdayAlarmID){
    if(weekday(tnow) != dowWednesday || hour(tnow) != 17 || minute(tnow) != 45){
//     showMe("Wednesday error", tnow);
    }
     else{
//      showMe("Wednesday OK", tnow);
    }
}
  if (id == thursdayAlarmID){
    if(weekday(tnow) != dowThursday || hour(tnow) != 17 || minute(tnow) != 45){
      showMe("Thursday error", tnow);
    }
    else{
//      showMe("Thursday OK", tnow);
    }
  }
  if (id == fridayAlarmID){
     if(weekday(tnow) != dowFriday || hour(tnow) != 17 || minute(tnow) != 45){
      showMe("Friday error", tnow);
    }
    else{
//     showMe("Friday OK", tnow);
    }
  }
  if (id == saturdayAlarmID){
    if(weekday(tnow) != dowSaturday || hour(tnow) != 17 || minute(tnow) != 45){
      showMe("Saturday error", tnow);
    }
    else{
      showMe("Saturday OK", tnow);
    }
  }
  if (id == sundayAlarmID){
    if(weekday(tnow) != dowSunday || hour(tnow) != 17 || minute(tnow) != 45){
      showMe("Sunday error", tnow);
    }
    else{
//      showMe("Sunday OK", tnow);
    }
  }
  if (id >= dtNBR_ALARMS){
    showMe("Creepy ID", tnow);
  }
  if (id == dtINVALID_ALARM_ID) {
    showMe("Invalid Alarm ID", tnow);
  }
}

void showMe(char* type, time_t tnow){
  sprintf(buf,"%s alarm %s, %d:%d:%d %d/%d/%d",type,dayStr(weekday(tnow)),
       hour(tnow),minute(tnow),second(tnow),month(tnow),day(tnow),year(tnow));
  Serial.println(buf);
}


int savedMonth = 0;
int savedDay = 0;

void loop()
{
  if(savedDay != day()){  //just to let me know it's alive
    savedDay = day();
    digitalWrite(13, digitalRead(13)==HIGH?LOW:HIGH);   // flip the led
  }
  if(savedMonth != month()){  // how far it's gotten
    savedMonth = month();
    sprintf(buf,"Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),day(),month(),year());
    Serial.println(buf);
  }

  Alarm.delay(0);
  adjustTime(55);
//  sprintf(buf,"Time %d:%d:%d %d/%d/%d", hour(),minute(),second(),day(),month(),year());
//  Serial.println(buf);
 
}
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 03, 2011, 09:06 am
OK, I have no life.   I admit it.  However, I put this little bit of code in the test sketch and am running it now.  I'll let this go on and off until something else is ready to test.  I think I can modify this to work with timers too and add them in as well.  An extended run of random timers and alarms with vicnet's trick to speed up time should beat the heck out of the code.  Sure, it isn't perfect, but it'll test it more than a room full of monkeys and typewriters (do they still exist?  typewriters that is).

Code: [Select]
if (id == randomAlarmID){
    if(weekday(tnow) != rday || hour(tnow) != rhour ||
         (rmin-1 > minute(tnow) && minute(tnow) > rmin+1)){
      sprintf(buf,"Random Failure, parameters %d, %d, %d, %d ",
              (timeDayOfWeek_t)rday, rhour, rmin, rsec);
      Serial.println(buf);
      showMe("Failed at", tnow);
    }
    else {
     showMe("Random", tnow);
    }
    Alarm.reset(id);           //reset it so I can make it again
    rday = random(1,8);
    rhour = random(0,24);
    rmin = random(0,60);
    rsec = random(0,60);
    randomAlarmID = Alarm.alarmRepeat((timeDayOfWeek_t)rday,rhour,rmin,rsec,onAlarm);
  }
Title: Re: bug in the timealarm library
Post by: viknet on Jul 03, 2011, 10:25 am
Do we need to feed your test program with banana ?  :smiley-mr-green:

it is a very good idea to test like you did, I hope it will work and I am very happy you used the reset alarm :-)

regards

Viknet
Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 11:05 am
That random test is useful.

I added a global errorCount variable that is incremented if an error is detected and modified showMe to print this :
Code: [Select]

void showMe(char* type, time_t tnow){
  sprintf(buf,"%s alarm %s, %d:%d:%d %d/%d/%d errors=%ld",type,dayStr(weekday(tnow)),
       hour(tnow),minute(tnow),second(tnow),month(tnow),day(tnow),year(tnow), errorCount);
  Serial.println(buf);
}


I am running the new library code that uses enumerations for the alarm types and it is looking good so far. I will leave it running for  a while and see if any errors crop up. If all is well I should be able to post the new code here later today. I have included a method to return the number of allocated ids and a method to free a given alarm.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 03, 2011, 11:13 am
by the way, if some of you are using serial to debug, I have these usefull function

Code: [Select]

#define DEBUG 1

#include <avr/pgmspace.h>

void printDigits(int digits)
{
 // utility function for digital clock display: prints preceding colon and leading 0
 Serial.print(':');
 if(digits < 10)
   Serial.print('0');
 Serial.print(digits);
}

void digitalClockDisplay()
{
 // digital clock display of the time
 Serial.print(' ');
 Serial.print(hour());
 printDigits(minute());
 printDigits(second());
 Serial.print(' ');
 Serial.print(dayStr(weekday()));
 Serial.print(" the ");
 Serial.print(day());
 Serial.print(' ');
 Serial.print(month());
 Serial.print(' ');
 Serial.print(year());
}


void showString (PGM_P s)
{
 char c;
 while ((c = pgm_read_byte(s++)) != 0)
   Serial.print(c);
}

#ifdef DEBUG
#define DEBUG_STAT_P(x) Serial.print('['); \
digitalClockDisplay(); \
Serial.print(':'); \
Serial.print(__FUNCTION__); \
Serial.print(':'); \
Serial.print(__LINE__); \
showString(PSTR("] ")); \
showString(PSTR(x)); \
Serial.println();

#define DEBUG_DYN_P(x,y) Serial.print('['); \
digitalClockDisplay(); \
Serial.print(':'); \
Serial.print(__FUNCTION__); \
Serial.print(':'); \
Serial.print(__LINE__); \
showString(PSTR("] ")); \
showString(PSTR(x)); \
Serial.println(y);

#else
#define DEBUG_STAT_P(x)
#define DEBUG_DYN_P(x,y)
#endif



you can then use
DEBUG_STAT_P("some static string");
DEBUG_DYNP_P("some static string",dynamic_variable);

the advantages is that any debug string take very little memory (SRAM) space and that you can disable all debug easily
Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 11:53 am
re debugging, I like to use printf.  This  consumes  just under  5% of the RAM on a 328 and I find it very convenient.

If you think it useful, save  the following to a file named printf.h in a directory named printf beneath the libraries directory.
Code: [Select]
// printf

extern "C" {
#include <inttypes.h>
#include <stdio.h>
}
static char _buf[100];
#define printf(...) \
do { \
sprintf(_buf, __VA_ARGS__); Serial.write(_buf); \
} while (0))


You can then do this:

Code: [Select]
#include <Time.h>
#include <TimeAlarms.h>
#include <printf.h>

void setup()
{
  Serial.begin(57600);
  printf("Initializing...\n");
  delay(2000);
  setTime(0,0,0,1,1,1971); 
  printf("Begin test, count= %d\n", Alarm.count());
  // digital clock display of the time
  printf("%d:%d:%d %d/%d/%d", hour(),minute(),second(),month(),day(),year() );

rest of code goes here ...

Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 06:23 pm
The tests on the new version are going well.
The random alarm creation was very handy although I changed the method for incrementing the clock so it did not skip over an alarm trigger.
Here is the code I used in loop:

Code: [Select]
  Alarm.delay(0);
  time_t nextTrigger = Alarm.getNextTrigger();
  if(nextTrigger > now() + 3) 
  {
     setTime(nextTrigger - 2);
     printf(" Set Time to %s %d:%d:%d %d/%d/%d\n",dayStr(weekday(now())), hour(),minute(),second(),day(),month(),year()); 
  }


nextTrigger is a new method that can be exposed for this kind of testing:

Code: [Select]

// returns the absolute time of the next scheduled alarm, or 0 if none
time_t TimeAlarmsClass::getNextTrigger()
{
time_t nextTrigger = 0xffffffff;  // the max time value

    for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
    {
      if(Alarm[id].Mode.isAllocated )
      {
          if(Alarm[id].nextTrigger <  nextTrigger)
             nextTrigger = Alarm[id].nextTrigger;
      }     
    }
    return nextTrigger == 0xffffffff ? 0 : nextTrigger; 
}


If the tests continue to go well I will  post the revised library source later today.
Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 07:06 pm
My tests of the new code are looking good.
One arduino is running weekly alarms on Saturday and a random day and time - it gone a simulated 10 years without reporting an error
The other test is running the TimeAlarmExample sketch that uses timers set for every 30 minutes and time of day alarms for 8:30 and 17:45. This test has triggered 50 alarms a day for a simulated month.

Here is the Time.h file

Code: [Select]
/*
  time.h - low level time and date functions
*/

#ifndef _Time_h
#define _Time_h

#include <inttypes.h>

typedef unsigned long time_t;

typedef enum {timeNotSet, timeNeedsSync, timeSet
}  timeStatus_t ;

typedef enum {
    dowInvalid, dowSunday, dowMonday, dowTuesday, dowWednesday, dowThursday, dowFriday, dowSaturday
} timeDayOfWeek_t;

typedef enum {
    tmSecond, tmMinute, tmHour, tmWday, tmDay,tmMonth, tmYear, tmNbrFields
} tmByteFields;    

typedef struct  {
  uint8_t Second;
  uint8_t Minute;
  uint8_t Hour;
  uint8_t Wday;   // day of week, sunday is day 1
  uint8_t Day;
  uint8_t Month;
  uint8_t Year;   // offset from 1970;
} tmElements_t, TimeElements, *tmElementsPtr_t;

//convenience macros to convert to and from tm years
#define  tmYearToCalendar(Y) ((Y) + 1970)  // full four digit year
#define  CalendarYrToTm(Y)   ((Y) - 1970)
#define  tmYearToY2k(Y)      ((Y) - 30)    // offset is from 2000
#define  y2kYearToTm(Y)      ((Y) + 30)   

typedef time_t(*getExternalTime)();
//typedef void  (*setExternalTime)(const time_t); // not used in this version


/*==============================================================================*/
/* Useful Constants */
#define SECS_PER_MIN  (60UL)
#define SECS_PER_HOUR (3600UL)
#define SECS_PER_DAY  (SECS_PER_HOUR * 24UL)
#define DAYS_PER_WEEK (7UL)
#define SECS_PER_WEEK (SECS_PER_DAY * DAYS_PER_WEEK)
#define SECS_PER_YEAR (SECS_PER_WEEK * 52UL)
#define SECS_YR_2000  (946684800UL) // the time at the start of y2k

/* Useful Macros for getting elapsed time */
#define numberOfSeconds(_time_) (_time_ % SECS_PER_MIN) 
#define numberOfMinutes(_time_) ((_time_ / SECS_PER_MIN) % SECS_PER_MIN)
#define numberOfHours(_time_) (( _time_% SECS_PER_DAY) / SECS_PER_HOUR)
#define dayOfWeek(_time_)  ((( _time_ / SECS_PER_DAY + 4)  % DAYS_PER_WEEK)+1) // 1 = Sunday
#define elapsedDays(_time_) ( _time_ / SECS_PER_DAY)  // this is number of days since Jan 1 1970
#define elapsedSecsToday(_time_)  (_time_ % SECS_PER_DAY)   // the number of seconds since last midnight
// The following macros are used in calculating alarms and assume the clock is set to a date later than Jan 1 1971
// Always set the correct time before settting alarms
#define previousMidnight(_time_) (( _time_ / SECS_PER_DAY) * SECS_PER_DAY)  // time at the start of the given day
#define nextMidnight(_time_) ( previousMidnight(_time_)  + SECS_PER_DAY )   // time at the end of the given day
#define elapsedSecsThisWeek(_time_)  (elapsedSecsToday(_time_) +  ((dayOfWeek(_time_)-1) * SECS_PER_DAY) )   // note that week starts on day 1
#define previousSunday(_time_)  (_time_ - elapsedSecsThisWeek(_time_))      // time at the start of the week for the given time
#define nextSunday(_time_) ( previousSunday(_time_)+SECS_PER_WEEK)          // time at the end of the week for the given time


/* Useful Macros for converting elapsed time to a time_t */
#define minutesToTime_t ((M)) ( (M) * SECS_PER_MIN) 
#define hoursToTime_t   ((H)) ( (H) * SECS_PER_HOUR) 
#define daysToTime_t    ((H)) ( (H) * SECS_PER_DAY) // fixed on Jul 3 2011
#define weeksToTime_t   ((W)) ( (W) * SECS_PER_WEEK)   

/*============================================================================*/
/*  time and date functions   */
int     hour();            // the hour now
int     hour(time_t t);    // the hour for the given time
int     hourFormat12();    // the hour now in 12 hour format
int     hourFormat12(time_t t); // the hour for the given time in 12 hour format
uint8_t isAM();            // returns true if time now is AM
uint8_t isAM(time_t t);    // returns true the given time is AM
uint8_t isPM();            // returns true if time now is PM
uint8_t isPM(time_t t);    // returns true the given time is PM
int     minute();          // the minute now
int     minute(time_t t);  // the minute for the given time
int     second();          // the second now
int     second(time_t t);  // the second for the given time
int     day();             // the day now
int     day(time_t t);     // the day for the given time
int     weekday();         // the weekday now (Sunday is day 1)
int     weekday(time_t t); // the weekday for the given time
int     month();           // the month now  (Jan is month 1)
int     month(time_t t);   // the month for the given time
int     year();            // the full four digit year: (2009, 2010 etc)
int     year(time_t t);    // the year for the given time

time_t now();              // return the current time as seconds since Jan 1 1970
void    setTime(time_t t);
void    setTime(int hr,int min,int sec,int day, int month, int yr);
void    adjustTime(long adjustment);

/* date strings */
#define dt_MAX_STRING_LEN 9 // length of longest date string (excluding terminating null)
char* monthStr(uint8_t month);
char* dayStr(uint8_t day);
char* monthShortStr(uint8_t month);
char* dayShortStr(uint8_t day);

/* time sync functions */
timeStatus_t timeStatus(); // indicates if time has been set and recently synchronized
void    setSyncProvider( getExternalTime getTimeFunction); // identify the external time provider
void    setSyncInterval(time_t interval); // set the number of seconds between re-sync

/* low level functions to convert to and from system time                     */
void breakTime(time_t time, tmElements_t &tm);  // break time_t into elements
time_t makeTime(tmElements_t &tm);  // convert time elements into time_t


#endif /* _Time_h */

Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 07:08 pm
the TimeAlarms.h file:

Code: [Select]
//  TimeAlarms.h - Arduino Time alarms header for use with Time library

#ifndef TimeAlarms_h
#define TimeAlarms_h

#include <inttypes.h>

#include "Time.h"

#define dtNBR_ALARMS 6   // max is 255
#define USE_SPECIALIST_METHODS  // define this for testing

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 
uint8_t alarmType              :3 ;  // enumeration of daily/weekly (in future: biweekly/semimonthly/monthly/annual)
                                     // note that the current API only supports daily or weekly alarm periods
}
    AlarmMode_t   ;

typedef enum  {dtNotAlarm, dtTimer, dtExplicitAlarm, dtDailyAlarm, dtWeeklyAlarm } dtAlarmPeriod_t ; // in future: dtBiweekly, dtMonthly, dtAnnual

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, dtAlarmPeriod_t alarmType, 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);

  // low level methods
  void enable(AlarmID_t ID);
  void disable(AlarmID_t ID);
  AlarmID_t getTriggeredAlarmId();          //returns the currently triggered  alarm id

#ifndef USE_SPECIALIST_METHODS 
private:  // the following methods are for testing and are not documented as part of the standard library
#endif
  void free(AlarmID_t ID);                  // free the id to allow is reause
  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 
  dtAlarmPeriod_t readType(AlarmID_t ID);   // return the alarm type for the given alarm ID
  uint8_t count();                          // returns the number of allocated timers
  time_t getNextTrigger();                  // returns the time of the next scheduled alarm

};

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 */

Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 07:11 pm
TimeAlarms.cpp file (no comments)

Code: [Select]
/*
TimeAlarms.cpp

testing version
*/

extern "C" {
#include <string.h> // for memset
}

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

#define IS_ONESHOT  true
#define IS_REPEAT   false
#define IS_ALARM    true
#define IS_TIMER    false


AlarmClass::AlarmClass()
{
 Mode.isAlarm =  Mode.isEnabled = Mode.isOneShot = 0;
 value = nextTrigger = 0;
 onTickHandler = NULL;  // prevent a callback until this pointer is explicitly set
}

void AlarmClass::updateNextTrigger()
{  
 if( (value != 0) && Mode.isEnabled )
 {
   time_t time = now();
   if(Mode.isAlarm && nextTrigger <= time )  
   {      
     if(Mode.alarmType == dtExplicitAlarm )
     {
       nextTrigger = value;  // yes, trigger on this value  
     }
     else if(Mode.alarmType == dtDailyAlarm)
     {
       if( value + previousMidnight(now()) <= time)
       {
         nextTrigger = value + nextMidnight(time);
       }
       else
       {
         nextTrigger = value + previousMidnight(time);
       }
     }
     else if(Mode.alarmType == dtWeeklyAlarm)
     {
       if( (value + previousSunday(now())) <= time)
       {
         nextTrigger = value + nextSunday(time);
       }
       else
       {
         nextTrigger = value + previousSunday(time);
       }
     }
     else  // its not a recognized alarm type - this should not happen
     {
       Mode.isEnabled = 0;  // Disable the alarm
     }  
   }
   if(Mode.isAlarm == false)
   {
     // its a timer
     nextTrigger = time + value;
   }
 }
 else
 {
   Mode.isEnabled = 0;
 }
}

TimeAlarmsClass::TimeAlarmsClass()
{
 isServicing = false;
 for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
    free(id);
}

// this method will now return an error if the value is greater than one day - use DOW methods for weekly alarms
AlarmID_t TimeAlarmsClass::alarmOnce(time_t value, OnTick_t onTickHandler){
 if( value <= SECS_PER_DAY)
   return create( value, onTickHandler, IS_ALARM, IS_ONESHOT, dtDailyAlarm );
 else
   return dtINVALID_ALARM_ID; // dont't allocate if the time is greater than one day
}

AlarmID_t TimeAlarmsClass::alarmOnce(const int H,  const int M,  const int S,OnTick_t onTickHandler){
  return create( AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT, dtDailyAlarm );
}

AlarmID_t TimeAlarmsClass::alarmOnce(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){
  unsigned long secondsToGivenDay = (DOW-1) * SECS_PER_DAY;
  return create( secondsToGivenDay + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_ONESHOT, dtWeeklyAlarm );
}

// this method will now return an error if the value is greater than one day - use DOW methods for weekly alarms
AlarmID_t TimeAlarmsClass::alarmRepeat(time_t value, OnTick_t onTickHandler){ // trigger daily at the given time
 if( value <= SECS_PER_DAY)
    return create( value, onTickHandler, IS_ALARM, IS_REPEAT, dtDailyAlarm );
 else
    return dtINVALID_ALARM_ID; // dont't allocate if the time is greater than one day
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler){ // as above with HMS arguments
    return create( AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT, dtDailyAlarm );
}

AlarmID_t TimeAlarmsClass::alarmRepeat(const timeDayOfWeek_t DOW, const int H,  const int M,  const int S, OnTick_t onTickHandler){
  unsigned long secondsToGivenDay = (DOW-1) * SECS_PER_DAY;
  return create( secondsToGivenDay + AlarmHMS(H,M,S), onTickHandler, IS_ALARM, IS_REPEAT, dtWeeklyAlarm );
}

AlarmID_t TimeAlarmsClass::timerOnce(time_t value, OnTick_t onTickHandler){
 return create( value, onTickHandler, IS_TIMER, IS_ONESHOT, dtTimer );
}

AlarmID_t TimeAlarmsClass::timerOnce(const int H,  const int M,  const int S, OnTick_t onTickHandler){
 return create( AlarmHMS(H,M,S), onTickHandler, IS_TIMER, IS_ONESHOT, dtTimer );
}

AlarmID_t TimeAlarmsClass::timerRepeat(time_t value, OnTick_t onTickHandler){
 return create( value, onTickHandler, IS_TIMER, IS_REPEAT, dtTimer);
}

AlarmID_t TimeAlarmsClass::timerRepeat(const int H,  const int M,  const int S, OnTick_t onTickHandler){
 return create( AlarmHMS(H,M,S), onTickHandler, IS_TIMER, IS_REPEAT, dtTimer);
}

void TimeAlarmsClass::enable(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
   Alarm[ID].Mode.isEnabled = (Alarm[ID].value != 0) && (Alarm[ID].onTickHandler != 0) ;
   Alarm[ID].updateNextTrigger();
 }
}

void TimeAlarmsClass::disable(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated)
   Alarm[ID].Mode.isEnabled = false;
}

void TimeAlarmsClass::free(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated)
 {
   Alarm[ID].Mode.isEnabled = Alarm[ID].Mode.isAllocated = false;
Alarm[ID].Mode.alarmType = dtNotAlarm;
   Alarm[ID].onTickHandler = 0;
Alarm[ID].value = 0;
Alarm[ID].nextTrigger = 0;  
 }
}

// write the given value to the given alarm
void TimeAlarmsClass::write(AlarmID_t ID, time_t value)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
   Alarm[ID].value = value;
   enable(ID);
 }
}

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

// return the alarm type for the given alarm ID
dtAlarmPeriod_t TimeAlarmsClass::readType(AlarmID_t ID)
{
 if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated)
   return (dtAlarmPeriod_t)Alarm[ID].Mode.alarmType ;
 else
   return dtNotAlarm;
}

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

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
}

void TimeAlarmsClass::delay(unsigned long ms)
{
 unsigned long start = millis();
 while( millis() - start  <= ms)
   serviceAlarms();
}

void TimeAlarmsClass::waitForDigits( uint8_t Digits, dtUnits_t Units)
{
 while(Digits != getDigitsNow(Units) )
 {
   serviceAlarms();
 }
}

void TimeAlarmsClass::waitForRollover( dtUnits_t Units)
{
 while(getDigitsNow(Units) == 0  )
   serviceAlarms();
 waitForDigits(0, Units);
}

uint8_t TimeAlarmsClass::getDigitsNow( dtUnits_t Units)
{
 time_t time = now();
 if(Units == dtSecond) return numberOfSeconds(time);
 if(Units == dtMinute) return numberOfMinutes(time);
 if(Units == dtHour) return numberOfHours(time);
 if(Units == dtDay) return dayOfWeek(time);
 return 255;  // This should never happen
}

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;
       else
          Alarm[servicedAlarmId].updateNextTrigger();
       if( TickHandler != NULL) {
         (*TickHandler)();     // call the handler
       }
     }
   }
   isServicing = false;
 }
}

// returns the absolute time of the next scheduled alarm, or 0 if none
time_t TimeAlarmsClass::getNextTrigger()
{
time_t nextTrigger = 0xffffffff;  // the max time value

   for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
   {
 if(Alarm[id].Mode.isAllocated )
 {
if(Alarm[id].nextTrigger <  nextTrigger)
  nextTrigger = Alarm[id].nextTrigger;
 }
}
return nextTrigger == 0xffffffff ? 0 : nextTrigger;  
}

// attempt to create an alarm and return true if successful
AlarmID_t TimeAlarmsClass::create( time_t value, OnTick_t onTickHandler, uint8_t isAlarm, uint8_t isOneShot, dtAlarmPeriod_t alarmType, uint8_t isEnabled)
{
 if( ! (isAlarm && now() < SECS_PER_YEAR)) // only create alarm ids if the time is at least Jan 1 1971
 {
for(uint8_t id = 0; id < dtNBR_ALARMS; id++)
{
if(Alarm[id].Mode.isAllocated == false)
{
// here if there is an Alarm id is available
 Alarm[id].Mode.isAllocated = true;
 Alarm[id].onTickHandler = onTickHandler;
 Alarm[id].Mode.isAlarm = isAlarm;
 Alarm[id].Mode.isOneShot = isOneShot;
 Alarm[id].Mode.alarmType = alarmType;
 Alarm[id].value = value;
 isEnabled ?  enable(id) : disable(id);
 return id;  // alarm created ok
}
}
 }
 return dtINVALID_ALARM_ID;
}

TimeAlarmsClass Alarm = TimeAlarmsClass() ;

Title: Re: bug in the timealarm library
Post by: viknet on Jul 03, 2011, 10:34 pm
hello mem,
I read your code and you made a huge work on it, you did include everything I need for my sketch, you even made it easier to later extend it with other type of alarm (monthly even/odd each 15th of each month...) and I think this is a must for some people (although I don't need them personaly ).

So a BIG THANKS for your work.


I will include this in my sketch (although it won't be straightforward) and will tell you if everything is working (just give me a few days).

Best regards

Viknet
Title: Re: bug in the timealarm library
Post by: viknet on Jul 03, 2011, 11:09 pm
I think we need an extended Alarm.write function:

Code: [Select]
void TimeAlarmsClass::write(AlarmID_t ID, time_t value,dtAlarmPeriod_t period)
{
  if(ID < dtNBR_ALARMS && Alarm[ID].Mode.isAllocated){
    Alarm[ID].value = value;
    enable(ID);
    Alarm[ID].Mode.alarmType=period;
  }
}


Otherwise it is not possible to set the period of an alarm (I need it in my sketch)
we could also add

TimeAlarmsClass::write(AlarmID_t ID, time_t value,dtAlarmPeriod_t period,OnTick_t onTickHandler)

I will use the last one (also not mandatory) on the contrary to the previous on which I definitly need.

Best regards
Title: Re: bug in the timealarm library
Post by: mem on Jul 03, 2011, 11:42 pm
Yes, I can see that the new structure could need an enhanced write that takes the dtAlarmPeriod_t but I am not sure how it would actually be used.

Can you explain the use case for changing the tick handler callback?

And also can you explain the use case where the period type would be  changed?



Title: Re: bug in the timealarm library
Post by: draythomp on Jul 04, 2011, 02:34 am
Well, I can't break it.  I'm still running tests, but nothing I've tried is failing.   All the capabilities I use work fine.  Right now I have about 20 total timers and alarms running from as short as a second to a second less than a day and it just keeps on working.  I intend to run this some more and try some random timers to see there may be something hiding, but looks good.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 04, 2011, 09:57 am
Quote
Yes, I can see that the new structure could need an enhanced write that takes the dtAlarmPeriod_t but I am not sure how it would actually be used.

Can you explain the use case for changing the tick handler callback?

And also can you explain the use case where the period type would be  changed?


Well I am building a weekly programmer like this one:(http://www.conrad.fr/wcsstore/ConradImages/PRODUITS/1626516_p_g.jpg) except having many output and instead of having display and button it will have a serial connection (connected to a wifi computer handling php pages)

Using serial I need to set and reset alarm to the user desire :=) all handler are identical and I have a structure which give me information for each alarmid (which input to turn on or off) which sprinkler to enable for  how long......

beeing able to get/set the periodicity enable me to change on the fly alarm, at this moment I only can free an alarm and create a new one.

In this case I can only determine the id after the creation so if a user want to modify the periodicity of alarmid=3,
I have to free alarmid=3  then create anotherone having all parameter (zones/durations) identical except periodicity and give the new id to the user.

Concerning the handler, beeing able to get/write would enable me to have two different tye of program with different logic behind, some program would be simpleon/off the other sprinkler program, If I need to display the program to the user I need to have access to the handler information. at this moment as said before, all program do the same and the program has a type with a big if at the beginning.

on a different subject, the fact that now weelky and daily cannot be set the same way I had to put something like this:

Code: [Select]
if (day==0)
 Alarm.alarmRepeat(H,M,S,handler);
else
 Alarm.alarmRepeat(day,H,M,S,handler);


when before I was only doing
Code: [Select]
Alarm.alarmRepeat(day,H,M,S,handler);

I made the "if" to test your lib but I am not that happy with it, it leads to unnecessarly cumbersome code. I don't know what would be the best, expose the create function to the outside or check if the day is 0 and then create a daily alarm.

best regards

viknet





Title: Re: bug in the timealarm library
Post by: mem on Jul 04, 2011, 12:17 pm
Viknet, thanks for the information.

In your application where the user can create or change any alarm type and period on any alarm, I  prefer an approach that rather than allowing any value to be written to any timer, restricting the input to the current alarm type. If the user needs to change the alarm type then he would need to delete the old alarm and create a new one of the required type. This allows some detection if invalid commands are received.

The code is open source so there is nothing stopping you changing your copy of the library to suit your application, but I prefer that the public version provides more error checking capability on changes applied to a timer than you want for your app.

As I mentioned in post #47, the new code differs from the playground version in that it returns an error if time values greater a day are given to the method to create daily alarms.
I think having separate  methods for daily and weekly alarms with error checking if the arguments are out of range is worth the extra if statement that may be needed in a sketch.
I would be interested in hearing views on this point from anyone using the current Timealarm library. I suspect that viknet and draythomp are not typical users and they would have no problem adjusting their code to suit, but I wonder if this would be a problem for others?

Michael
Title: Re: bug in the timealarm library
Post by: viknet on Jul 04, 2011, 02:58 pm
michael,

I understand your point of view concerning alarms, but if the write(AlarmID_t ID, time_t value) exist then the write(AlarmID_t ID, time_t value,dtAlarmPeriod_t period) needd to exist, I can understand both way of thinking (also I prefer the last),

But beeing able to read/write the periodicity is a must if you can read/write the value, that was my major fear when you proposed the migration.

Of course I understand the fact that this is opensource and I fully understand that what I need is not necessarly what most programmers need (and don't worry If I need to change the lib I will do it),
your call for comment is the good way of doing it, but seeing that that I was the fisrt to discover the bug concerning Dow, I think that there is not that much people using this feature.
maybe your request for comment, need to be open in another to gain visibility.

regards

Viknet


best regards
 
Title: Re: bug in the timealarm library
Post by: mem on Jul 04, 2011, 03:40 pm
Viknet, rest assured that you will be able to have the functionality you need even if its not in the playground version.

I am keen to keep the playground version  simple and safe. But I recognise that there are useful capabilities for testing or specialist applications that go beyond this scope and  you may have noticed that the alpha code I posted here yesterday had the write, read, readType,  free, count and getNextTrigger as private methods unless a #define was used to expose them.
My intention is to hide all of those functions in the standard distribution and discourage their use with sketches that are indented for public distribution.  I doubt any of these functions are needed for the kinds of applications that the typical arduino user is making.  Experienced  users such as yourself can find the #define to turn these on but I would discourage the publishing of sketches that require these methods as I think its very easy for  inexperienced users to get into trouble  if they play around with this low level alarm logic.
I feel strongly that in the Arduino world its better to provide robust but simple capabilities that address 90% of the likely applications rather than something highly flexible but easy to abuse.
Perhaps there is room for two libraries, one limited to the public methods I have proposed and another with a different name that includes low level functions.
I would be interested to hear comments on this.

Michael
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 04, 2011, 07:22 pm
Back when I was looking at how to control things around the house I got most of my ideas from devices available at the store.  The more expensive drip water controllers had days of the week and a couple of alarms allotted to each day.  So, a total of 14 alarms that I could program for any start time and duration.  Others had more alarms per day and multiple outputs.  They were all the same type and I couldn't change them to be something else.  The price on these things went from around U$20 for one alarm a day up to well over a hundred for more control.  None of them was remotely programmable nor reported what they were doing.  That's one reason why I started looking for something I could program to do exactly what I want.

I like the simple interface of create it, let it expire and then create another one.  But, that makes it hard to actually use because there is no central place to change it, you have to change the code tor create another one as well.  So, a repeat alarm is good.  Now some folks need to be able to cancel a current one (maybe it rained), but allow it to repeat again day after tomorrow.  Other folks want to do other things that I haven't even suspected were possible.  So, there is probably no end to the possible uses of alarms.  Timers are different in that you set it and it expires and does something.  The idea of a repeat is a good one so the programmer doesn't have to keep resetting it.  The ability to cancel it is good also so failsafe alarms can be created. 

And vicnet has a really good point that he was the one that found the initial problem with the daily alarm.  I would have run into it during a future phase of my work, but I was using a daily alarmOnce and recreating it in the callback because I might not have wanted it to go off next time exactly the same way.  Alarms and timers can be used as program flow control tools as well as turning on relays.  However, this means that few people used AlarmRepeat in a daily fashion and actually tested the code, including me.

This was partly because, at the time I started working on that project, there was practically no mention of TimeAlarm in the playground.  This really surprised me because things like sprinkler timers and Christmas lights would have this as a central component.  Much of the stuff we do around the house for real applications is alarm based.  And, most notably, almost everyone makes an arduino clock as one of their first projects.  I was concerned that the library was out of date and even posted a thread that only got two responses since I could have easily googled for it here (http://arduino.cc/forum/index.php/topic,59161.msg425832.html#msg425832).  The responders didn't realize I was trying to make sure that the library was valid and that I had read almost everything on the darn internet about it already. 

Net, the ability to create, modify and delete a daily alarm is necessary.  Additionally, being able to count and list them would make it versatile for people that want to create a sophisticated control system.  For example, if a user wants a menu interface to the alarms they create for something like a sprinkler controller, they will need to know how many there are, when they expire and what kind to put up a nice display.  The guy that did his controller for a dish washer could have easily used this kind of capability.

Please don't misunderstand, I realize that a line has to be drawn somewhere, features can't just be added forever.  But a recurring daily alarm is so darn nice to build into real-life control systems that some complexity is not only warranted, it's downright wonderful.  And, some of us (well, maybe only me) are using these little devices to do real life stuff.  Heck, I'm working on building one into my refrigerator to control it to my tastes instead of what the manufacturer thinks I need. 

So, put the primitive features at the top of the list of capabilities and lower down, the more sophisticated ones.  The people using it will read it and figure out what they need as they build the devices.  Keeping it simple will help the beginner, but people don't stay beginners for very long.  Simple is nice, but we out grow it pretty quickly.

Honestly, I've been keeping myself under control.  Automatic setback thermostats have a weekday vs weekend alarm.  Real expensive ones have holidays built in and are programmable (my power company has these available for a price).  Birthday alarms are really fun.  Monthly alarms are great for paying the power bill.  Odd vs even days make for a nice cycle in some projects.  Alarms that calculate sunrise and sunset are great for outdoor lights.  Should I go on.
Title: Re: bug in the timealarm library
Post by: mem on Jul 04, 2011, 09:19 pm
This library has been implemented to suit the typical Arduino user, not an expert programmer.
Here are the basic capabilities supported in the current version for building typical applications:
-   Create  alarms  for a particular time of day that repeat daily or on a particular day of week.
-   Create a one time only alarms for particular time of day or dates
-   Create timers that trigger once only or repeat after an interval that can range from one second to one week
-   Disable and enable any of the created alarms or timers

Here are capabilities, some supported in the old version that could continue to be supported in the standard version or moved into the advanced category (they will probably be included in the standard version unless there is a good argument otherwise)
-   Determine if a given id is an alarm (time of day based) or a timer (duration based)
-   Read the current duration of a timer or trigger time of an alarm
-   Change the value (duration/trigger time) of an alarm/timer
-   Free up an alarm or timer to allow its resources to be used in the creation of another alarm or timer

And these are the advanced capabilities  that are being considered that can be enabled through a hash define:
-   Get the trigger time of the next alarm/timer
-   Get the number of allocated timers
-   Determine if a given alarm/timer id is allocated

My current thinking is that the typical Arduino user of this library will not need the advanced functionality listed above.  If a feature is only useful to programmers interested in making sophisticated control systems then IMO it does not belong in a library targeted for Arduino.  If you disagree with this point of view then perhaps we could discuss this as a matter of philosophy in a new thread in the general discussion section.  But although you have both made the case why you would like these features, I wonder if either of you would say that your needs are representative of typical  Arduino users of this library.

I would prefer not to have two libraries, one that is Arduino friendly, another for sophisticated users, but I would rather do that then add complex capabilities into the arduino library that are not relevant to the majority of arduino users.

As said in my earlier post, you are not going to lose the more advanced capabilities, they are available if you enable them through a hash define. But I do want to avoid the advanced features becoming part of the standard API for this library unless there is a strong use case relevant to more than a handful of users. I believe that almost all of the Arduino applications using this library can be built without the need for any of the advanced capabilities.

The first version of this library was posted here (http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1217881285) almost three years ago and has had 15768 reads.
The updated posting was here (http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1263305457) with 12947 reads.
I don't think you will a strong case for the features you are looking for in those threads.  If there are more recent threads requesting the more advanced functionality then please post the links. If functionality is useful for a lot of arduino users  then I will not hesitate to support it.

If it were not for the specialist requirement to test the library and to support you two that have contributed your time to this, I would not be inclined to include those more advanced capabilities at all. I do appreciate the time and effort you have devoted to this topic so please don't take this post as criticism. Its just that having watched and participated in the Arduino community since 2007 I think simplicity is more important than rich functionality. Although I realize that each person can have a different interpretation of this, I do believe that in a good Arduino API, less is more.

That's my view, lets see if we get wider feedback.

Michael
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 04, 2011, 09:34 pm
A define to turn them on is a reasonable idea IMHO.  Especially since anyone using very many alarms will be editing the max number anyway.  And, a small number of 'default' capabilities that are available without doing anything is reasonable also.  What I guess I'm asking is not to lose any of the features that have come about because someone needed them.  Sheltering them behind a define is a pretty good idea to keep complications down and not lose any inventiveness that has gone before.  That makes it especially easy for someone, once they get past a certain point in the learning curve, to go augment away.

By the way, I still haven't been able to break it.  The only failure so far was when it rolled over 2106 a couple of times.  I may be putting this latest code in something that is running for real just to push the envelope.
Title: Re: bug in the timealarm library
Post by: mem on Jul 04, 2011, 10:08 pm
I have run from 1971 to 2021 without any errors. Are you running the version that increments the time to just below the next trigger or the one that increments by 55 seconds?

Anyway, running in  one of your real apps would be a good test.


Quote
What I guess I'm asking is not to lose any of the features that have come about because someone needed them


What I was trying to say in my previous post is that a few people needing a feature is not it itself a justification for adding complexity to the API.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 04, 2011, 10:21 pm
Quote
I think simplicity is more important than rich functionality


This is the part I prefer I fully agree with you on this and having a two stage complexity that your propose is a very good idea.

I am already using the readType and free method extensively, and will continue to propose/implement myself other feature If I think I need them.

The new version is perfectly woking on my side without issue.

Going a bit further I have an issue with the
Code: [Select]
time_t TimeAlarmsClass::read(AlarmID_t ID) the fact that is returns 01 when failing is a bit strange because if someone set and alarm to 1 seconde after mignight it wont be able to distinguish an error from a real alarm.

unfortunately, I have no answer other than some hackery (for exemple using 111111111 ie  07 / 09 / 73 @ 7:11:51pm EST) which is probably a lot less used than 1 seconde after midnight (but still not the right way of doing it)

This issue is not a big deal for me and I personaly don't care (because user can only set minute precise alarm) but I wanted to share this with you in case you have a better Idea.

Best regards

Vincent
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 04, 2011, 10:29 pm
Actually, I've run both.  The tests that exceeded the upper limits were the just before trigger time ones.  I haven't gotten past 2014 on the 55 second tests.   I haven't done any real time testing since it takes so darn long.  I have it running right now in a real device, but the first alarm is not for several hours.  The timers seem to work fine in it though, I have a lot of them running in it.

Regarding complexity, I can't disagree, but it would be sad to lose working changes that other's might need and subsequently have to reinvent.

Y'know, if you were to post a note in the forum under 'project guidance' I bet a bunch of people would chime in for opinions.  Those folks don't seem reluctant to sound off.  However, they can be unkind as well at times.
Title: Re: bug in the timealarm library
Post by: mem on Jul 04, 2011, 11:10 pm


Going a bit further I have an issue with the
Code: [Select]
time_t TimeAlarmsClass::read(AlarmID_t ID) the fact that is returns 01 when failing is a bit strange because if someone set and alarm to 1 seconde after mignight it wont be able to distinguish an error from a real alarm.


it returns 0l (thats 0 as a long) not 01

In fact, I have already made a change to define the following in TimeAlarms.h
    #define dtINVALID_TIME     0L
my latest version of read returns this constant. Because 0l (0) is not an a valid alarm value it can be distinguished from a real alarm




Title: Re: bug in the timealarm library
Post by: mem on Jul 04, 2011, 11:24 pm


Regarding complexity, I can't disagree, but it would be sad to lose working changes that other's might need and subsequently have to reinvent.



Perhaps we will have to agree to disagree on this point. I believe that keeping things simple is more important than satisfying every demand.


Title: Re: bug in the timealarm library
Post by: viknet on Jul 04, 2011, 11:26 pm
my mistake...............


sorry for misreading your code , one more question so: could we then setup a daily midnight alarm (AFAIK time goes from 23:59:59 to 0:0:0)


regards

Viknet
Title: Re: bug in the timealarm library
Post by: mem on Jul 05, 2011, 12:50 am

one more question so: could we then setup a daily midnight alarm (AFAIK time goes from 23:59:59 to 0:0:0)

regards

Viknet


The following will set an alarm to trigger every day at midnight:
    Alarm.alarmRepeat(24,0,0, MidnightAlarm);

this will trigger each day at 0:0:0
Title: Re: bug in the timealarm library
Post by: mem on Jul 05, 2011, 09:06 pm
viknet and draythomp, I made some improvements to the TimeAlarms.cpp to remove some code (the new enumeration was duplicating information in some of the old flags in the data structure. The high level logic should be the same as the last code posted but this version will need a sanity check.  You can find the new code here (http://arduino.cc/playground/uploads/Code/Time_beta.zip). 

Note the Time.h header file has changed so copy that into your Time library directory
Copy the TimeAlarms/beta directory into the TimeAlarms library folder.

If you both get this running ok then I will open it up for wider inspection.

Michael
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 05, 2011, 11:54 pm
Uh, kinda make it hard to test

Code: [Select]

TimeAlarmsTest.cpp: In function 'void onAlarm()':
TimeAlarmsTest:37: error: 'class TimeAlarmsClass' has no member named 'getTriggeredAlarmId'
TimeAlarmsTest:51: error: 'class TimeAlarmsClass' has no member named 'free'
TimeAlarmsTest.cpp: In function 'void loop()':
TimeAlarmsTest:137: error: 'class TimeAlarmsClass' has no member named 'getNextTrigger'
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 05, 2011, 11:59 pm
Oh, nevermind, found it.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 06, 2011, 01:25 am
same issue, had to find the beta directory first :-)

missing
void TimeAlarmsClass::write(AlarmID_t ID, time_t value,dtAlarmPeriod_t period)
that I added immediatly,

The new isallocated function make code easier to read, but for the sake of my own knowledge, why a function instead of a macro ?


testing tonight and tommorow evening will send you result then.

regards
Title: Re: bug in the timealarm library
Post by: viknet on Jul 06, 2011, 07:50 am
It is leaking,

I have to see if it is your base code or my modification (new write function)

regards

-------------------------------
Updated:
the dtNBR_ALARMS was back to 6 (from the 8 I am currently using) and that triggered some memory leak, it was there with the previous version and I am not at all sure It come from your sketch, it is probably my code So no issue there, I restart my tests this morning without a memory leak.
Title: Re: bug in the timealarm library
Post by: viknet on Jul 06, 2011, 11:23 pm
perfectly working on my side :-)

what about you ?
Title: Re: bug in the timealarm library
Post by: draythomp on Jul 07, 2011, 02:06 am
I've had it going in a random fashion from a couple of years back to 5 years forward, starting over when it goes past the upper.  Both timers and alarms have been working fine.  I also ran it once to failure in the future and no problem came up.

I suggest publishing it and let other folk play with it as much as they want to.
Title: Re: bug in the timealarm library
Post by: mem on Jul 07, 2011, 03:35 am
Thanks for the feedback.

I will create a new zip with the directories in the expected places so its less confusing to install and publish tomorrow for wider testing

Title: Re: bug in the timealarm library
Post by: mem on Jul 07, 2011, 12:24 pm
Here (http://arduino.cc/playground/uploads/Code/Time_beta070711.zip) is a link to the beta Library that fixes the weekly alarm problem. The only change from the code I posted above is the directory structure now matches the old library so the contents can be directly copied into the libraries folder.  I also added this text in a release note:

Quote
Changes made July 5 2011:

This is a beta test version of the libraries that provides a fix for
recurring weekly alarms.

TheTime library is unchanged except for fixes to a macro in Time.h that
converts the number of days to elapsed seconds and the macros that are used
to calculate weekly alarms.

The functionality of the TimeAlarms library has changed slightly in order to
make the library easier to maintain and enhance.

In short, if your application used an Alarm method to create weekly
alarms then you should change your sketch  to use the methods that take the
explicit DayOfWeek parameter. Only the Alarm methods are affected,
Timer methods are as before.

Here are the details:
The previous version could set weekly alarms by calling the AlarmOnce method
with a value greater than one day to indicate the day and time of the weekly
alarm. This has changed and explicit calls to set weekly alarms must be used
when setting weekly alarms. If you were setting weekly alarms by calling either
   Alarm.alarmRepeat(weeklyValue,  AlarmFunction);
then you need to change this to:
  arm.alarmRepeat(DayOfWeek, Hour, Minute, Second,  AlarmFunction);
 
Also the clock must be set to a time on or after Jan1 1971 when using the Alarm methods.
Timer methods are unchanged and will function even if the clock has not been set


Lets see how this goes for a day or so and then I will circulate this more widely

Michael

p.s. viknet, note that I did not add in the enhanced write method. As explained in my earlier post, I think the library is better suited for its intended purpose without it so will need to add into your version.
Title: Re: bug in the timealarm library
Post by: mem on Jul 09, 2011, 11:00 am
I posted info on the update in a new thread  (http://arduino.cc/forum/index.php/topic,66048.0.html) so people would not need to read through the seven pages of background to get to the information on the beta release.

Michael

Title: Re: bug in the timealarm library
Post by: copet_pitik on Jul 09, 2011, 12:12 pm
First, i am noob in arduino and just tinkering arduino.

i make a clock using :
1. Arduino UNO
2. RTC DS1307
3. LCD 16*2

this is how it is work:
==> Visual Basic set RTC time
==> Visual Basic set the Alarm and Save to Arduino EEPROM

and this is the problem that I got:
I have an interval mode alarm, this mode have value to start the alarm, end the alarm, and interval value.
when i compare RTC time and EEPROM value and start the alarm, alarm time is late.
example:
I set the alarm :
- start time : 13.00 pm
- end time : 14.00 pm
- interval :  5 minute

the alarm should 'sound' at 13.05.00 pm (HH.MM.SS), but i get the alarm sound at 13.05.30 and will continue till 14.05.30 pm.

the code is below this post:



Quote
:smiley-fat: sorry for my english:smiley-fat:


Title: Re: bug in the timealarm library
Post by: copet_pitik on Jul 09, 2011, 12:14 pm
the code:
Quote

deleted
junk detected

Title: Re: bug in the timealarm library
Post by: mem on Jul 09, 2011, 12:37 pm
copet_pitik, this thread was reporting a specific issue with the Library so this is not the best place for discussions on help using the library.

I have created a thread for that purpose here  (http://arduino.cc/forum/index.php/topic,66054.0.html)and I suggest the we move this discussion there. Can you post a shorter version of your sketch that illustrates the problem so its easer to see if your problem is with Alarms or delays in your code to write to eeprom. It would be easier to help if you can translate comments into english so I can understand the purpose of each of you functions. A summary of the purpose of your sketch and overview of the logic you used to implement it would also help in understanding what your sketch is doing.

Michael
Title: Re: bug in the timealarm library
Post by: copet_pitik on Jul 09, 2011, 03:18 pm

copet_pitik, this thread was reporting a specific issue with the Library so this is not the best place for discussions on help using the library.

I have created a thread for that purpose here  (http://arduino.cc/forum/index.php/topic,66054.0.html)and I suggest the we move this discussion there. Can you post a shorter version of your sketch that illustrates the problem so its easer to see if your problem is with Alarms or delays in your code to write to eeprom.

Michael


^_^ wokey ^_^
Title: Re: bug in the timealarm library
Post by: arios85 on Feb 05, 2014, 07:35 am
Hello, I was wondering if an example could be done that displays all of the alarms at once without using the triggered command and using the read(AlarmID_t ID) method. I just cant seem to be able to get multiple alarms to display their time and that isn't triggered by its own alarm but rather a different alarm solely dedicated in collecting all the alarms at once and printing them. Using the read command from the library, I thought would be able to do this, i just can't figure out how.