[PATCH] closes #3889
Gedare Bloom
gedare at rtems.org
Mon Jul 26 14:41:57 UTC 2021
On Sat, Jul 24, 2021 at 6:10 PM zack leung <zakthertemsdev at gmail.com> wrote:
>
> > - 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.
>
Then you can use
result.tv_nsec = 0;
result.tv_sec = 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.
>
It should be _TOD_Get(&now)? Otherwise, you pass a copy of the struct,
which then gets treated like it is a pointer (and that would be wrong
also).
>
> >
> > - 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.
>
You remove single space characters within the parentheses of the
function call, "( timerid" not "(timerid")
> > +
> > +
> > +
> > +
> 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.
>
It will be much better for you to learn how to use git, than try to
manually change path files.
>
> 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?
>
Use git-rebase with interactive mode. You should spend some time
learning how to use git.
>
>
>
>
>
> 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
More information about the devel
mailing list