[ECOS] Possible Bug in Cyg_Alarm ...

[ECOS] Possible Bug in Cyg_Alarm ...

[Thomas BINDER]

Hi folks!

I think I might have discovered a bug in the alarm handling of ECOS. If two (or more) alarms are set up to fire at the very same counter value, only one (the first that is initialized) istriggered , even if an interval is specified.

The problem is due to the fact that an alarm is only added to a Cyg_Counter if its trigger value is > (instead of >=) than the counter value.

The attached code snippet will reproduce the problem.

The program will print the number of times an alarm fired so far. The output looks like:

starting alarm test
alarms fired 0 times
alarms fired 0 times
alarms fired 0 times
alarms fired 0 times
alarms fired 1 times
alarms fired 1 times
[...]


Changing the add_alarm method fixes this problem such that the output of the program finally looks as shown below. The attached patch can be excuted from the top-level directory (one above packages): patch -p0 < clock.cxx.patch

starting alarm test
alarms fired 0 times
alarms fired 0 times
alarms fired 0 times
alarms fired 0 times
alarms fired 2 times
alarms fired 2 times
alarms fired 2 times
alarms fired 3 times
alarms fired 3 times
alarms fired 3 times
[...]



=========================== AlarmBug,cpp ===========================


#include <pkgconf/kernel.h>
#include <cyg/kernel/kapi.h>
#include <cyg/kernel/thread.hxx>
#include <cyg/kernel/thread.inl>
#include <cyg/kernel/clock.hxx>
#include <cyg/kernel/flag.hxx>

#include <new>


static char thrMem[ sizeof( Cyg_Thread ) ];
static char thrStack[ 0x1000 ];

Cyg_Thread* thr;


static void Run( cyg_addrword_t );
static void AlarmCB( Cyg_Alarm* alrm, CYG_ADDRWORD data );

externC void cyg_user_start()
{
    thr = new( thrMem )
	Cyg_Thread( 10, Run, 0, const_cast< char* >( "" ),
		    reinterpret_cast< cyg_addrword_t >( thrStack ),
		    sizeof( thrStack ) );
    
    thr->resume();
    
    Cyg_Scheduler::start();
}


Cyg_Alarm   alrm1( Cyg_Clock::real_time_clock, AlarmCB, 0 );
Cyg_Alarm   alrm2( Cyg_Clock::real_time_clock, AlarmCB, 0 );


unsigned int	fire( 0 );

static void Run( cyg_addrword_t )
{
    diag_printf("starting alarm test\n");
    
    cyg_tick_count  now( cyg_current_time() );

    alrm1.initialize( now + 100 );
    alrm2.initialize( now + 100, 100 );

    for ( ;; )
    {
	diag_printf("alarms fired %u times\n", fire);
	Cyg_Thread::self()->delay( 30 );
    }
}


static void AlarmCB( Cyg_Alarm* alrm, CYG_ADDRWORD data )
{
    ++fire;
}

====================== AlarmBug.cpp =======================

====================== clock.cxx.patch ======================
--- packages/kernel/current/src/common/clock.cxx.orig   Tue Aug  6 16:52:31 2002
+++ packages/kernel/current/src/common/clock.cxx        Tue Aug  6 16:51:40 2002
@@ -355,11 +355,11 @@
         {
             CYG_ASSERTCLASS(list_alarm, "Bad alarm in counter list" );
 
-            // The alarms are in ascending trigger order. When we
-            // find an alarm that is later than us, we go in front of
+            // The alarms are in ascending trigger order. When we find an alarm
+            // that is later (or at the same time!) than us, we go in front of
             // it.
         
-            if( list_alarm->trigger > alarm->trigger )
+            if( list_alarm->trigger >= alarm->trigger )
             {
                 alarm_list_ptr->insert( list_alarm, alarm );
                 break;
====================== clock.cxx.patch ======================



--
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] Possible Bug in Cyg_Alarm ...

[Larice Robert]

> Hi folks!
 
> I think I might have discovered a bug in the alarm handling of ECOS. If two (or more) alarms are set up to fire at the very same counter value, only one (the first that is initialized) istriggered , even if an interval is specified.
 
> The problem is due to the fact that an alarm is only added to a Cyg_Counter if its trigger value is > (instead of >=) than the counter value.



hello Thomas,

a tiny improvement of your problem description is to say:

   Cyg_Counter::add_alarm() fails to add a given alarm to alarm_list
     (CYGIMP_KERNEL_COUNTERS_SORT_LIST implementation)
     if there happens to be an alarm for the same time at the tail of
     alarm_list

(note the tail aspect, if you just look for > versus >= one would not
 understand why this should make any difference, unless one checks
 for the end of the do .. while condition)


i vote for your patch.


but it is necessairy to check whether there is somewhere a specification
which gurantees the execution order of alarm callbacks.
your patch reverses this order.

Robert Larice

-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] Possible Bug in Cyg_Alarm ...

[Thomas BINDER]

>>> Larice Robert <larice@vidisys.de> 08/07/02 09:52am >>>

hello Thomas,

> a tiny improvement of your problem description is to say:
[...]

Sorry for the lack of information, I thought this was abvious :-)


> i vote for your patch.
> but it is necessairy to check whether there is somewhere a specification which gurantees the execution order of alarm callbacks. your patch reverses this order.

Yes, the order of alarm intialization and callback execution is reversed for all alarms that expire at the same absolute counter value. Unfortunately, keeping the order of callback executions makes add_alarm a little less efficient. 

Maybe it should be (compile time) configurable whether the order should be preserved or not.

Tom




--
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss