[PATCH] closes #3889
Joel Sherrill
joel at rtems.org
Fri Jul 23 22:15:25 UTC 2021
I concur with Gedare's comments.
I would think your commit message would be similar to the subject of the ticket.
This looks like a decent example:
https://git.rtems.org/rtems/commit/?id=6c23252cdd8ea63d0fe13d9f99b7befbf070fe80
Please update and submit. Pay attention to compiler warnings.
On Fri, Jul 23, 2021 at 12:02 PM Gedare Bloom <gedare at rtems.org> wrote:
>
> 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
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list