> + 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 <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