Iox #489 replace introspection threads with periodic task

[elBoberido]

Pre-Review Checklist for the PR Author

1. Branch follows the naming format (iox-#123-this-is-a-branch)
2. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit messages are signed (git commit -s)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
3. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
4. Relevant issues are linked
5. Add sensible notes for the reviewer
6. All checks have passed
7. Assign PR to reviewer

Notes for Reviewer

This PR replaces the thread in the introspection classes with a PeriodicTask and simplifies the code. The test time with the RouDiEnvironment will also be reduced a little bit (1s posh_modultetests and 2s in posh_integrationtests), since the stopping of the threads is not done by a polling with 100ms period.

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

1. All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

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

References

• Closes Replace introspection threads with PeriodicTask #489 and popo/user_trigger.hpp:33:5: error: exception specification of explicitly defaulted default constructor does not match the calculated one #494

[dkroenke]

Two questions:

1. Could be the TimeSpecReference in the run() method maybe Monotonic? I think the timeout interval does not depend on the current value of time.

2. This is maybe for future use: For validation purpose the user maybe wants to know how often the task was executed. A getter method like uint64_t getExecutionCount() could be helpful for tests.

[elBoberido]

@dkroenke I will have a look if a monotonic clock is possible with the semaphore.

Regarding the uint64_t getExecutionCount(). In tests, you can always add a counter to the function which will be executed. This could be helpful for other situations though. I would consider it as low prio and create an issue for this. This is a good first issue for a new contributor. Do you agree?

[elBoberido]

@dkroenke regarding the monotonic clock, I just checked the the semaphore documentation and an absolute timeout is required, therefore we cannot use a monotonic clock

[MatthiasKillat]

Improves the sending process of introspection data. I just would consider to use chrono (why would we not use it?) . Tests are missing but this is a draft anyway.

[elBoberido]

Regarding the uint64_t getExecutionCount(). In tests, you can always add a counter to the function which will be executed. This could be helpful for other situations though. I would consider it as low prio and create an issue for this. This is a good first issue for a new contributor. Do you agree?

done with #502

[dkroenke]

@elfenpiff 350cb50 has no iox number.

[mossmaurice]

using namespace iox::units::duration_literals will enable you to use the user-defined literal operation 1_s Locally in a class it should not lead to evil namespace polution.

Reminds me of plan to clean up the using namespace in iceoryx_posh_types.hpp 😋

[elBoberido]

that's unfortunately not possible within a class

[mossmaurice]

Oh, you're right. One thing I saw when checking the coreguidlines. They explicitly exclude using namespace std::units::duration_literals, see. So we might leave them in headers but probably not in iceoryx_posh_types.hpp as this will increase lookup times.

[mossmaurice]

What about adding a test case that call start() again while the thread/task is still running?

[elBoberido]

that's what PeriodicTaskWhichIsActiveAppliesNewIntervalAfterStart is doing

[mossmaurice]

Hmm, ok you're calling start() indirectly via the other c'tor. That's what I missed. Could we make the user more aware that the behaviour of the two c'tors is very different? One does start the other not. I have the feeling folks will do the same mistake as I in the near future (and my future self, too).

[elBoberido]

Any suggestions? An @attention doxygen tag?

[mossmaurice]

I'm torn inbetween. What about removing the "automatic start c'tor"? An explicit start call might be simpler for most users.

[elBoberido]

Can do that, but then you have the opposite problem to not forget to start it. I'm also not quite sure about this.

Just as food for thought, how about something like this

class PeriodicTask {
    enum class OnInit {
        STOPPED,
        RUNNING
    }

    PeriodicTask(OnInit onInit, ...)
};

PeriodicTask task{PeriodicTask::OnInit::RUNNING, ...}

It could also be the separator between the actual PeriodicTask args and the variadic args forwarded to the callable and thus help prevent ambiguities.

[mossmaurice]

I like your approach, but it might create too much noise. There can be already quite a lot of args. If we leave out the "automatic start c'tor" won't the user notice this right away and then add start?

[elBoberido]

well, I think it will take as much time as when starting immediately and noticing it's not what you wanted. Maybe even longer because noticing that something is not happening might take longer to realize.
Maybe we also need input from others :D

[elBoberido]

added a struct to indicate the automatic or manual start of the task

[mossmaurice]

🥳

[elBoberido]

only 5 remaining. Those also need to go the way of the dodo in order to support Windows.

[elBoberido]

oh, and could the one of the reviewer please take care to the reviewer task list ;)

[sculpordwarf]

@elBoberido This will generate a warning in more strict builds:
https://rextester.com/live/WANPI61259
-Weverything

504430806/source.cpp:7:32: warning: extra ';' after member function definition [-Wextra-semi]
    PublisherInfo() noexcept {};
                               ^

[elBoberido]

@sculpordwarf thanks for the hint. I'll fix it with my next PR. I'm wondering why our CI didn't catch it