Pages: [1] 2 3 ... 7   Go Down
Author Topic: bug in the timealarm library  (Read 10985 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Jr. Member
**
Karma: 1
Posts: 51
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
nextTrigger = value + previousMidnight(time);
is not correct it schould be
Code:
nextTrigger = value + previousfirstdowMidnight(time);
assuming the function previousfirstdowMidnight is properly created
furthermore the test made for daily alarm is not done:
Code:
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
Logged

Global Moderator
Netherlands
Offline Offline
Shannon Member
*****
Karma: 220
Posts: 13834
In theory there is no difference between theory and practice, however in practice there are many...
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset


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

Rob Tillaart

Nederlandse sectie - http://arduino.cc/forum/index.php/board,77.0.html -
(Please do not PM for private consultancy)

Offline Offline
Jr. Member
**
Karma: 1
Posts: 51
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

regards
Logged

New River, Arizona
Offline Offline
God Member
*****
Karma: 19
Posts: 936
Arduino rocks
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Trying to keep my house under control http://www.desert-home.com/

Offline Offline
Jr. Member
**
Karma: 1
Posts: 51
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

New River, Arizona
Offline Offline
God Member
*****
Karma: 19
Posts: 936
Arduino rocks
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Trying to keep my house under control http://www.desert-home.com/

Offline Offline
Jr. Member
**
Karma: 1
Posts: 51
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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:
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
« Last Edit: June 29, 2011, 01:13:23 am by viknet » Logged

London
Offline Offline
Tesla Member
***
Karma: 10
Posts: 6255
Have fun!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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:
#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:
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
Logged

London
Offline Offline
Tesla Member
***
Karma: 10
Posts: 6255
Have fun!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Logged

Offline Offline
Jr. Member
**
Karma: 1
Posts: 51
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

London
Offline Offline
Tesla Member
***
Karma: 10
Posts: 6255
Have fun!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

Offline Offline
Jr. Member
**
Karma: 1
Posts: 51
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

mem,
concerning your proposal for the
Code:
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
Logged

London
Offline Offline
Tesla Member
***
Karma: 10
Posts: 6255
Have fun!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

« Last Edit: June 29, 2011, 08:35:21 am by mem » Logged

New River, Arizona
Offline Offline
God Member
*****
Karma: 19
Posts: 936
Arduino rocks
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
#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);
  
}


Logged

Trying to keep my house under control http://www.desert-home.com/

New River, Arizona
Offline Offline
God Member
*****
Karma: 19
Posts: 936
Arduino rocks
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

TimeAlarms.h
Code:
#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 */

Logged

Trying to keep my house under control http://www.desert-home.com/

Pages: [1] 2 3 ... 7   Go Up
Jump to: