On Wed, Jun 30, 2021 at 6:48 PM zack leung <zakthertems...@gmail.com> wrote:
>
> > + 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.
>
There are two problems. First, you mix adding nanoseconds + seconds.
How much time is 1s + 1ns? Your calculation suggests it is 2ns.
Second, you can't cast down to 32-bit unless you're sure the result of
the arithmetic is less than 2^32. You should be ok here, because there
should be at most 1000000000 in the tv_nsec field, but in general you
do need to be very careful about truncating time values in case there
might be any bits used in the upper 32b of a 64b variable.
> 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?
>
I would lean toward keeping everything in timespec types.
> Zack
>
> Ps: thanks for being so patient with my patch. I'm still learning !
>
>
> On Tue, 29 Jun 2021 at 16:44, Gedare Bloom <ged...@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 <zakthertems...@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@rtems.org
>> > http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
