This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: cyg_alarm - suggestion for enhancement


On Fri, 2002-08-30 at 10:51, Nick Garnett wrote:
> NavEcos <ecos@navosha.com> writes:
> 
> > *cough*.  My that was incredibly stupid of me wasn't it?  I decided to
> > retrieve the values of the alarm callback function and data that
> > accompanies it as 2 separate functions to avoid pointers.  If you would
> > rather have it as a single call, let me know and I'll fix it.
> > 
> > Here are the prototypes now:
> > 
> > void cyg_alarm_set_callback(
> >      cyg_handle_t       alarm,
> >      cyg_alarm_t        *alarmfn, /* new alarm callback function  */
> >      cyg_addrword_t     data      /* new alarm callback data      */
> > );
> > 
> > cyg_alarm_t *cyg_alarm_get_callback( cyg_handle_t alarm );
> > 
> > cyg_addrword_t cyg_alarm_get_data( cyg_handle_t alarm );
> > 
> > 
> 
> That's better. Personally I would have used a single get function to
> emphasize the "togetherness" of the two values. But it is not a big
> enough point to argue about.
> 
> However, one thing that did occur to me is that the set function
> should be protected by taking the scheduler lock. If an interrupt
> occurs between setting the alarm function and setting the data, we
> could end up calling the function with the wrong data.
> 
> So the set_callback function should look like this:
> 
> inline void Cyg_Alarm::set_callback (
>         cyg_alarm_fn  *alarmfn,
>         CYG_ADDRWORD  newdata
>         )
> {
>     Cyg_Scheduler::lock();
>     alarm = alarmfn;
>     data = newdata;
>     Cyg_Scheduler::unlock();
> }
> 
> I think with that change I would be happy to approve it.

I see your point here but I did it that way since I (maybe incorrectly)
assumed that the alarm would be in an inactive state if you are changing
the callback function.  If the retrigger value is non 0 you have a race
condition because if you change the CB while it's running, you cannot
know beforehand which CB will get called since you can be pre-empted at
anytime.

How about this: I prevent the callback from being changed unless the
alarm's retrigger time is set to 0 or the alarm is disabled?  After all:
my justification for this is to make it compatible with the wd functions
of vxWorks, which doesn't have a retrigger value.  Still, there is the
possibility that a second thread could enable the alarm while the alarm
is being modified, but if a coder is doing that, they are asking for
trouble anyhow.

I will combine the "get" routines, I don't have religious connections to
it since I abstract the OS always in my projects with inline functions
and macros.

-Rich

> -- 
> Nick Garnett - eCos Kernel Architect
> http://www.eCosCentric.com/
> 



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]