[PATCH] Addded test for timer_create with clock_monotonic
Gedare Bloom
gedare at rtems.org
Tue Jun 29 16:43:55 UTC 2021
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
More information about the devel
mailing list