[PATCH] closes #3889
zack leung
zakthertemsdev at gmail.com
Sun Jul 25 00:10:20 UTC 2021
> - remaining = 0;
> + result->tv_nsec=0;
> + result->tv_sec=0;
This isn't correct. result is an uninitialized pointer.
So then how do I set the timespec's value to 0? I have changed the
timespec variables i have declared from pointers to regular variables.
in the original code the result ( used to be the remaining time) should be
0.
> + _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.
Since the now timespec is defined as
struct timesepc now
TOD_GET used like this :
_TOD_GET(now)
I think the code will now work as intended as it now has a reference.
>
> - 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.
Can you explain where the whilespace is? Sometimes i have issues with
trailing whitespace as sometimes when i run format patch it's different
from the file that I'm working on.
> +
> +
> +
> +
Don't add extra whitespaces. We don't use more than one blank line in
a row in RTEMS C
Again I may have to change it manually within the patch file. I may talk
with people on discord for help to avoid flooding the mailing list.
Also how i solved the nesting was the completely delete the repository and
then i would rebuilt rtems. After making a emailed patch how do i do
another one without running into the same problem?
On Fri, 23 Jul 2021 at 22:15, Joel Sherrill <joel at rtems.org> wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210725/d223c28b/attachment.html>
More information about the devel
mailing list