Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace existing RealtimeBox implementation with RealtimeBoxBestEffort implementation #146

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

firesurfer
Copy link
Contributor

@firesurfer firesurfer commented Jan 25, 2024

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
Copy link

codecov-commenter commented Jan 31, 2024

Codecov Report

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

Project coverage is 75.36%. Comparing base (a11e9cf) to head (02df04a).

Files with missing lines Patch % Lines
include/realtime_tools/realtime_box.h 90.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   75.36%   75.36%           
=======================================
  Files           8        7    -1     
  Lines         341      341           
  Branches       62       62           
=======================================
  Hits          257      257           
  Misses         50       50           
  Partials       34       34           
Flag Coverage Δ
unittests 75.36% <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%) ⬇️

@firesurfer
Copy link
Contributor Author

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

@firesurfer
Copy link
Contributor Author

Fixes #138

@firesurfer
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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_)

@firesurfer
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Member

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

@saikishor
Copy link
Member

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
Copy link
Contributor Author

firesurfer commented Apr 5, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

@firesurfer
Copy link
Contributor Author

Just a friendly ping :)

* @return false if the mutex could not be locked
* @note only safe way to access pointer type content (rw)
*/
bool trySet(const std::function<void(T &)> & func)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool trySet(const std::function<void(T &)> & func)
bool try_set(const std::function<void(T &)> & func)

we've been following snake-case in ros2_control, please adjust the rest too

Copy link
Contributor Author

@firesurfer firesurfer Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I am a bit puzzled about this comment given that the PR that introduced these names was accepted #139

This PR ist just for replacing the old RealtimeBox with the new implementation while still keeping it backward compatible with the corresponding deprecation warnings.

Btw. @christophfroehlich As far as I know it is possible with clang-tidy to enforce such code styles. Perhaps you might want to look into that :)

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@firesurfer few minor comments.

Thank you and sorry for the huge delay

Comment on lines +247 to +248
template <class T, typename mutex_type>
class RealtimeBox<T, mutex_type, true>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template <class T, typename mutex_type>
class RealtimeBox<T, mutex_type, true>
template <class T, typename mutex_type>
[[deprecated("RealtimeBox is not recommended to use with Raw pointers! Use smart pointers instead.")]]
class RealtimeBox<T, mutex_type, true>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should actually be: Only use the "get/set(std::function) overloads of the RealtimeBox (which is the reason why we added this template specialisation for pointers).

But that deprecation message is already printed when the default get/set methods are used with pointers!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it wiser to mark the class as deprecated?, as you know from the beginning that type is not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should give a quick recap why it is designed like this given that quite some time has passed now:

  1. The existing RealtimeBox implementation is not safe to use with any kind of pointer. Because only any operation on the pointer itself would be locked by the mutex but not the access to the data behind the pointer

  2. The RealtimeBoxBestEffort solves this issue by providing additional overloads to get/set which take a function. These overloads lock the mutex and call the function. Inside this function manipulating the pointer is safe.
    Additionally if the Type is a pointer the normal get/set methods which return a reference to the internal value are disabled via SFINEA (std::enable_if)

  3. It makes sense to replace the existing RealtimeBox with the RealtimeBoxBestEffort (This is what this PR aims for). Additionally the requirement was made: We shall not break existing code, but we want to display deprecation notices if the default get/set methods are used with pointers. Therefore I added a template specialisation for pointers removes the std::enable_if for get/set with pointer types and adds deprecation notices for the get/set methods without the std::function overloads.

This should encourage users to migrate to the get/set(std::function) version for pointer types.
So in a future release the pointer specialisation of the class can be removed.

So to answer your question:

Pointer types are supported BUT you need to use the get/set(std::function) methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@firesurfer I understand your point. What I'm saying is, we don't have to break the Raw pointer in this version, but in the future versions, we can make the users use the smart pointers. So, if we mark the class deprecated with the notice to use the smart pointers in future releases, the users will be able to migrate in the meantime.

If we mark the methods alone, they still think the Raw pointers will be supported in the future. I'm not sure, if it is interesting to support Raw pointer in the future. If we still want to support, then the way you marked the depreciation is good enough @bmagyar @christophfroehlich any comments on this?

* the value behind the pointer
*/
template <class T, typename mutex_type>
class RealtimeBox<T, mutex_type, true>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@firesurfer In this overlayed class, do we need to have a copy of most of the methods similar to the previous one? or is there a way to reuse them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saikishor From the "base" implementation:

// Provide a specialisation for non pointer types
// NOTE: When migrating to a safe access only version just remove the specialisation for pointer
// and let this be the only version!

My argument would be that in the longterm the pointer specialisation should be removed. I think the only save way to access the values behind pointers is to use the "get/set(std::function) overloads).
By keeping the code duplicated we can simplify this process.

My suggestion is to keep this version for at least one release and remove the pointer specialisation in the next one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm! @bmagyar @christophfroehlich any strong opinion on this one?

Comment on lines +145 to +146
// This does not and should not compile!
// auto value = box.get();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This does not and should not compile!
// auto value = box.get();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the moment this would compile btw. (Because we added the pointer specialisation for compatibility reasons)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, then let's remove the comment.

test/realtime_box_tests.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants