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