On Sat, Jul 24, 2021 at 6:10 PM zack leung <zakthertems...@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 <j...@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 <ged...@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 >> > <zakthertems...@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@rtems.org >> > > http://lists.rtems.org/mailman/listinfo/devel >> > _______________________________________________ >> > devel mailing list >> > devel@rtems.org >> > http://lists.rtems.org/mailman/listinfo/devel _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel