Skip to content

Commit

Permalink
iox-eclipse-iceoryx#337 Improve code quality based on review comments…
Browse files Browse the repository at this point in the history 
… and handle expected errors

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <Sankara.Narayanan@in.bosch.com>
  • Loading branch information
@shankar-in
shankar-in committed Jun 2, 2021
1 parent 1f064dd commit 5669d41
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 74 deletions.
7 changes: 5 additions & 2 deletions iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/periodic_timer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down
48 changes: 32 additions & 16 deletions iceoryx_hoofs/source/posix_wrapper/periodic_timer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -48,10 +49,11 @@ void PeriodicTimer::start(const iox::units::Duration interval) noexcept

void PeriodicTimer::stop() noexcept
{
if (*(m_waitSemaphore.getValue()) == static_cast<int>(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());
}
}

Expand All @@ -72,19 +74,26 @@ cxx::expected<units::Duration, TimerErrorCause> PeriodicTimer::now() noexcept
cxx::expected<iox::posix::WaitResult, TimerErrorCause> PeriodicTimer::wait(TimerCatchupPolicy policy) noexcept
{
// To check if the TIMER is active (if the sempahore is acquired)
if (*(m_waitSemaphore.getValue()) == static_cast<int>(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<iox::posix::WaitResult>(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 =
Expand All @@ -96,7 +105,7 @@ cxx::expected<iox::posix::WaitResult, TimerErrorCause> 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())
{
Expand All @@ -108,17 +117,24 @@ cxx::expected<iox::posix::WaitResult, TimerErrorCause> PeriodicTimer::wait(Timer
return cxx::success<iox::posix::WaitResult>(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<iox::posix::WaitResult>(m_waitResult);
}

default:
{
return cxx::error<TimerErrorCause>(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())
{
Expand Down
148 changes: 92 additions & 56 deletions iceoryx_hoofs/test/moduletests/test_posix_periodic_timer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -89,35 +92,38 @@ 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);

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);
Expand All @@ -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), [&] {
Expand All @@ -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());
});

Expand Down

0 comments on commit 5669d41

Please sign in to comment.