Iox #32 implement posix timer and introduced timing test #69
Conversation
…atform Signed-off-by: Christian Eltzschig <[email protected]>
…iming tests; tested timing test construct in TimerStopWatch tests Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
…xpensive, therefore the timeout is increased Signed-off-by: Christian Eltzschig <[email protected]>
…ch-timer' of github.com:elfenpiff/iceoryx into iox-eclipse-iceoryx#32-implement-posix-timer-via-dispatch-timer
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
…ch-timer' of github.com:elfenpiff/iceoryx into iox-eclipse-iceoryx#32-implement-posix-timer-via-dispatch-timer
Signed-off-by: Christian Eltzschig <[email protected]>
…ort-part-II Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
…clipse-iceoryx#32-implement-posix-timer-via-dispatch-timer
…ch-timer' of github.com:elfenpiff/iceoryx into iox-eclipse-iceoryx#32-implement-posix-timer-via-dispatch-timer
Signed-off-by: Christian Eltzschig <[email protected]>
… the timer Signed-off-by: Christian Eltzschig <[email protected]>
…n on heavy system load Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
| #include <functional> | ||
| #include <string> | ||
|
|
||
| /// @brief This headers provides TIMING_TEST unit test infrastructure. The idea |
There was a problem hiding this comment.
The name "TIMING_TEST" is a litle bit misleading here because there are no timing constrints checked here. What is here done is to repeat the test until the condition is met or to fail. I know the background why we are doing that but then i would rename it to something like "REPEAT_TEST". That name is more meaningful. Iknow that we have different timing constraints on different platforms like arm or x64. But this should be tested different.
Maybe we can use the timeout mechanism of the semaphore to offer a real timing test which checks for a certain timing constraint?
There was a problem hiding this comment.
I think we should stick to the name TIMING_TEST since with this name we communicate to fellow developers that we are testing a construct with timing constraints. Our approach is to test it repeatedly but maybe later we have an even better approach and then the name should be the same and the rest should just change under the hood.
…hickup Signed-off-by: Christian Eltzschig <[email protected]>
… long only for mac os Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
Signed-off-by: Christian Eltzschig <[email protected]>
| // This test has to be repeated more often since we are spawning a lot | ||
| // of threads in here |
There was a problem hiding this comment.
Is this meant as an @todo?
Signed-off-by: Christian Eltzschig <[email protected]>
elfenpiff
dismissed stale reviews from FerdinandSpitzschnueffler, mossmaurice, and dkroenke
via
b2ecbe8
|
|
||
| static void stopTimerThread(timer_t timerid) | ||
| { | ||
| timerid->keepRunning.store(false, std::memory_order_relaxed); |
There was a problem hiding this comment.
this could be reordered after the notify_one call
| startTime = timerid->parameter.startTime; | ||
| } | ||
| curr_value->it_value.tv_sec = curr_value->it_interval.tv_sec - (currentTime.tv_sec - startTime.tv_sec); | ||
| curr_value->it_value.tv_nsec = curr_value->it_interval.tv_nsec - (currentTime.tv_nsec - startTime.tv_nsec); |
There was a problem hiding this comment.
the calculation could become negative, are positive seconds and negative nanoseconds allowed?
| { | ||
| timer->parameter.mutex.lock(); | ||
| bool doCallCallback = | ||
| !timer->parameter.runOnce || timer->parameter.runOnce && !timer->parameter.wasCallbackCalled; |
There was a problem hiding this comment.
please add braces to make the intention clear
|
|
||
| auto end = osTimer.now(); | ||
| public: | ||
| iox::units::Duration timeout{4_ms}; |
There was a problem hiding this comment.
could you make it const and uppercase to indicate that it's a constant
| osTimer.stop(); | ||
| TEST_F(TimerStopWatch_test, ResetWithDurationIsExpired) | ||
| { | ||
| Timer sut(1_ms); |
| TEST_F(TimerStopWatch_test, ResetWithDurationIsExpired) | ||
| { | ||
| Timer sut(1_ms); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(2)); |
There was a problem hiding this comment.
if no magic number was used, this could be 2 times the constant
Signed-off-by: Christian Eltzschig <[email protected]>
…ence is positive solved Signed-off-by: Christian Eltzschig <[email protected]>
No description provided.