From 5669d414e8094e3d0cd456de78e9cebcc1194c30 Mon Sep 17 00:00:00 2001 From: "Sankara Narayanan Chandrasekar (RBEI/EMT2)" Date: Wed, 2 Jun 2021 13:25:36 +0530 Subject: [PATCH] iox-#337 Improve code quality based on review comments and handle expected errors Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) --- .../posix_wrapper/periodic_timer.hpp | 7 +- .../source/posix_wrapper/periodic_timer.cpp | 48 ++++-- .../moduletests/test_posix_periodic_timer.cpp | 148 +++++++++++------- 3 files changed, 129 insertions(+), 74 deletions(-) diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/periodic_timer.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/periodic_timer.hpp index 1d57dc3504..67192cd49a 100644 --- a/iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/periodic_timer.hpp +++ b/iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/periodic_timer.hpp @@ -26,7 +26,10 @@ namespace iox { namespace posix { -/// @brief This enum class offers the timer error codes. +/// @brief constant holds the acquired value of binary semaphore 0 +static constexpr int SEM_ACQUIRED = 0; + +/// @brief This enum class offers the timer error codes enum class TimerErrorCause { INVALID_ARGUMENTS, @@ -38,7 +41,7 @@ enum class TimerErrorCause INVALID_STATE }; -/// @brief This enum class offers the timer state events. +/// @brief This enum class offers the timer state events enum class TimerState : uint8_t { STOP, /// if the Timer is disabled diff --git a/iceoryx_hoofs/source/posix_wrapper/periodic_timer.cpp b/iceoryx_hoofs/source/posix_wrapper/periodic_timer.cpp index 4b66801ed7..caa60625e4 100644 --- a/iceoryx_hoofs/source/posix_wrapper/periodic_timer.cpp +++ b/iceoryx_hoofs/source/posix_wrapper/periodic_timer.cpp @@ -35,9 +35,10 @@ PeriodicTimer::PeriodicTimer(const iox::units::Duration interval) noexcept void PeriodicTimer::start() noexcept { stop(); - auto waitResult = m_waitSemaphore.timedWait(m_interval); - cxx::Ensures(!waitResult.has_error()); - m_timeForNextActivation = now().value() + m_interval; + cxx::Ensures(!m_waitSemaphore.timedWait(m_interval).has_error()); + auto currentTime = now(); + cxx::Ensures(!currentTime.has_error()); + m_timeForNextActivation = currentTime.value() + m_interval; } void PeriodicTimer::start(const iox::units::Duration interval) noexcept @@ -48,10 +49,11 @@ void PeriodicTimer::start(const iox::units::Duration interval) noexcept void PeriodicTimer::stop() noexcept { - if (*(m_waitSemaphore.getValue()) == static_cast(posix::SemaphoreWaitState::TIMEOUT)) + auto semValue = m_waitSemaphore.getValue(); + cxx::Ensures(!semValue.has_error()); + if (semValue.value() == iox::posix::SEM_ACQUIRED) { - auto stopResult = m_waitSemaphore.post(); - cxx::Ensures(!stopResult.has_error()); + cxx::Ensures(!m_waitSemaphore.post().has_error()); } } @@ -72,19 +74,26 @@ cxx::expected PeriodicTimer::now() noexcept cxx::expected PeriodicTimer::wait(TimerCatchupPolicy policy) noexcept { // To check if the TIMER is active (if the sempahore is acquired) - if (*(m_waitSemaphore.getValue()) == static_cast(posix::SemaphoreWaitState::TIMEOUT)) + auto semValue = m_waitSemaphore.getValue(); + cxx::Ensures(!semValue.has_error()); + if (semValue.value() == iox::posix::SEM_ACQUIRED) { - if (now().value() > m_timeForNextActivation) + auto currentTime = now(); + cxx::Ensures(!currentTime.has_error()); + if (currentTime.value() > m_timeForNextActivation) { - if (policy == TimerCatchupPolicy::IMMEDIATE_TICK) + switch (policy) { - m_timeForNextActivation = now().value(); + case iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK: + { + m_timeForNextActivation = currentTime.value(); m_waitResult.state = iox::posix::TimerState::TICK; return cxx::success(m_waitResult); } - else if (policy == TimerCatchupPolicy::SKIP_TO_NEXT_TICK) + + case iox::posix::TimerCatchupPolicy::SKIP_TO_NEXT_TICK: { - auto delay = now().value() - m_timeForNextActivation; // calculate the time delay + auto delay = currentTime.value() - m_timeForNextActivation; // calculate the time delay if (delay > m_interval) { auto totalSlotToBeSkipped = @@ -96,7 +105,7 @@ cxx::expected PeriodicTimer::wait(Timer { m_timeForNextActivation = m_timeForNextActivation + m_interval; // Skip to the next activation } - auto timeDiff = m_timeForNextActivation - now().value(); // calculate remaining time for activation + auto timeDiff = m_timeForNextActivation - currentTime.value(); // calculate remaining time for activation auto waitResult = m_waitSemaphore.timedWait(timeDiff); if (waitResult.has_error()) { @@ -108,17 +117,24 @@ cxx::expected PeriodicTimer::wait(Timer return cxx::success(m_waitResult); } } - else + + case iox::posix::TimerCatchupPolicy::HOLD_ON_DELAY: { - auto timeDiff = now().value() - m_timeForNextActivation; // Calculate the time delay + auto timeDiff = currentTime.value() - m_timeForNextActivation; // Calculate the time delay m_waitResult.state = iox::posix::TimerState::DELAY; m_waitResult.timeDelay = timeDiff; return cxx::success(m_waitResult); } + + default: + { + return cxx::error(TimerErrorCause::INVALID_ARGUMENTS); + } + } } else { - auto actualWaitDuration = m_timeForNextActivation - now().value(); + auto actualWaitDuration = m_timeForNextActivation - currentTime.value(); auto waitResult = m_waitSemaphore.timedWait(actualWaitDuration); if (waitResult.has_error()) { diff --git a/iceoryx_hoofs/test/moduletests/test_posix_periodic_timer.cpp b/iceoryx_hoofs/test/moduletests/test_posix_periodic_timer.cpp index b87c992006..17cab2886b 100644 --- a/iceoryx_hoofs/test/moduletests/test_posix_periodic_timer.cpp +++ b/iceoryx_hoofs/test/moduletests/test_posix_periodic_timer.cpp @@ -65,22 +65,25 @@ TEST_F(PeriodicTimer_test, ZeroINTERVALTest) Timer sut(0_s); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - bool result = timerState.value().state == iox::posix::TimerState::TICK ? true : false; - ASSERT_TRUE(result); + ASSERT_FALSE(timerState.has_error()); + EXPECT_EQ(timerState.value().state, iox::posix::TimerState::TICK); } TIMING_TEST_F(PeriodicTimer_test, DurationINTERVALTest, Repeat(5), [&] { Timer sut(INTERVAL); - auto timeBeforeWait = sut.now().value(); + auto timeBeforeWait = sut.now(); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - auto timeAfterWait = sut.now().value(); - auto duration = timeAfterWait - timeBeforeWait; - bool result = (duration.toMilliseconds() == INTERVAL.toMilliseconds()) ? true : false; + auto timeAfterWait = sut.now(); - TIMING_TEST_EXPECT_FALSE(timerState.has_error()); - TIMING_TEST_EXPECT_TRUE(result); + ASSERT_FALSE(timeBeforeWait.has_error()); + ASSERT_FALSE(timerState.has_error()); + ASSERT_FALSE(timeAfterWait.has_error()); + + auto duration = timeAfterWait.value() - timeBeforeWait.value(); + + TIMING_TEST_EXPECT_TRUE(duration.toMilliseconds() == INTERVAL.toMilliseconds()); }); TEST_F(PeriodicTimer_test, TimerStopTest) @@ -89,12 +92,11 @@ TEST_F(PeriodicTimer_test, TimerStopTest) sut.stop(); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - bool result = timerState.value().state == iox::posix::TimerState::STOP ? true : false; - ASSERT_TRUE(result); + ASSERT_FALSE(timerState.has_error()); + EXPECT_EQ(timerState.value().state, iox::posix::TimerState::STOP); } - TEST_F(PeriodicTimer_test, TimerStopAfterWaitTest) { Timer sut(INTERVAL); @@ -102,22 +104,26 @@ TEST_F(PeriodicTimer_test, TimerStopAfterWaitTest) auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); sut.stop(); timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - bool result = timerState.value().state == iox::posix::TimerState::STOP ? true : false; - ASSERT_TRUE(result); + ASSERT_FALSE(timerState.has_error()); + EXPECT_EQ(timerState.value().state, iox::posix::TimerState::STOP); } - TIMING_TEST_F(PeriodicTimer_test, ResetWithNewDurationINTERVALTest, Repeat(5), [&] { - Timer sut(INTERVAL); + constexpr int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. iox::units::Duration NEW_DURATION{100_ms}; - const int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. + Timer sut(INTERVAL); + sut.start(NEW_DURATION); - - auto timeBeforeWait = sut.now().value(); + auto timeBeforeWait = sut.now(); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - auto timeAfterWait = sut.now().value(); - auto duration = timeAfterWait - timeBeforeWait; + auto timeAfterWait = sut.now(); + + ASSERT_FALSE(timeBeforeWait.has_error()); + ASSERT_FALSE(timerState.has_error()); + ASSERT_FALSE(timeAfterWait.has_error()); + + auto duration = timeAfterWait.value() - timeBeforeWait.value(); TIMING_TEST_EXPECT_FALSE(timerState.has_error()); TIMING_TEST_EXPECT_TRUE(duration.toMilliseconds() - NEW_DURATION.toMilliseconds() <= RANGE_APPROX); @@ -127,87 +133,116 @@ TIMING_TEST_F(PeriodicTimer_test, currentTimeTest, Repeat(5), [&] { Timer sut(INTERVAL); struct timespec ts; - clock_gettime(CLOCK_REALTIME, &ts); - auto timeNow = sut.now().value(); + TIMING_TEST_EXPECT_TRUE(clock_gettime(CLOCK_REALTIME, &ts) == 0); + + auto timeNow = sut.now(); auto currentSystemTime = iox::units::Duration(ts); - bool result = timeNow.toMilliseconds() == currentSystemTime.toMilliseconds() ? true : false; - - TIMING_TEST_EXPECT_TRUE(result); + TIMING_TEST_EXPECT_FALSE(timeNow.has_error()); + TIMING_TEST_EXPECT_TRUE(timeNow.value().toMilliseconds() == currentSystemTime.toMilliseconds()); }); TIMING_TEST_F(PeriodicTimer_test, periodicityWithoutExecutionTimeTest, Repeat(5), [&] { Timer sut(INTERVAL); - const int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. - auto timeUntilNextActivation = sut.now().value() + INTERVAL; + constexpr int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. + auto timeUntilNextActivation = sut.now(); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - auto currentTime = sut.now().value(); + auto currentTime = sut.now(); - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK ? true : false); - TIMING_TEST_EXPECT_TRUE(currentTime.toMilliseconds() - timeUntilNextActivation.toMilliseconds() <= RANGE_APPROX); + TIMING_TEST_EXPECT_FALSE(timeUntilNextActivation.has_error()); + TIMING_TEST_EXPECT_FALSE(timerState.has_error()); + TIMING_TEST_EXPECT_FALSE(currentTime.has_error()); + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK); + TIMING_TEST_EXPECT_TRUE(currentTime.value().toMilliseconds() - (timeUntilNextActivation.value() + INTERVAL).toMilliseconds() <= RANGE_APPROX); }); TIMING_TEST_F(PeriodicTimer_test, periodicityExecutionTimeLessThanActivationTimeTest, Repeat(5), [&] { constexpr int EXECUTIONTIME = 30; - const int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. + constexpr int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. Timer sut(INTERVAL); - auto timeUntilNextActivation = sut.now().value() + INTERVAL; - + + auto timeBeforeActivation = sut.now(); std::this_thread::sleep_for(std::chrono::milliseconds(EXECUTIONTIME)); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - auto currentTime = sut.now().value(); + auto currentTime = sut.now(); + + TIMING_TEST_EXPECT_FALSE(timeBeforeActivation.has_error()); + TIMING_TEST_EXPECT_FALSE(timerState.has_error()); + TIMING_TEST_EXPECT_FALSE(currentTime.has_error()); + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK); - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK ? true : false); - TIMING_TEST_EXPECT_TRUE(currentTime.toMilliseconds() - timeUntilNextActivation.toMilliseconds() <= RANGE_APPROX); + auto expectedTimeOfActivation = timeBeforeActivation.value() + INTERVAL; + TIMING_TEST_EXPECT_TRUE(currentTime.value().toMilliseconds() - expectedTimeOfActivation.toMilliseconds() <= RANGE_APPROX); }); TIMING_TEST_F(PeriodicTimer_test, immediateCatchupPolicyTest, Repeat(5), [&] { constexpr int EXECUTIONTIME = 70; + constexpr int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. Timer sut(INTERVAL); std::this_thread::sleep_for(std::chrono::milliseconds(EXECUTIONTIME)); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK ? true : false); + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK); - auto currentTimeAfterExecution = sut.now().value(); + auto currentTimeAfterExecution = sut.now(); timerState = sut.wait(iox::posix::TimerCatchupPolicy::IMMEDIATE_TICK); - auto currentTimeAfterWait = sut.now().value(); + auto currentTimeAfterWait = sut.now(); - auto remainingTimeForNextActivation = currentTimeAfterWait - currentTimeAfterExecution; - const int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK ? true : false); + TIMING_TEST_EXPECT_FALSE(currentTimeAfterExecution.has_error()); + TIMING_TEST_EXPECT_FALSE(timerState.has_error()); + TIMING_TEST_EXPECT_FALSE(currentTimeAfterWait.has_error()); + + auto remainingTimeForNextActivation = currentTimeAfterWait.value() - currentTimeAfterExecution.value(); + + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK); TIMING_TEST_EXPECT_TRUE(remainingTimeForNextActivation.toMilliseconds() <= RANGE_APPROX); }); TIMING_TEST_F(PeriodicTimer_test, skipToNextTickCatchupPolicyWithLessDelayTest, Repeat(5), [&] { constexpr int EXECUTIONTIME = 70; + constexpr int TIME_SLOT_CONSUMED = 2; + constexpr int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. Timer sut(INTERVAL); - const int TIME_SLOT_CONSUMED = 2; + - auto timeBeforeActivation = sut.now().value(); + auto timeBeforeActivation = sut.now(); std::this_thread::sleep_for(std::chrono::milliseconds(EXECUTIONTIME)); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::SKIP_TO_NEXT_TICK); - auto timeAfterExecution = sut.now().value(); + auto timeAfterExecution = sut.now(); - auto timeBetweenActivation = timeAfterExecution - timeBeforeActivation; - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK ? true : false); - TIMING_TEST_EXPECT_TRUE(timeBetweenActivation.toMilliseconds() >= (INTERVAL.toMilliseconds() * TIME_SLOT_CONSUMED)); + TIMING_TEST_EXPECT_FALSE(timeBeforeActivation.has_error()); + TIMING_TEST_EXPECT_FALSE(timerState.has_error()); + TIMING_TEST_EXPECT_FALSE(timeAfterExecution.has_error()); + + auto timeBetweenActivation = timeAfterExecution.value() - timeBeforeActivation.value(); + auto diffInActivationTime = timeBetweenActivation.toMilliseconds() - (INTERVAL.toMilliseconds() * TIME_SLOT_CONSUMED); + + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK); + TIMING_TEST_EXPECT_TRUE(diffInActivationTime <= RANGE_APPROX); }); TIMING_TEST_F(PeriodicTimer_test, skipToNextTickCatchupPolicyWithLargeDelayTest, Repeat(5), [&] { constexpr int EXECUTIONTIME = 150; + constexpr int TIME_SLOT_CONSUMED = 3; + constexpr int RANGE_APPROX = 2; // 2ms approximation. This may be lost in execution. Timer sut(INTERVAL); - const int TIME_SLOT_CONSUMED = 3; + - auto timeBeforeActivation = sut.now().value(); + auto timeBeforeActivation = sut.now(); std::this_thread::sleep_for(std::chrono::milliseconds(EXECUTIONTIME)); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::SKIP_TO_NEXT_TICK); - auto timeAfterExecution = sut.now().value(); + auto timeAfterExecution = sut.now(); + + TIMING_TEST_EXPECT_FALSE(timeBeforeActivation.has_error()); + TIMING_TEST_EXPECT_FALSE(timerState.has_error()); + TIMING_TEST_EXPECT_FALSE(timeAfterExecution.has_error()); - auto timeBetweenActivation = timeAfterExecution - timeBeforeActivation; - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK ? true : false); - TIMING_TEST_EXPECT_TRUE(timeBetweenActivation.toMilliseconds() >= (INTERVAL.toMilliseconds() * TIME_SLOT_CONSUMED)); + auto timeBetweenActivation = timeAfterExecution.value() - timeBeforeActivation.value(); + auto diffInActivationTime = timeBetweenActivation.toMilliseconds() - (INTERVAL.toMilliseconds() * TIME_SLOT_CONSUMED); + + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::TICK); + TIMING_TEST_EXPECT_TRUE(diffInActivationTime <= RANGE_APPROX); }); TIMING_TEST_F(PeriodicTimer_test, errorCatchupPolicyTest, Repeat(5), [&] { @@ -216,9 +251,10 @@ TIMING_TEST_F(PeriodicTimer_test, errorCatchupPolicyTest, Repeat(5), [&] { std::this_thread::sleep_for(std::chrono::milliseconds(EXECUTIONTIME)); auto timerState = sut.wait(iox::posix::TimerCatchupPolicy::HOLD_ON_DELAY); - auto delayExpected = EXECUTIONTIME - INTERVAL.toMilliseconds(); - TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::DELAY ? true : false); + + TIMING_TEST_EXPECT_FALSE(timerState.has_error()); + TIMING_TEST_EXPECT_TRUE(timerState.value().state == iox::posix::TimerState::DELAY); TIMING_TEST_EXPECT_TRUE(delayExpected == timerState.value().timeDelay.toMilliseconds()); });