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_cha...@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@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
