Replace existing RealtimeBox implementation with RealtimeBoxBestEffort implementation

[firesurfer]

Hi as said in #139 this PR replaces the existing RealtimeBox implementation with the new implementation provided in #139 . I merged the tests into a single file and they work fine (even though the existing implementation had only a very small amount of tests)

The implementation of the get/set methods matches exactly the one from the existing implementation apart from some template magic which disables the standard get/set methods for pointer types. (For which they wouldnt provide any thread safety btw from my point of view - see #138)

TLDR: Merging this has the potential to break existing code if someone used pointer types with the RealtimeBox. For everything else it should work just fine.

I would also be happy to provide some documentation on how to use this implementation but I am not sure where in the repository it should go.

This PR should be merged after #139

EDIT: One question regarding the copyright notice for the existing tests. How should this be handled? At the moment I didn't copy any of it to the merged file.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.37%. Comparing base (b6552e4) to head (85a421e). Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
include/realtime_tools/realtime_box.h	90.00%	2 Missing and 2 partials :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   76.37%   76.37%           
=======================================
  Files           7        6    -1     
  Lines         237      237           
  Branches       40       40           
=======================================
  Hits          181      181           
  Misses         26       26           
  Partials       30       30           

Flag	Coverage Δ
unittests	76.37% <90.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
include/realtime_tools/realtime_box.h	90.00% <90.00%> (-10.00%)	:arrow_down:

[firesurfer]

@christophfroehlich I will update this PR as soon as #139 is finished

[firesurfer]

Fixes #138

[firesurfer]

@christophfroehlich Given that #139 is now merged I updated this PR. Please see my notes in the initial description about open points for discussion.

Also pinging @saikishor

[firesurfer]

@christophfroehlich Just a note about the CI. Interestingly I just had the case that pre-commit in the CI failed but did run sucessfully on my system. (Note about using the right include guard - REALTIME_TOOLS__REALTIME_BOX_H_)

[christophfroehlich]

@christophfroehlich Just a note about the CI. Interestingly I just had the case that pre-commit in the CI failed but did run sucessfully on my system. (Note about using the right include guard - REALTIME_TOOLS__REALTIME_BOX_H_)

Strange. Have you run pre-commit run --all? otherwise it checks only the changed files in the index.

[firesurfer]

Strange. Have you run pre-commit run --all? otherwise it checks only the changed files in the index.

Yes that's what I did run. But I couldn't reproduce it now. .

[christophfroehlich]

I would also be happy to provide some documentation on how to use this implementation but I am not sure where in the repository it should go.

We don't have a RST documentation of this repo, but I plan to rework the API documentation somewhen on control.ros.org. For now, please just add anything in the manner of rosdoc2 to be published on index.ros.org

[saikishor]

Is there a way to add some methods with old pointers and deprecation warnings?

[saikishor]

Is there a way to add some methods with old pointers and deprecation warnings?

if this is not possible, I don't think it is nice to completely override it and force everyone to change. If we want this to be the default implementation, then we would need to add a deprecation message to the old one and ask everyone to switch to the new one.

[firesurfer]

The current implementation in the RealTimeBoxBestEffort basically disables the normal trySet, tryGet...etc methods for pointer types.

Given that it is inherently unsafe to use pointers with this implemenation and the default trySet,tryGet methods (and also the existing RealTimeBox implementation) one could argue that it could be profitable to actually break builds here.

Nevertheless I completely understand your argument.

I guess adding a deprecation notice might be possible but I am not sure what is the best way at the moment. We could use SFINEA to provide a version of the class for pointers that include the deprecation notices (and removes the typename std::enable_if_t<!is_ptr_or_smart_ptr<U>, U> for the normal trySet, tryGet methods).

In the end I think this really comes to the question: Allow wrong behavior or possibly break builds?

[saikishor]

The current implementation in the RealTimeBoxBestEffort basically disables the normal trySet, tryGet...etc methods for pointer types.

Given that it is inherently unsafe to use pointers with this implemenation and the default trySet,tryGet methods (and also the existing RealTimeBox implementation) one could argue that it could be profitable to actually break builds here.

Nevertheless I completely understand your argument.

I guess adding a deprecation notice might be possible but I am not sure what is the best way at the moment. We could use SFINEA to provide a version of the class for pointers that include the deprecation notices (and removes the typename std::enable_if_t<!is_ptr_or_smart_ptr<U>, U> for the normal trySet, tryGet methods).

In the end I think this really comes to the question: Allow wrong behavior or possibly break builds?

I already agree with you on the unsafe part from the other PR, the problem here is we have a single branch that runs for Rolling, Iron, and also Humble, so breaking it would be breaking the setup of a lot of users, and such API breaking is usually not seen good.

I could think of the following 3 options:

• Branch the rolling from others and then make this RealTimeBoxBestEffort as default over there.
• Add a deprecation notice on the current one and ask users to move to the updated one this deprecation can be completely removed after one ROS release, so the users can migrate gradually.
• We can somehow try to wrap the raw pointer of the user to a smart pointer and then use the current class.

What do you think about this?

[firesurfer]

Branch the rolling from others and then make this RealTimeBoxBestEffort as default over there.

This introduces additional maintenance effort -> I can not judge if this would be worth the effort

Add a deprecation notice on the current one and ask users to move to the updated one this deprecation can be completely removed after one ROS release, so the users can migrate gradually.

According to https://stackoverflow.com/questions/48045559/how-do-i-declare-sfinae-class I could do that. The idea would be: Provide a specialisation for pointers, enable the methods there and add deprecation notes to them

I am currently trying to figure out a way to do this

We can somehow try to wrap the raw pointer of the user to a smart pointer and then use the current class.

This does not solve the problem. As the aim of this implementation is to provide thread safe access to its contents the access to any pointer type has to be wrapped in the way shown in the examples. (Using the shared_ptr does not make it safer than using a raw pointer)

[firesurfer]

@saikishor I just pushed a version which add a specialisation for pointer types. The tests will now show a deprecation note if get/set are used with pointer types. Also the code starts looking like a stl class...not sure if thats a good thing :D

[christophfroehlich]

EDIT: One question regarding the copyright notice for the existing tests. How should this be handled? At the moment I didn't copy any of it to the merged file.

Please just add another line to the copyright claim and don't remove the old one if old code remained inside (at least I saw this in other ROS repos)

[firesurfer]

@saikishor Perhaps you'll find some time for another review? It will now handle pointers types as before but print a deprecation warning

[firesurfer]

Just a friendly ping :)