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
---
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
#include
-
I think the blank line separated rtems includes from others. So please
don't remove it.
+#include
#include
#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
#include
#include
+#include
/*
* - 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
+
