[PATCH v2] Test needed for timer_create with CLOCK_MONOTONIC
Gedare Bloom
gedare at rtems.org
Thu Aug 5 14:58:54 UTC 2021
On Thu, Aug 5, 2021 at 8:56 AM zack leung <zakthertemsdev at gmail.com> wrote:
>
> Here you could add an assert that ptimer->clock_type ==
> CLOCK_MONOTONIC || ptimer->clock_type == CLOCK_REALTIME.
>
> Would I need to assert that the timer is not a valid timer type? Also In psxtimer create I check if the timer is invalid.
>
> I can do a similar if statement as I did in psxtimercreate
>
You don't need an if statement in this case, because it should never
happen under normal circumstances. But maybe someone messed up how
they call this function
> if ( ptimer->clock_type != CLOCK_REALTIME && ptimer->clock_type != CLOCK_MONOTONIC )
> rtems_set_errno_and_return_minus_one( EINVAL );
> Zack
>
> On Wed, 4 Aug 2021 at 16:02, Gedare Bloom <gedare at rtems.org> wrote:
>>
>> On Wed, Jul 28, 2021 at 6:28 PM Zacchaeus Leung
>> <zakthertemsdev at gmail.com> wrote:
>> >
>> > the timer_create() method can use CLOCK_MONOTONIC but there was no test for this
>> >
>> > Closes #3888
>> > ---
>> > cpukit/include/rtems/posix/timer.h | 1 +
>> > cpukit/posix/src/psxtimercreate.c | 3 +-
>> > cpukit/posix/src/timergettime.c | 52 ++++++++++++++---------
>> > testsuites/psxtests/psxtimer02/psxtimer.c | 26 ++++++++++++
>> > 4 files changed, 60 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/cpukit/include/rtems/posix/timer.h b/cpukit/include/rtems/posix/timer.h
>> > index bcbf07a65a..7ae089173a 100644
>> > --- a/cpukit/include/rtems/posix/timer.h
>> > +++ b/cpukit/include/rtems/posix/timer.h
>> > @@ -48,6 +48,7 @@ typedef struct {
>> > uint32_t ticks; /* Number of ticks of the initialization */
>> > uint32_t overrun; /* Number of expirations of the timer */
>> > struct timespec time; /* Time at which the timer was started */
>> > + clockid_t clock_type; /* The type of timer */
>> > } POSIX_Timer_Control;
>> >
>> > /**
>> > diff --git a/cpukit/posix/src/psxtimercreate.c b/cpukit/posix/src/psxtimercreate.c
>> > index a63cf1d100..2b5a10140f 100644
>> > --- a/cpukit/posix/src/psxtimercreate.c
>> > +++ b/cpukit/posix/src/psxtimercreate.c
>> > @@ -40,7 +40,7 @@ int timer_create(
>> > {
>> > POSIX_Timer_Control *ptimer;
>> >
>> > - if ( clock_id != CLOCK_REALTIME )
>> > + if ( clock_id != CLOCK_REALTIME && clock_id != CLOCK_MONOTONIC )
>> > rtems_set_errno_and_return_minus_one( EINVAL );
>> >
>> > if ( !timerid )
>> > @@ -91,6 +91,7 @@ int timer_create(
>> > ptimer->timer_data.it_value.tv_nsec = 0;
>> > ptimer->timer_data.it_interval.tv_sec = 0;
>> > ptimer->timer_data.it_interval.tv_nsec = 0;
>> > + ptimer->clock_type = clock_id;
>> >
>> > _Watchdog_Preinitialize( &ptimer->Timer, _Per_CPU_Get_snapshot() );
>> > _Watchdog_Initialize( &ptimer->Timer, _POSIX_Timer_TSR );
>> > diff --git a/cpukit/posix/src/timergettime.c b/cpukit/posix/src/timergettime.c
>> > index ee2a566f0e..b29d03efa1 100644
>> > --- a/cpukit/posix/src/timergettime.c
>> > +++ b/cpukit/posix/src/timergettime.c
>> > @@ -28,6 +28,7 @@
>> > #include <rtems/score/todimpl.h>
>> > #include <rtems/score/watchdogimpl.h>
>> > #include <rtems/seterr.h>
>> > +#include <rtems/timespec.h>
>> >
>> > /*
>> > * - When a timer is initialized, the value of the time in
>> > @@ -39,35 +40,44 @@
>> > int timer_gettime(
>> > timer_t timerid,
>> > struct itimerspec *value
>> > )
>> >
>> > {
>> > POSIX_Timer_Control *ptimer;
>> > - ISR_lock_Context lock_context;
>> > - uint64_t now;
>> > - uint32_t remaining;
>> > + ISR_lock_Context lock_context;
>> > + Per_CPU_Control *cpu;
>> > + struct timespec now;
>> > + struct timespec expire;
>> > + struct timespec result;
>> >
>> > if ( !value )
>> > rtems_set_errno_and_return_minus_one( EINVAL );
>> >
>> > ptimer = _POSIX_Timer_Get( timerid, &lock_context );
>> > - if ( ptimer != NULL ) {
>> > - Per_CPU_Control *cpu;
>> > -
>> > - cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
>> > - now = cpu->Watchdog.ticks;
>> > -
>> > - if ( now < ptimer->Timer.expire ) {
>> > - remaining = (uint32_t) ( ptimer->Timer.expire - now );
>> > - } else {
>> > - remaining = 0;
>> > - }
>> > + if ( ptimer == NULL ) {
>> > + rtems_set_errno_and_return_minus_one( EINVAL );
>> > + }
>> >
>> > - _Timespec_From_ticks( remaining, &value->it_value );
>> > - value->it_interval = ptimer->timer_data.it_interval;
>> > + cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
>> > + rtems_timespec_from_ticks( ptimer->Timer.expire, &expire );
>> >
>> > - _POSIX_Timer_Release( cpu, &lock_context );
>> > - return 0;
>> > + if ( ptimer->clock_type == CLOCK_MONOTONIC ) {
>> > + _Timecounter_Nanouptime(&now);
>> > + }
>> > + if ( ptimer->clock_type == CLOCK_REALTIME ) {
>> Minor nit: this could be 'else if'
>>
>> > + _TOD_Get(&now);
>> > }
>> Here you could add an assert that ptimer->clock_type ==
>> CLOCK_MONOTONIC || ptimer->clock_type == CLOCK_REALTIME. Or you can
>> add:
>> else {
>> _POSIX_Timer_Release( cpu, &lock_context );
>> rtems_set_errno_and_return_minus_one( EINVAL );
>> }
>> and make sure to test by calling this function directly with an
>> invalid clock_type. This of course depends on making the above into
>> 'else if'.
>>
>> >
>> > - rtems_set_errno_and_return_minus_one( EINVAL );
>> > -}
>> > +
>> > + if ( rtems_timespec_less_than( &now, &expire ) ) {
>> > + rtems_timespec_subtract( &now, &expire, &result );
>> > + } else {
>> > + result.tv_nsec = 0;
>> > + result.tv_sec = 0;
>> > + }
>> > +
>> > + value->it_value=result;
>> add spaces around =
>>
>> > + value->it_interval = ptimer->timer_data.it_interval;
>> > +
>> > + _POSIX_Timer_Release( cpu, &lock_context );
>> > + return 0;
>> > +}
>> > \ No newline at end of file
>> > diff --git a/testsuites/psxtests/psxtimer02/psxtimer.c b/testsuites/psxtests/psxtimer02/psxtimer.c
>> > index 9f79d33c42..1a79369efb 100644
>> > --- a/testsuites/psxtests/psxtimer02/psxtimer.c
>> > +++ b/testsuites/psxtests/psxtimer02/psxtimer.c
>> > @@ -126,6 +126,32 @@ void *POSIX_Init (
>> > puts( "timer_delete - bad id - EINVAL" );
>> > status = timer_delete( timer );
>> > fatal_posix_service_status_errno( status, EINVAL, "bad id" );
>> > +
>> > + puts( "timer_create (monotonic) - bad timer id pointer - EINVAL" );
>> > + status = timer_create( CLOCK_MONOTONIC, &event, NULL );
>> > + fatal_posix_service_status_errno( status, EINVAL, "bad timer id" );
>> > +
>> > + puts( "timer_create (monotonic) - OK" );
>> > + status = timer_create( CLOCK_MONOTONIC, NULL, &timer );
>> > + posix_service_failed( status, "timer_create OK" );
>> > +
>> > + puts( "timer_create (monotonic) - too many - EAGAIN" );
>> > + status = timer_create( CLOCK_MONOTONIC, NULL, &timer1 );
>> > + fatal_posix_service_status_errno( status, EAGAIN, "too many" );
>> > +
>> > + clock_gettime( CLOCK_MONOTONIC, &now );
>> > + itimer.it_value = now;
>> > + itimer.it_value.tv_sec = itimer.it_value.tv_sec - 1;
>> > + puts( "timer_settime (monotonic) - bad itimer value - previous time - EINVAL" );
>> > + status = timer_settime( timer, TIMER_ABSTIME, &itimer, NULL );
>> > + fatal_posix_service_status_errno( status, EINVAL, "bad itimer value #3" );
>> > +
>> > + clock_gettime( CLOCK_MONOTONIC, &now );
>> > + itimer.it_value = now;
>> > + itimer.it_value.tv_sec = itimer.it_value.tv_sec + 1;
>> > + puts( "timer_settime (monotonic) - bad id - EINVAL" );
>> > + status = timer_settime( timer1, TIMER_ABSTIME, &itimer, NULL );
>> > + fatal_posix_service_status_errno( status, EINVAL, "bad id" );
>> >
>> > TEST_END();
>> > rtems_test_exit (0);
>> > --
>> > 2.32.0
>> >
>> > _______________________________________________
>> > devel mailing list
>> > devel at rtems.org
>> > http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list