Cleaning and extending yarp::os::RateThread

[diegoferigo]

Attempting to debug the hardware fault described in https://github.com/robotology/icub-support/issues/449, I'm digging into the RateThread & friends code. Considering git-blame the last major edits look very old, and in the meantime several new features (including C++11 support) have been included into yarp.

I'm considering to clean and comment the code, and to enhance the statistic analysis. I'm aware this is a very delicate section and before proceeding I'd like to know if something blocking on this portion of code exists.

cc @drdanz @lornat75 @traversaro

[barbalberto]

I'm currently working right now on RateThread in the process of refactoring the way clock and network clock are handled in yarp. While doing so I've already promoted the timing to use c++11 features as @drdanz started to do on other part of yarp. Yes, this is a critical part of code, so it'll requires estended review and testing. If you want we can chat a bit to avoid conflicts

[diegoferigo]

Yes we can chat f2f on how to proceed on this.

[randaz81]

Known issues about C++11 (possibile loose connections to this issue): https://github.com/robotology/yarp/issues/1064 https://github.com/robotology/yarp/issues/1063 https://github.com/robotology/yarp/issues/1061 https://github.com/robotology/yarp/issues/1035

[lornat75]

Hi @diegoferigo, thank you for your help. Extending the statistics may be useful (just consider performance hit given that a run() function is called at high rate), which features do you think may be important?

About the code refactoring, I tend to avoid touching code that works... the fact that it was not touched in many years is a positive sign :) however, I am not against improvements, therefore I am open to discussions.

[diegoferigo]

@lornat75 After chatting with @barbalberto, which is refactoring yarp::os::RateThread to the most recent timing features shipped with yarp, we concluded that a possible contribution from my side could be adding an additional yarp::os::ThreadStats (that I'm currently developing) that implements all the statistic currently running in yarp::os::RateThreadCallbackAdapter. The idea is to be able to use this new class inside RateThreadCallbackAdapter for calculating the default statistic of the entire run(), and allowing using it for debugging purposes in a more general scenario (e.g. calculating the elapsed time of a portion of code inside run() or other functions that are called with a fixed rate, in other words a generic tick()-tock() implementation). Our work is going on in parallel.

The advanced statistics I'm developing is optional and will be similar to what @marcoaccame implemented in the icub-main:checkFreqOfTRQmeasures branch. I'll keep backward compatibility and I don't plan to add any overhead to the current implementation.

Initially, this class will be used for debugging https://github.com/robotology/icub-support/issues/449, and hence it will be tested externally to RateThread. Then, when the refactoring will be ready, we can merge these two parallel branches and start the discussion for their merge into yarp (definitely after the next yarp release).

Any comment about?

[jgvictores]

Small comment regarding current master/develop state of RateThread, and fits under the Cleaning and extending yarp::os::RateThread description:

It's apparently been there for over 12 years now, but I've only recently detected the apparent type inconsistency between rate [ms] constructor RateThread(int period); and setter bool setRate(int period);, vs getter double getRate();. I understand get should return the set rate so there type should match (there are other methods to retrieve estimates, e.g. getEstPeriod(), and it is also the standard semantics).

Via examples such as this one (gist) I've seen Windows 10 with system_clock precision = 0.1 microseconds/tic and Ubuntu Xenial with system_clock precision = 0.001 microseconds/tic, so maybe change the constructor and setter to double is reasonable (not a breaking change anyway)?

PS: Is/should setRate() be thread-safe (IMHO, should be documented)?

[jgvictores]

apparent type inconsistency between rate [ms] constructor RateThread(int period); and setter bool setRate(int period);, vs getter double getRate();

This and more done at #1718

[Nicogene]

We have to check if #1718 fixed one or more issues related to RateThread :thinking: