[PATCH] part of implimenting a monotonic clock in rtems part of this is not the final patch.
Christian Mauderer
oss at c-mauderer.de
Fri Jun 18 08:01:18 UTC 2021
Hello Zack,
On 18/06/2021 04:34, zack_on_the_speed_chanel wrote:
> so I tested it with the but I have not made a new test file I'll do that soon. How do I add a new test? All the tests that test the timer functionality do work. I"m confident that all the changes I had to made to implement a monotonic clock should be there.
Regarding a new test: Just grep for example for psxtimer01 in the
sources and add the new test simmilar to the existing one.
The most relevant files are spec/build/testsuites/psxtests/*.yml for the
waf based build system and testsuites/psxtests/Makefile.am and
.../configure.ac for the old build system.
But please check whether you need a new test program. If you basically
only copy the test routine of psxtimer01 or psxtimer02 and replace all
CLOCK_REALTIME by CLOCK_MONOTONIC then you shouldn't add a new test.
Instead create a function in the test that is called once with
CLOCK_REALTIME and once with CLOCK_MONOTONIC.
Please make sure that the test not only tests the API but also whether
the timer work as expected. A timer with CLOCK_REALTIME can be affected
by changes in the wall clock. CLOCK_MONOTONIC should not be affected. So
please make sure that your test checks whether that is really the case.
Best regards
Christian
>
>
> Thanks
> Zack
>
> Sent with ProtonMail Secure Email.
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>
> On Tuesday, June 15th, 2021 at 7:29 AM, Christian Mauderer <oss at c-mauderer.de> wrote:
>
>> If you add a new functionallity you should add a test that tests the
>>
>> expected behaviour. You just shouldn't replace one but add a new test or
>>
>> a new test case to an existing test.
>>
>> Best regards
>>
>> Christian
>>
>> On 14/06/2021 22:18, zack_on_the_speed_chanel wrote:
>>
>>> also i'll revert the posix test that I changed back to normal because it was only for testing.
>>>
>>> Sent with ProtonMail Secure Email.
>>>
>>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>>>
>>> On Monday, June 14th, 2021 at 8:14 PM, zack_on_the_speed_chanel zack_on_the_speed_chanel at protonmail.ch wrote:
>>>
>>>> I've made most of the corrections to the code. I fixed up the formatting but I still don't know if I have to add anything for the settime and delete_timer(). i assume as the monotonic clock only affects the value I think it's all I have to do. I also can try to modify a posix test to check my theory.
>>>>
>>>> Thanks
>>>>
>>>> Zack
>>>>
>>>> Sent with ProtonMail Secure Email.
>>>>
>>>> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
>>>>
>>>> On Saturday, June 12th, 2021 at 9:31 AM, Christian Mauderer oss at c-mauderer.de wrote:
>>>>
>>>>> Hello Zack,
>>>>>
>>>>> I don't really know a lot about the timer toppic. So this is more of a
>>>>>
>>>>> style and general suggestion review.
>>>>>
>>>>> On 09/06/2021 20:27, zack wrote:
>>>>>
>>>>>> From: zack zack_on_the_speed_chanel at protonmail.ch
>>>>>>
>>>>>> cpukit/include/rtems/posix/timer.h | 6 ++-
>>>>>>
>>>>>> cpukit/posix/src/psxtimercreate.c | 5 +-
>>>>>>
>>>>>> cpukit/posix/src/timergettime.c | 61 ++++++++++++++++++++---
>>>>>>
>>>>>> testsuites/psxtests/psxtimer02/psxtimer.c | 26 +++++++---
>>>>>>
>>>>>> 4 files changed, 81 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/cpukit/include/rtems/posix/timer.h b/cpukit/include/rtems/posix/timer.h
>>>>>>
>>>>>> index bcbf07a65a..f8cf6115b2 100644
>>>>>>
>>>>>> --- a/cpukit/include/rtems/posix/timer.h
>>>>>>
>>>>>> +++ b/cpukit/include/rtems/posix/timer.h
>>>>>>
>>>>>> @@ -21,7 +21,7 @@
>>>>>>
>>>>>> #include <rtems/score/objectdata.h>
>>>>>>
>>>>>> #include <rtems/score/watchdog.h>
>>>>>> ---------------------------------
>>>>>
>>>>> I think the blank line separated rtems includes from others. So please
>>>>>
>>>>> don't remove it.
>>>>>
>>>>>> +#include <time.h>
>>>>>>
>>>>>> #include <pthread.h>
>>>>>>
>>>>>> #ifdef __cplusplus
>>>>>>
>>>>>> @@ -47,7 +47,9 @@ typedef struct {
>>>>>>
>>>>>> struct itimerspec timer_data; /* Timing data of the timer /
>>>>>>
>>>>>> 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 */
>>>>>>
>>>>>> - struct timespec time; /* Time at which the timer was started */
>>>>>>
>>>>>
>>>>> Not sure what changed in that line. Looks like a whitespace change.
>>>>>
>>>>> Please avoid these.
>>>>>
>>>>>> - clockid_t clock_type;
>>>>>
>>>>> Why a blank line?
>>>>>
>>>>>> } POSIX_Timer_Control;
>>>>>>
>>>>>> /**
>>>>>>
>>>>>> diff --git a/cpukit/posix/src/psxtimercreate.c b/cpukit/posix/src/psxtimercreate.c
>>>>>>
>>>>>> index a63cf1d100..e78c359bd5 100644
>>>>>>
>>>>>> --- a/cpukit/posix/src/psxtimercreate.c
>>>>>>
>>>>>> +++ b/cpukit/posix/src/psxtimercreate.c
>>>>>>
>>>>>> @@ -40,7 +40,7 @@ int timer_create(
>>>>>>
>>>>>> {
>>>>>>
>>>>>> POSIX_Timer_Control *ptimer;
>>>>>>
>>>>>> - if ( clock_id != CLOCK_REALTIME )
>>>>>>
>>>>>> - if ( clock_id != CLOCK_REALTIME || clock_id != CLOCK_MONOTONIC )
>>>>>>
>>>>>
>>>>> Not sure about that one. It's allways a bit difficult whith these
>>>>>
>>>>> constructions but I think this one is allways true and therefore will
>>>>>
>>>>> allways return an error.
>>>>>
>>>>> Let's assume clock_id is CLOCK_MONOTONIC. In that case (clock_id !=
>>>>>
>>>>> CLOCK_REALTIME) is true which means that the function will return with
>>>>>
>>>>> an error. The same works for CLOCK_RREALTIME and the second condition.
>>>>>
>>>>> If you apply De Morgan to the term you see that
>>>>>
>>>>> (clock_id != CLOCK_REALTIME || clock_id != CLOCK_MONOTONIC)
>>>>>
>>>>> is the same as
>>>>>
>>>>> !(clock_id == CLOCK_REALTIME && clock_id == CLOCK_MONOTONIC)
>>>>>
>>>>> The two inner comparisons are exclusive. So that won't work.
>>>>>
>>>>> What you most likely want is a
>>>>>
>>>>> !(clock_id == CLOCK_REALTIME || clock_id == CLOCK_MONOTONIC)
>>>>>
>>>>> or with De Morgan again:
>>>>>
>>>>> (clock_id != CLOCK_REALTIME && clock_id != CLOCK_MONOTONIC)
>>>>>
>>>>>> rtems_set_errno_and_return_minus_one( EINVAL );
>>>>>>
>>>>>> if ( !timerid )
>>>>>>
>>>>>>
>>>>>> @@ -91,7 +91,8 @@ 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 );
>>>>>>
>>>>>> _Objects_Open_u32(&_POSIX_Timer_Information, &ptimer->Object, 0);
>>>>>>
>>>>>> diff --git a/cpukit/posix/src/timergettime.c b/cpukit/posix/src/timergettime.c
>>>>>>
>>>>>> index ee2a566f0e..62011cde58 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
>>>>>>
>>>>>>
>>>>>>
>>>>>> @@ -43,21 +44,67 @@ int timer_gettime(
>>>>>>
>>>>>> {
>>>>>>
>>>>>> POSIX_Timer_Control *ptimer;
>>>>>>
>>>>>> ISR_lock_Context lock_context;
>>>>>>
>>>>>> - uint64_t now;
>>>>>>
>>>>>> uint32_t remaining;
>>>>>>
>>>>>> - Per_CPU_Control *cpu;
>>>>>>
>>>>>>
>>>>>> - struct timespec * now; // get time now either with
>>>>>>
>>>>>>
>>>>>> - struct timespec * expire; // expire
>>>>>>
>>>>>>
>>>>>> - struct timespec * result;// get remaining time
>>>>>>
>>>>>>
>>>>>> if ( !value )
>>>>>>
>>>>>> - rtems_set_errno_and_return_minus_one( EINVAL );
>>>>>>
>>>>>> - rtems_set_errno_and_return_minus_one( EINVAL );
>>>>>>
>>>>>> ptimer = _POSIX_Timer_Get( timerid, &lock_context );
>>>>>>
>>>>>> - if ( ptimer != NULL ) {
>>>>>>
>>>>>> - Per_CPU_Control *cpu;
>>>>>>
>>>>>> - if(ptimer== NULL ){
>>>>>>
>>>>>
>>>>> Style: There should be a space before the ==
>>>>>
>>>>>> - rtems_set_errno_and_return_minus_one( EINVAL );
>>>>>> - }
>>>>>>
>>>>>> +if ( ptimer->clock_type ==CLOCK_REALTIME) {
>>>>>
>>>>> Style again: Space after ==
>>>>>
>>>>> Why that many blank lines?
>>>>>
>>>>>> - cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
>>>>>>
>>>>>> - _TOD_Get(now); // get current time
>>>>>>
>>>>>>
>>>>>> - rtems_timespec_from_ticks (ptimer->Timer.expire,expire ); // get the time to expire
>>>>>>
>>>>>>
>>>>>
>>>>> That are C++ style comments. We normally have C-Style comments in RTEMS
>>>>>
>>>>> only. Beneath that the comments exceed the line length. And some of them
>>>>>
>>>>> are quite superflous. A _TOD_Get(now) doesn't need any comment. The line
>>>>>
>>>>> is already telling that.
>>>>>
>>>>> Again: Blank lines. I won't mention the further blank lines below!
>>>>>
>>>>>> - if (now->tv_nsec+now->tv_sec > expire->tv_nsec+expire->tv_sec) { // check if the time expired
>>>>>>
>>>>>> - rtems_timespec_subtract (now ,expire , result); //
>>>>>>
>>>>>>
>>>>>
>>>>> Empty comment at the end of the line?
>>>>>
>>>>>> - remaining = (uint32_t) result->tv_nsec+result->tv_sec;
>>>>>>
>>>>>>
>>>>>> - } else {
>>>>>>
>>>>>> - remaining = 0;
>>>>>>
>>>>>>
>>>>>> - }
>>>>>>
>>>>>> - _Timespec_From_ticks( remaining, &value->it_value );
>>>>>>
>>>>>> - value->it_interval = ptimer->timer_data.it_interval;
>>>>>>
>>>>>> - _POSIX_Timer_Release( cpu, &lock_context );
>>>>>>
>>>>>> - return 0;
>>>>>>
>>>>>> - }
>>>>>>
>>>>>>
>>>>>> +if ( ptimer->clock_type ==CLOCK_MONOTONIC) {
>>>>>
>>>>> Odd indentation and missing blank before the closing bracket.
>>>>>
>>>>> Indentation and some blanks on brackets seems to be a problem on quite
>>>>>
>>>>> some more lines too.
>>>>>
>>>>>> - cpu = _POSIX_Timer_Acquire_critical( ptimer, &lock_context );
>>>>>>
>>>>>>
>>>>>> - now = cpu->Watchdog.ticks;
>>>>>>
>>>>>> - _Timecounter_Nanouptime(now );
>>>>>>
>>>>>>
>>>>>> - rtems_timespec_from_ticks (ptimer->Timer.expire,expire );
>>>>>>
>>>>>>
>>>>>> - if (now->tv_nsec+now->tv_sec > expire->tv_nsec+expire->tv_sec) {
>>>>>>
>>>>>> - rtems_timespec_subtract (now, expire, result);
>>>>>>
>>>>>>
>>>>>> - if ( now < ptimer->Timer.expire ) {
>>>>>>
>>>>>> - remaining = (uint32_t) ( ptimer->Timer.expire - now );
>>>>>>
>>>>>>
>>>>>> - remaining = (uint32_t) result->tv_nsec+result->tv_sec;
>>>>>> } else {
>>>>>> remaining = 0;
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>> diff --git a/testsuites/psxtests/psxtimer02/psxtimer.c b/testsuites/psxtests/psxtimer02/psxtimer.c
>>>>>>
>>>>>> index 9f79d33c42..029e638c76 100644
>>>>>>
>>>>>> --- a/testsuites/psxtests/psxtimer02/psxtimer.c
>>>>>>
>>>>>> +++ b/testsuites/psxtests/psxtimer02/psxtimer.c
>>>>>>
>>>>>> @@ -59,17 +59,31 @@ void *POSIX_Init (
>>>>>>
>>>>>> fatal_posix_service_status_errno( status, EINVAL, "bad clock id" );
>>>>>>
>>>>>> puts( "timer_create - bad timer id pointer - EINVAL" );
>>>>>>
>>>>>>
>>>>>> - status = timer_create( CLOCK_REALTIME, &event, NULL );
>>>>>>
>>>>>> - status = timer_create( CLOCK_MONOTONIC, &event, NULL );
>>>>>>
>>>>>> fatal_posix_service_status_errno( status, EINVAL, "bad timer id" );
>>>>>>
>>>>>
>>>>> It seems that you replace the test for REALTIME timers. Are they no
>>>>>
>>>>> longer supported with your changes? I'm not sure whether that is a good
>>>>>
>>>>> idea because it would break all existing applications that use REALTIME
>>>>>
>>>>> timer.
>>>>>
>>>>>> puts( "timer_create - OK" );
>>>>>>
>>>>>>
>>>>>> - status = timer_create( CLOCK_REALTIME, NULL, &timer );
>>>>>>
>>>>>> - status = timer_create( CLOCK_MONOTONIC, NULL, &timer );
>>>>>>
>>>>>> posix_service_failed( status, "timer_create OK" );
>>>>>>
>>>>>> puts( "timer_create - too many - EAGAIN" );
>>>>>>
>>>>>> - status = timer_create( CLOCK_REALTIME, NULL, &timer1 );
>>>>>>
>>>>>> - status = timer_create( CLOCK_MONOTONIC, NULL, &timer1 );
>>>>>>
>>>>>> fatal_posix_service_status_errno( status, EAGAIN, "too many" );
>>>>>>
>>>>>> - puts( "timer_create (monotonic) - bad timer id pointer - EINVAL" );
>>>>>>
>>>>>> - status = timer_create( CLOCK_MONOTONIC, &event, NULL );
>>>>>>
>>>>>> - fatal_posix_service_status_errno( status, EINVAL, "bad timer id" );
>>>>>>
>>>>>> - puts( "timer_create (Monotonic) - OK" );
>>>>>>
>>>>>> - status = timer_create( CLOCK_MONOTONIC, NULL, &timer );
>>>>>>
>>>>>> - posix_service_failed( status, "timer_create OK" );
>>>>>>
>>>>>> - puts( "timer_create (monotonic)- too many - EAGAIN" );
>>>>>>
>>>>>> - status = timer_create( CLOCK_MONOTONIC, NULL, &timer1 );
>>>>>>
>>>>>> - fatal_posix_service_status_errno( status, EAGAIN, "too many" );
>>>>>>
>>>>>> - puts( "timer_delete - bad id - EINVAL" );
>>>>>>
>>>>>> status = timer_delete( timer1 + 1 );
>>>>>>
>>>>>> fatal_posix_service_status_errno( status, EINVAL, "bad id" );
>>>>>>
>>>>>> @@ -100,14 +114,14 @@ void *POSIX_Init (
>>>>>>
>>>>>> status = timer_settime( timer, TIMER_ABSTIME, &itimer, NULL );
>>>>>>
>>>>>> fatal_posix_service_status_errno( status, EINVAL, "bad itimer value #2" );
>>>>>>
>>>>>> - clock_gettime( CLOCK_REALTIME, &now );
>>>>>>
>>>>>> - clock_gettime( CLOCK_MONOTONIC, &now );
>>>>>>
>>>>>> itimer.it_value = now;
>>>>>>
>>>>>> itimer.it_value.tv_sec = itimer.it_value.tv_sec - 1;
>>>>>>
>>>>>> puts( "timer_settime - bad itimer value - previous time - EINVAL" );
>>>>>>
>>>>>> status = timer_settime( timer, TIMER_ABSTIME, &itimer, NULL );
>>>>>>
>>>>>> fatal_posix_service_status_errno( status, EINVAL, "bad itimer value #3" );
>>>>>>
>>>>>> - clock_gettime( CLOCK_REALTIME, &now );
>>>>>>
>>>>>> - clock_gettime( CLOCK_MONOTONIC, &now );
>>>>>>
>>>>>> itimer.it_value = now;
>>>>>>
>>>>>> itimer.it_value.tv_sec = itimer.it_value.tv_sec + 1;
>>>>>>
>>>>>> puts( "timer_settime - bad id - EINVAL" );
>>>>>>
>>>>>> @@ -129,4 +143,4 @@ void *POSIX_Init (
>>>>>>
>>>>>> TEST_END();
>>>>>>
>>>>>> rtems_test_exit (0);
>>>>>>
>>>>>> -}
>>>>>>
>>>>>> +}
>>>>>>
>>>>>> \ No newline at end of file
>>>>>>
>>>>>
>>>>> devel mailing list
>>>>>
>>>>> devel at rtems.org
>>>>>
>>>>> http://lists.rtems.org/mailman/listinfo/devel
>
More information about the devel
mailing list