[PATCH] closes #3889
Gedare Bloom
gedare at rtems.org
Fri Jul 23 17:01:58 UTC 2021
Hi Zak,
Please provide a useful first commit line. I think I've mentioned this
before, but the current guidance is found at
https://devel.rtems.org/wiki/Developer/Git#GitCommits
Put the ticket closing line within the commit message, usually on its
own line at the end of your commit message
On Thu, Jul 22, 2021 at 6:29 PM Zacchaeus Leung
<zakthertemsdev at gmail.com> wrote:
>
> ---
> cpukit/include/rtems/posix/timer.h | 1 +
> cpukit/posix/src/psxtimercreate.c | 1 +
> cpukit/posix/src/timergettime.c | 61 +++++++++++++++++++-----------
> 3 files changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/cpukit/include/rtems/posix/timer.h b/cpukit/include/rtems/posix/timer.h
> index bcbf07a65a..7ae089173a 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; /* The type of timer */
> } POSIX_Timer_Control;
>
> /**
> diff --git a/cpukit/posix/src/psxtimercreate.c b/cpukit/posix/src/psxtimercreate.c
> index a63cf1d100..804c7a41e7 100644
> --- a/cpukit/posix/src/psxtimercreate.c
> +++ b/cpukit/posix/src/psxtimercreate.c
> @@ -91,6 +91,7 @@ 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;
>
> _Watchdog_Preinitialize( &ptimer->Timer, _Per_CPU_Get_snapshot() );
> _Watchdog_Initialize( &ptimer->Timer, _POSIX_Timer_TSR );
> diff --git a/cpukit/posix/src/timergettime.c b/cpukit/posix/src/timergettime.c
> index ee2a566f0e..ff2176ac60 100644
> --- a/cpukit/posix/src/timergettime.c
> +++ b/cpukit/posix/src/timergettime.c
> @@ -28,6 +28,7 @@
> #include <rtems/score/todimpl.h>
> #include <rtems/score/watchdogimpl.h>
> #include <rtems/seterr.h>
> +#include <rtems/timespec.h>
>
> /*
> * - When a timer is initialized, the value of the time in
> @@ -35,39 +36,55 @@
> * - When this function is called, it returns the difference
> * between the current time and the initialization time.
> */
> -
avoid random whitespace changes.
> int timer_gettime(
> timer_t timerid,
> struct itimerspec *value
> -)
> +)
avoid random whitespace changes. I can't even tell what this one changes.
> {
> 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;
> + struct timespec *expire;
> + struct timespec *result;
>
> - if ( !value )
> - rtems_set_errno_and_return_minus_one( EINVAL );
> + if (!value)
> + rtems_set_errno_and_return_minus_one(EINVAL);
avoid whitespace changes. This change is inconsistent with our coding
conventions.
>
> - ptimer = _POSIX_Timer_Get( timerid, &lock_context );
> - if ( ptimer != NULL ) {
> - Per_CPU_Control *cpu;
> + ptimer = _POSIX_Timer_Get(timerid, &lock_context);
avoid whitespace changes. This change is inconsistent with our coding
conventions.
> + if (ptimer == NULL) {
add spaces after ( and before ), e.g.,
if ( ptimer == NULL ) {
^ ^
> + rtems_set_errno_and_return_minus_one(EINVAL);
Add spaces around EINVAL
> + }
>
> - cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
> - now = cpu->Watchdog.ticks;
> + cpu = _POSIX_Timer_Acquire_critical(ptimer, &lock_context);
avoid whitespace changes. This change is inconsistent with our coding
conventions.
> + rtems_timespec_from_ticks(ptimer->Timer.expire, expire);
This isn't correct. expire is an uninitialized pointer. How do you
compile and run this code? I'm surprised it passes tests with this
kind of bug.
What you should do instead is declare your struct timespec variable
and pass a pointer to it, e.g.,
struct timespec expire; <-- put that above
rtems_timespec_from_ticks( ptimer->Timer.expire, &expire );
^
^ ^
don't forget to add spaces after ( and before ). Use & to pass the
pointer to the local stack-allocated expire struct.
>
> - if ( now < ptimer->Timer.expire ) {
> - remaining = (uint32_t) ( ptimer->Timer.expire - now );
> + if (ptimer->clock_type == CLOCK_MONOTONIC) {
^
^
add spaces after ( and before )
> + _Timecounter_Nanouptime(now);
This isn't correct. now is an uninitialized pointer. How do you
compile and run this code? I'm surprised it passes tests with this
kind of bug.
Declare now like expire, and pass a pointer to the stack-local variable.
> + }
> + if (ptimer-
>clock_type == CLOCK_REALTIME) {
add spaces after ( and before )
> + _TOD_Get(now);
This isn't correct. now is an uninitialized pointer. How do you
compile and run this code? I'm surprised it passes tests with this
kind of bug.
> + }
> +
> +
> + if( rtems_timespec_less_than( now, expire )) {
^ ^
add space after if and before ) at the end.
> + rtems_timespec_subtract(now, expire, result);
This isn't correct. now, expire, and result are uninitialized pointers.
Declare result like now and expire, and pass a reference to the
stack-local variable.
> } else {
> - remaining = 0;
> + result->tv_nsec=0;
> + result->tv_sec=0;
This isn't correct. result is an uninitialized pointer.
Add spaces around =.
> }
> +
> + (*value).it_value=*result;
why using *. instead of -> notation?
> + value->it_interval = ptimer->timer_data.it_interval;
> +
> + _POSIX_Timer_Release(cpu, &lock_context);
> + return 0;
> +}
> +
> +
> +
> +
Don't add extra whitespaces. We don't use more than one blank line in
a row in RTEMS C Code.
>
> - _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 );
> -}
> --
> 2.32.0
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list