bug in the timealarm library

[mem]

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

[draythomp]

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




[draythomp]

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.

[mem]

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

[draythomp]

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.

[mem]

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?

[viknet]

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)

[draythomp]

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

[viknet]

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

[mem]

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

[mem]

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.

[draythomp]

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.

[mem]

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.

[viknet]

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

[mem]

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.