[PATCH] Addded test for timer_create with clock_monotonic

zack leung zakthertemsdev at gmail.com
Thu Jul 1 00:47:59 UTC 2021


> +      remaining = (uint32_t)result->tv_nsec + result->tv_sec;
What does "remaining" mean here?  What do you get if you add a
(truncated) nanoseconds value to a seconds value? (Hint: the units
aren't the same, so the arithmetic is meaningless.)

I thought I could convert it back into the uint32_t. I thought Adding will
give me the remaining time.

 remaining = (uint32_t) ( ptimer->Timer.expire - now );

something similar to what was done there.



> -    _Timespec_From_ticks( remaining, &value->it_value );

> +    _Timespec_From_ticks(remaining, &value->it_value);

Why convert back and forth between timespec and ticks? why not just

update this function to use timespec values?


Do you want me to change remaining to be a timespec? change the field
it_value to a timespec?

Zack

Ps: thanks for being so patient with my patch. I'm still learning !


On Tue, 29 Jun 2021 at 16:44, Gedare Bloom <gedare at rtems.org> wrote:

> Hi Zack,
>
> Please provide a full name in your git-commit author metadata
> (git-config.user)
>
> Please use a short tag at the start of your commit to identify the
> scope, in this case, it will be "posix/timer"
>
> Check typo "addded" in your commit message. I think this commit is
> related to a ticket? if it finishes it, please use
> Closes #nnnn.
> where nnnn is the ticket number, or
> Updates #nnnn.
> If the patch does not close the ticket out.
>
> On Tue, Jun 29, 2021 at 9:19 AM Zack <zakthertemsdev at gmail.com> wrote:
> >
> > ---
> >  cpukit/include/rtems/posix/timer.h        |  1 +
> >  cpukit/posix/src/psxtimercreate.c         |  5 +-
> >  cpukit/posix/src/timergettime.c           | 79 ++++++++++++++++-------
> >  testsuites/psxtests/psxtimer02/psxtimer.c | 38 ++++++++++-
> >  4 files changed, 98 insertions(+), 25 deletions(-)
> >
> > diff --git a/cpukit/include/rtems/posix/timer.h
> b/cpukit/include/rtems/posix/timer.h
> > index bcbf07a65a..839fe3293c 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;
> To be consistent, add a comment like the lines above.
>
> >  } POSIX_Timer_Control;
> >
> >  /**
> > diff --git a/cpukit/posix/src/psxtimercreate.c
> b/cpukit/posix/src/psxtimercreate.c
> > index a63cf1d100..b60be3f229 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,7 +91,8 @@ 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;
> add spaces around =. Here, the lines above are aligned on the =, so
> you should also align. Write code that uses a consistent style as
> surrounding code / as the RTEMS Style.
>
> > +
> This blank line adds a lot of whitespace characters (spaces). Blank
> lines should be empty.
>
> >    _Watchdog_Preinitialize( &ptimer->Timer, _Per_CPU_Get_snapshot() );
> >    _Watchdog_Initialize( &ptimer->Timer, _POSIX_Timer_TSR );
> >    _Objects_Open_u32(&_POSIX_Timer_Information, &ptimer->Object, 0);
> > diff --git a/cpukit/posix/src/timergettime.c
> b/cpukit/posix/src/timergettime.c
> > index ee2a566f0e..57b0ab4918 100644
> > --- a/cpukit/posix/src/timergettime.c
> > +++ b/cpukit/posix/src/timergettime.c
> > @@ -6,6 +6,14 @@
> >   * @brief Function Fetches State of POSIX Per-Process Timers
> >   */
> >
> > +/**
> > + * @file
> > + *
> > + * @ingroup POSIXAPI
> > + *
> > + * @brief Function Fetches State of POSIX Per-Process Timers
>
> A file isn't a function
>
> > + */
> > +
> >  /*
> >   *  14.2.4 Per-Process Timers, P1003.1b-1993, p. 267
> >   *
> > @@ -21,13 +29,13 @@
> >  #include "config.h"
> >  #endif
> >
> > -#include <time.h>
> >  #include <errno.h>
> > -
> Why delete the space separating the two groups of includes, system
> includes and rtems includes?
>
> >  #include <rtems/posix/timerimpl.h>
> >  #include <rtems/score/todimpl.h>
> >  #include <rtems/score/watchdogimpl.h>
> >  #include <rtems/seterr.h>
> > +#include <rtems/timespec.h>
> > +#include <time.h>
> Why do you reorder the includes?
>
> >
> >  /*
> >   *          - When a timer is initialized, the value of the time in
> > @@ -36,38 +44,65 @@
> >   *            between the current time and the initialization time.
> >   */
> >
> > -int timer_gettime(
> > -  timer_t            timerid,
> > -  struct itimerspec *value
> > -)
> > -{
> > +int timer_gettime(timer_t timerid, struct itimerspec *value) {
> Why do you reformat the style of the function declaration? I shouldn't
> need to spend time reviewing your style changes, focus instead on
> writing code in the proper style and avoid making random style changes
> that are not related to the new functionality you're introducing.
>
> >    POSIX_Timer_Control *ptimer;
> > -  ISR_lock_Context     lock_context;
> > -  uint64_t             now;
> > -  uint32_t             remaining;
> > +  ISR_lock_Context lock_context;
> > +  uint32_t remaining;
> > +  Per_CPU_Control *cpu;
> > +  struct timespec *now;    // get time now either with
> we don't use // comments, and I don't know that you need this comment
> anyway.
> > +  struct timespec *expire; // expire
> ditto
>
> > +
> any good reason for a blank line here?
>
> > +  struct timespec *result;
> > +
> > +  if (!value)
> > +    rtems_set_errno_and_return_minus_one(EINVAL);
> >
> > -  if ( !value )
> > -    rtems_set_errno_and_return_minus_one( EINVAL );
> Changing the style and not in the right way. Please read and follow
> the RTEMS Coding Conventions:
> https://docs.rtems.org/branches/master/eng/coding.html
>
> > +  ptimer = _POSIX_Timer_Get(timerid, &lock_context);
> > +  if (ptimer == NULL) {
> > +    rtems_set_errno_and_return_minus_one(EINVAL);
> > +  }
> > +
> > +  if (ptimer->clock_type == CLOCK_REALTIME) {
> >
> > -  ptimer = _POSIX_Timer_Get( timerid, &lock_context );
> > -  if ( ptimer != NULL ) {
> > -    Per_CPU_Control *cpu;
> > +    cpu = _POSIX_Timer_Acquire_critical(ptimer, &lock_context);
> >
> > -    cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
> With the style changes mixed in here, it is hard to see what you
> changed that is functional change vs cosmetic.
>
> > -    now = cpu->Watchdog.ticks;
> > +    _TOD_Get(now); // get current time
> remove comment
>
> > +    rtems_timespec_from_ticks(ptimer->Timer.expire,
> > +                              expire);
> shouldn't need this linebreak?
>
> >
> > -    if ( now < ptimer->Timer.expire ) {
> > -      remaining = (uint32_t) ( ptimer->Timer.expire - now );
> > +    if (now->tv_nsec + now->tv_sec >
> > +        expire->tv_nsec + expire->tv_sec) {
> This is not the right thing to do. Use timespec helpers to compare times.
>
> > +      rtems_timespec_subtract(now, expire, result);
> > +
> > +      remaining = (uint32_t)result->tv_nsec + result->tv_sec;
> What does "remaining" mean here?  What do you get if you add a
> (truncated) nanoseconds value to a seconds value? (Hint: the units
> aren't the same, so the arithmetic is meaningless.)
>
> >      } else {
> >        remaining = 0;
> >      }
> >
> > -    _Timespec_From_ticks( remaining, &value->it_value );
> > +    _Timespec_From_ticks(remaining, &value->it_value);
> Why convert back and forth between timespec and ticks? why not just
> update this function to use timespec values?
>
> >      value->it_interval = ptimer->timer_data.it_interval;
> > +     _POSIX_Timer_Release(cpu, &lock_context);
> > +    return 0;
> > +  }
> > +
> > +  if (ptimer->clock_type == CLOCK_MONOTONIC) {
> > +
> > +    cpu = _POSIX_Timer_Acquire_critical(ptimer, &lock_context);
> > +    rtems_timespec_from_ticks(ptimer->Timer.expire, expire);
> This code is shared in both cases. Why not do this before splitting
> between CLOCK_REALTIME and CLOCK_MONOTONIC? There is more code that
> can be shared in the two cases. Identify what has to be different for
> the two cases, and write your code to maximize reuse and limit
> copy-paste.
>
> >
> > -    _POSIX_Timer_Release( cpu, &lock_context );
> > +    if (now->tv_nsec + now->tv_sec > expire->tv_nsec + expire->tv_sec) {
> Bad arithmetic. Use helpers.
>
> At this point, 'now' is uninitialized.
>
> > +
> > +      rtems_timespec_subtract(now, expire, result);
> > +      remaining = (uint32_t)result->tv_nsec + result->tv_sec;
> > +    } else {
> > +      remaining = 0;
> > +    }
> > +
> > +    _Timespec_From_ticks(remaining, &value->it_value);
> > +    value->it_interval = ptimer->timer_data.it_interval;
> > +    _POSIX_Timer_Release(cpu, &lock_context);
> >      return 0;
> >    }
> >
> > -  rtems_set_errno_and_return_minus_one( EINVAL );
> > +  rtems_set_errno_and_return_minus_one(EINVAL);
> >  }
> > diff --git a/testsuites/psxtests/psxtimer02/psxtimer.c
> b/testsuites/psxtests/psxtimer02/psxtimer.c
> > index 9f79d33c42..6aea61c498 100644
> > --- a/testsuites/psxtests/psxtimer02/psxtimer.c
> > +++ b/testsuites/psxtests/psxtimer02/psxtimer.c
> > @@ -20,6 +20,7 @@
> >   *  http://www.rtems.org/license/LICENSE.
> >   */
> >
> > +
> Don't add stray blank lines. I don't think there are any places in
> RTEMS where we have two or more blank lines in a row.
>
> >  #ifdef HAVE_CONFIG_H
> >  #include "config.h"
> >  #endif
> > @@ -63,7 +64,7 @@ void *POSIX_Init (
> >    fatal_posix_service_status_errno( status, EINVAL, "bad timer id" );
> >
> >    puts( "timer_create - OK" );
> > -  status = timer_create( CLOCK_REALTIME, NULL, &timer );
> > +  status = timer_create( CLOCK_REALTIME , NULL, &timer );
> >    posix_service_failed( status, "timer_create OK" );
> >
> >    puts( "timer_create - too many - EAGAIN" );
> > @@ -127,6 +128,41 @@ void *POSIX_Init (
> >    status = timer_delete( timer );
> >    fatal_posix_service_status_errno( status, EINVAL, "bad id" );
> >
> > +
> > +  /*
> > +   *  If these are not filled in correctly, we don't pass its error
> checking.
> > +   */
> > +
> > +
> > +puts( "timer_create - bad timer id pointer - EINVAL" );
> missing indentation
>
> > +  status = timer_create( CLOCK_MONOTONIC, &event, NULL );
> > +  fatal_posix_service_status_errno( status, EINVAL, "bad timer id" );
> > +
> > +  puts( "timer_create - OK" );
> > +  status = timer_create( CLOCK_MONOTONIC, NULL, &timer );
> > +  posix_service_failed( status, "timer_create OK" );
> > +
> > +  puts( "timer_create - 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 - 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 - 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210701/55d0ae17/attachment-0001.html>


More information about the devel mailing list