Iox #337 deprecate posix timer #479

nihalchari · 2021-01-05T13:56:15Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

code contributed by @shankar-bosch

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

All checkboxes in the PR checklist are checked or crossed out
Merge

References

Partly closes Deprecate posix timer #337

shankar-in · 2021-01-05T14:05:36Z

This PR contains only the code related to the deadline use case. The Keep alive use case from @elBoberido will be adapted in the following PR.

elBoberido · 2021-01-05T18:49:57Z

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

+class Timer
+{
+  private:
+    iox::units::Duration m_measuringDuration;
+    iox::units::Duration m_startTime;
+
+    iox::units::Duration getCurrentMonotonicTime() noexcept;
+
+  public:
+    /// @brief Checks if the timer has expired compared to its start time
+    /// @return Is the elapsed time larger than duration set
+    bool hasTimerExpired() noexcept;
+
+    Timer(const iox::units::Duration) noexcept;
+
+    /// @brief reinitializes the starting time for the timer to the current time
+    void resetDurationTimer() noexcept;
+};


could you move this to iceoryx_utils/cxx and rename it to DeadlineTimer. Other frameworks call it also this way, e.g. QDeadlineTimer. I think an additional namespace is not necessary.

@MatthiasKillat @mossmaurice @dkroenke what do you think?

Yes I can rename this to deadline timer. The reason behind the new namespace introduction was the time calculations are based on the std::chrono and not using the POSIX apis.

DeadlineTimer is fine and reflects its functionality. The namespace cxx is ok for now, but we may need to differentiate between reimplementing STL (like) constructs and creating other useful helper classes in the long run. The DeadlineTimer would then belong into the second category.

@MatthiasKillat you have to think big. The STL has to add the stuff we put into the cxx namespace ;)

@dkroenke what do you think about moving this to cxx? It doesn't have any POSIX dependency anymore

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

iceoryx_utils/source/posix_wrapper/timer.cpp

MatthiasKillat

Looks OK functionally, only optional style remarks from my side.

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

iceoryx_utils/source/posix_wrapper/timer.cpp

iceoryx_utils/test/moduletests/test_posix_mutex.cpp

dkroenke · 2021-01-06T10:42:00Z

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

@@ -321,6 +321,36 @@ class Timer
 };

 } // namespace posix
+
+// IOX-#337 Alternate Timer to replace the POSIX timer


please make a /// @note of that

or maybe even better make an @brief with a small description. In one year nobody cares that this functionality was in POSIX Timer

Can you make the class comment more descriptive ?
What does it mean to be a deadline timer ? How do I use it ?

Okay added more descriptions for the class

Also, this file should be renamed to deadline_timer.hpp

no, the functionality needs to be moved to a new file. This file will contain a new timer which just wraps a semaphore and does a blocking wait until it's either interrupted or timed out. It will be used in PeridoicTask, which in combination with the new timer provides the functionality of the execution of a callback from the old timer.
The semaphore might later on be replaced by a timer_create call, but instead of spawning a thread, it will just block, like the semaphore and if someone needs a separate thread, the PeriodicTask should be used.

iceoryx_utils/source/posix_wrapper/timer.cpp

iceoryx_utils/test/moduletests/test_posix_mutex.cpp

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

elBoberido · 2021-01-06T12:12:00Z

@shankar-bosch I just noticed that your commit messages are not according our [guidelines] (https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md)

@nihalchari wasn't the PR template shown when created the PR?

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

orecham

Logic is fine. Just need to rename the timer file & add class documentation.

orecham · 2021-01-07T08:16:09Z

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

@@ -321,6 +321,36 @@ class Timer
 };

 } // namespace posix
+
+// IOX-#337 Alternate Timer to replace the POSIX timer


Can you make the class comment more descriptive ?
What does it mean to be a deadline timer ? How do I use it ?

orecham · 2021-01-07T08:18:04Z

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

@@ -321,6 +321,36 @@ class Timer
 };

 } // namespace posix
+
+// IOX-#337 Alternate Timer to replace the POSIX timer


Also, this file should be renamed to deadline_timer.hpp

orecham · 2021-01-07T10:09:19Z

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

+/// @brief Class which offers the deadline timer functionality
+/// @note This class is introduced as an alternate timer functionality for the deprecated POSIX timers
+///
+/// @code


iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

shankar-in · 2021-01-07T10:11:09Z

@ithier I havent renamed the file because currently this PR still keeps the deprecated methods for posix timers. once this is tested and removed the file would be renamed and moved out of the posix wrappers.

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

shankar-in · 2021-01-07T10:26:23Z

Working on the failing test.

dkroenke

please rework the RemainingTimeCheckIfNotExpired test

dkroenke · 2021-01-07T10:30:29Z

iceoryx_utils/test/moduletests/test_posix_timer.cpp

+
+    sut.reset(20_s);
+    std::this_thread::sleep_for(std::chrono::milliseconds(2 * TIMEOUT.milliSeconds<int>()));
+    EXPECT_THAT(sut.remainingTime(), Le(TIMEOUT));


This test will not work because you reset the timer with 20 Seconds and expect then a value lower or equal to 20 milliseconds after waiting 40 milliseconds. The logic is also incorrect because the test would also pass if no time has passed (Lower equal)

Proposal:

create a timer and wait for a defined time

get the remaining time

check if the result is in a range ±10 percent from the expected value

You need to make a range because the test could flicker due to the non-deterministic Github CI.
For an example please take a look at TIMING_TEST_F(Timer_test, RestartWithDifferentTiming) in line 237 of this file.

iceoryx_utils/source/posix_wrapper/timer.cpp

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp

iceoryx_utils/test/moduletests/test_posix_mutex.cpp

elBoberido

could you also please remove the obsolete methods like hasExpiredComparedToCreationTime from the old timer class

nihalchari · 2021-01-08T05:52:01Z

@shankar-bosch I just noticed that your commit messages are not according our [guidelines] (https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md)

@nihalchari wasn't the PR template shown when created the PR?

@elBoberido it was there.

@shankar-bosch could u please change this commit message "iox-337: initial commit for deadline impl " to "iox-#337 initial commit for deadline impl "
a7517f5

Also hyphen - used after issue no can be replaced with space in following commits
36cc71e
afef470

shankar-in · 2021-01-08T07:33:55Z

could you also please remove the obsolete methods like hasExpiredComparedToCreationTime from the old timer class

I have 3 stories created for POSIX timer,

part 1 : Introduce alternate timers for keepalive and deadline
part 2 : Check that the already reported posix timer related problems are gone - [iox#243] Segmentation Fault in posix Timer & RouDi stalls when posix timer is used in activation dispatcher
part 3 : full replacement of Posix Timer and Deletion from repo

Removing of the deprecated methods comes in part 3. where the timer will also be moved out of the posix wrapper package to a new class.

Is this okay?

dkroenke · 2021-01-08T10:32:49Z

I have 3 stories created for POSIX timer,
...
Removing of the deprecated methods comes in part 3. where the timer will also be moved out of the posix wrapper package to a new class.

Is this okay?

@shankar-bosch sounds like a plan, please add these three points to the issue #337 or ask @marthtz to do it, because he is the issue owner.

shankar-in · 2021-01-08T11:23:17Z

I have 3 stories created for POSIX timer,
...
Removing of the deprecated methods comes in part 3. where the timer will also be moved out of the posix wrapper package to a new class.
Is this okay?

@shankar-bosch sounds like a plan, please add these three points to the issue #337 or ask @marthtz to do it, because he is the issue owner.

@dkroenke Yes discussed with @marthtz during our daily.

elBoberido · 2021-01-08T11:27:12Z

could you also please remove the obsolete methods like hasExpiredComparedToCreationTime from the old timer class

I have 3 stories created for POSIX timer,
1. part 1 : Introduce alternate timers for keepalive and deadline

2. part 2 : Check that the already reported posix timer related problems are gone - [iox#243] Segmentation Fault in posix Timer & RouDi stalls when posix timer is used in activation dispatcher

3. part 3 : full replacement of Posix Timer and Deletion from repo
Removing of the deprecated methods comes in part 3. where the timer will also be moved out of the posix wrapper package to a new class.

Is this okay?

@shankar-bosch if this would be a big change I would agree, but since you already ported the code to the new class it doesn't make sense to keep the old code around and it's also not much to delete. I also object to merge this with the new class in the old file. It's not much work to move it to an own file and have it clean from the beginning.

If you plan t do bigger changes I always suggest to create one PR with only the new class and tests and another one with integration of the class and maybe removing of old code.

shankar-in · 2021-01-08T11:53:03Z

could you also please remove the obsolete methods like hasExpiredComparedToCreationTime from the old timer class

I have 3 stories created for POSIX timer,
1. part 1 : Introduce alternate timers for keepalive and deadline

2. part 2 : Check that the already reported posix timer related problems are gone - [iox#243] Segmentation Fault in posix Timer & RouDi stalls when posix timer is used in activation dispatcher

3. part 3 : full replacement of Posix Timer and Deletion from repo
Removing of the deprecated methods comes in part 3. where the timer will also be moved out of the posix wrapper package to a new class.
Is this okay?
@shankar-bosch if this would be a big change I would agree, but since you already ported the code to the new class it doesn't make sense to keep the old code around and it's also not much to delete. I also object to merge this with the new class in the old file. It's not much work to move it to an own file and have it clean from the beginning.

If you plan t do bigger changes I always suggest to create one PR with only the new class and tests and another one with integration of the class and maybe removing of old code.

@elBoberido
yes I agree. But we already have a plan so Can I check with team and @marthtz before i take up this activity?

elBoberido · 2021-01-08T12:27:47Z

@elBoberido
yes I agree. But we already have a plan so Can I check with team and @marthtz before i take up this activity?

@shankar-bosch sorry, but this is about getting clean code to master and it's not something that takes hours or days to accomplish. It's not additional functionality, just moving new code to a dedicated location and removing dead code.

Sorry if this sounds harsh. It's not intended.

shankar-in · 2021-01-08T13:42:25Z

@elBoberido
yes I agree. But we already have a plan so Can I check with team and @marthtz before i take up this activity?

@shankar-bosch sorry, but this is about getting clean code to master and it's not something that takes hours or days to accomplish. It's not additional functionality, just moving new code to a dedicated location and removing dead code.

Sorry if this sounds harsh. It's not intended.

@elBoberido Not at all. I was informed that POSIX timers goes for 1.0 which is scheduled for March with low prio. It was planned by splitting in parts during the refinement and take up across sprints.
Sorry if I am arguing too much and not taking up this review point right away. As a team member I have to communicate when there is a deviation in plan and get their feedback too.
I had a chat with @marthtz he is okay with this and I will be taking up this activity in the same PR.

elBoberido · 2021-01-08T13:56:17Z

@shankar-bosch so according to your plan this situation might last until march. That's one more reason not to merge this to master in the current form. This increases the technical dept for no good reason. What happens if you get assigned to another task or, hopefully not, get ill? Then this situation remains even longer.

Your PR is neither too big nor does is constantly get merge conflicts due to other PRs nor does it take long to accomplish the requested changes. Seriously, I don't understand the problem.

shankar-in · 2021-01-08T14:30:52Z

@shankar-bosch so according to your plan this situation might last until march. That's one more reason not to merge this to master in the current form. This increases the technical dept for no good reason. What happens if you get assigned to another task or, hopefully not, get ill? Then this situation remains even longer.
Your PR is neither too big nor does is constantly get merge conflicts due to other PRs nor does it take long to accomplish the requested changes. Seriously, I don't understand the problem.

@elBoberido I dont have. I agree with your points completely. Spoke with the team already. I am working on this review point now. Sorry for any confusion caused.

iceoryx_posh/source/roudi/roudi_process.cpp

iceoryx_posh/test/integrationtests/test_posh_mepoo.cpp

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

shankar-in · 2021-01-19T07:14:10Z

Please sign off all commits!

@elfenpiff I have signed all my commits pls verify and approve this PR

elfenpiff

Could you please add some additional test cases.

elfenpiff · 2021-01-19T11:33:58Z

iceoryx_utils/test/moduletests/test_cxx_deadline_timer.cpp

+const Duration DeadlineTimer_test::TIMEOUT{10_ms};
+const int DeadlineTimer_test::SLEEPTIME = DeadlineTimer_test::TIMEOUT.milliSeconds<int>();
+
+TIMING_TEST_F(DeadlineTimer_test, DurationOfNonZeroIsExpiresAfterTimeout, Repeat(5), [&] {


What if the Deadline timer has a timeout of zero? Is the Callback then called as fast as possible? We should also always keep the following test ideas in mind:

Zero

One

Many

Corner Cases

Limits

to cover every aspect of a class.

This is only the deadline timer use case. in case if the timer has a duration set to zero the end time will be the current time and the timer will be already expired. I can add a test case for this.

elfenpiff · 2021-01-19T11:37:08Z

iceoryx_utils/test/moduletests/test_cxx_deadline_timer.cpp

+    TIMING_TEST_EXPECT_FALSE(sut.hasExpired());
+});
+
+TIMING_TEST_F(DeadlineTimer_test, ResetWithCustomizedTimeAfterBeingExpiredIsNotExpired, Repeat(5), [&] {


Could you please write the same test for the case when the time is not being expired.

elfenpiff · 2021-01-19T11:37:40Z

iceoryx_utils/test/moduletests/test_cxx_deadline_timer.cpp

+});
+
+
+TIMING_TEST_F(DeadlineTimer_test, RemainingTimeCheckIfNotExpired, Repeat(5), [&] {


Could you please write a test where you verify that the remaining time is zero when the timer is expired.

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

iceoryx_utils/test/moduletests/test_cxx_deadline_timer.cpp

…ine timer Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

marthtz · 2021-01-21T10:22:28Z

iceoryx_utils/test/moduletests/test_cxx_deadline_timer.cpp

+    const int EXPECTED_REMAINING_TIME = PASSED_TIMER_TIME - RANGE_APPROX;
+
+    TIMING_TEST_EXPECT_TRUE(remainingTime >= EXPECTED_REMAINING_TIME && remainingTime <= PASSED_TIMER_TIME);
+});


Missing newline at end of file?

Added a new line to the end of file

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

shankar-in · 2021-01-27T05:25:21Z

@elfenpiff Could you pls check whether your review comments are addressed and approve if you are okay with the changes?

elfenpiff

@shankar-in Thank you for the good work.

shankar-in · 2021-01-27T17:27:53Z

@elBoberido Thanks a lot!

dkroenke requested a review from elBoberido January 5, 2021 17:12

elBoberido requested a review from mossmaurice January 5, 2021 18:37

elBoberido requested changes Jan 5, 2021

View reviewed changes

MatthiasKillat previously approved these changes Jan 6, 2021

View reviewed changes

dkroenke reviewed Jan 6, 2021

View reviewed changes

elBoberido reviewed Jan 6, 2021

View reviewed changes

dkroenke requested changes Jan 6, 2021

View reviewed changes

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp Outdated Show resolved Hide resolved

orecham requested changes Jan 7, 2021

View reviewed changes

shankar-in dismissed MatthiasKillat’s stale review via 1de4211 January 7, 2021 10:06

orecham reviewed Jan 7, 2021

View reviewed changes

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp Outdated Show resolved Hide resolved

orecham reviewed Jan 7, 2021

View reviewed changes

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp Outdated Show resolved Hide resolved

dkroenke requested changes Jan 7, 2021

View reviewed changes

elBoberido reviewed Jan 7, 2021

View reviewed changes

iceoryx_utils/source/posix_wrapper/timer.cpp Outdated Show resolved Hide resolved

iceoryx_utils/source/posix_wrapper/timer.cpp Outdated Show resolved Hide resolved

elBoberido reviewed Jan 7, 2021

View reviewed changes

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp Outdated Show resolved Hide resolved

iceoryx_utils/include/iceoryx_utils/posix_wrapper/timer.hpp Outdated Show resolved Hide resolved

iceoryx_utils/test/moduletests/test_posix_mutex.cpp Show resolved Hide resolved

elBoberido reviewed Jan 7, 2021

View reviewed changes

mossmaurice previously approved these changes Jan 15, 2021

View reviewed changes

iceoryx_posh/source/roudi/roudi_process.cpp Outdated Show resolved Hide resolved

iceoryx_posh/test/integrationtests/test_posh_mepoo.cpp Show resolved Hide resolved

dkroenke added the enhancement New feature label Jan 15, 2021

iox-eclipse-iceoryx#337 rename the file for better readability

4c801b3

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

shankar-in dismissed stale reviews from mossmaurice, dkroenke, and elBoberido via 4c801b3 January 16, 2021 10:00

elBoberido previously approved these changes Jan 16, 2021

View reviewed changes

dkroenke self-requested a review January 18, 2021 09:15

dkroenke previously approved these changes Jan 18, 2021

View reviewed changes

orecham previously approved these changes Jan 18, 2021

View reviewed changes

nihalchari requested a review from elfenpiff January 19, 2021 06:52

elfenpiff requested changes Jan 19, 2021

View reviewed changes

iox-eclipse-iceoryx#337 add corner test cases for deadline timer

c4c6c06

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

shankar-in dismissed stale reviews from orecham, dkroenke, and elBoberido via c4c6c06 January 20, 2021 17:09

elBoberido reviewed Jan 20, 2021

View reviewed changes

iceoryx_utils/test/moduletests/test_cxx_deadline_timer.cpp Show resolved Hide resolved

iox-eclipse-iceoryx#337 enhance the test to check the status of deadl…

b516e0b

…ine timer Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

elBoberido previously approved these changes Jan 20, 2021

View reviewed changes

marthtz reviewed Jan 21, 2021

View reviewed changes

iox-eclipse-iceoryx#337 add new line to end of file

5e0d4ba

Signed-off-by: Sankara Narayanan Chandrasekar (RBEI/EMT2) <[email protected]>

shankar-in dismissed elBoberido’s stale review via 5e0d4ba January 27, 2021 05:24

elBoberido approved these changes Jan 27, 2021

View reviewed changes

elfenpiff approved these changes Jan 27, 2021

View reviewed changes

elBoberido added 2 commits January 27, 2021 15:57

Merge branch 'master' into iox-eclipse-iceoryx#337-deprecate-posix-timer

ecab381

Merge branch 'master' into iox-eclipse-iceoryx#337-deprecate-posix-timer

4131459

elBoberido merged commit b3ae7a4 into eclipse-iceoryx:master Jan 27, 2021

		});


		TIMING_TEST_F(DeadlineTimer_test, RemainingTimeCheckIfNotExpired, Repeat(5), [&] {

Iox #337 deprecate posix timer #479

Iox #337 deprecate posix timer #479

Conversation

nihalchari commented Jan 5, 2021 • edited by elBoberido Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

Post-review Checklist for the Eclipse Committer

References

shankar-in commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthiasKillat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido commented Jan 6, 2021

orecham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shankar-in commented Jan 7, 2021

shankar-in commented Jan 7, 2021

dkroenke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

nihalchari commented Jan 8, 2021

shankar-in commented Jan 8, 2021

dkroenke commented Jan 8, 2021

shankar-in commented Jan 8, 2021

elBoberido commented Jan 8, 2021

shankar-in commented Jan 8, 2021 • edited Loading

elBoberido commented Jan 8, 2021 • edited Loading

shankar-in commented Jan 8, 2021

elBoberido commented Jan 8, 2021

shankar-in commented Jan 8, 2021

shankar-in commented Jan 19, 2021

elfenpiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shankar-in commented Jan 27, 2021

elfenpiff left a comment

Choose a reason for hiding this comment

shankar-in commented Jan 27, 2021

nihalchari commented Jan 5, 2021 •

edited by elBoberido

Loading

shankar-in commented Jan 8, 2021 •

edited

Loading

elBoberido commented Jan 8, 2021 •

edited

Loading