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

Roadmap and guidelines for using Box/Buffer/Queue #279

Open
christophfroehlich opened this issue Feb 10, 2025 · 5 comments
Open

Roadmap and guidelines for using Box/Buffer/Queue #279

christophfroehlich opened this issue Feb 10, 2025 · 5 comments
Labels

Comments

@christophfroehlich
Copy link
Contributor

christophfroehlich commented Feb 10, 2025

We want to summarize the differences between RealtimeBox and RealtimeBuffer, and compare it with the new Lock-Free Queue proposal.
In the end, we should have guidelines when to use which concept and if maybe any of them is obsolete.

Finally, we will apply the guidelines to ros2_controllers wherever necessary.

General

Within the ros2_control framework, we have only the following use-cases

RealtimeBox

Got refurbed with #139 #146

[Sai]

  • It is more a thread-safe way to accessing data than RT safe.
  • Using pointers with this doesn't make a lot of sense as the data protected by the mutex or the wrapper can be modified and accessed without any restrictions. IMO, it shouldn't be the case with the RealtimeBox

RealtimeBuffer

[Sai] RealtimeBuffer is what we mostly want, but we have some cons in this:

  • As we all agreed in last WG meetings, NON_POLLING should be the default one now and it should work as expected Realtime Publisher - Select one behavior (Polling or NonPolling) and remove the other one #212
  • The member variables are using the pointer to the original data (We have to check what happens if the data goes out of scope and we try to access it)
  • When accessing using the readFromRT etc, we get a raw pointer, may be this is not so nice.
  • The other limitation I see is that the Buffer size is limited to size of 2.

[Denis] We also need “WriteFromRT”

Lock-Free Queue

Was suggested in #14 and implemented with #253

[Sai]

  • We don't use mutexes for the implementation
  • We can have the buffer size as we need
  • Can work with multi producer and multi consumer and single producer and single consumer efficiently
  • and when we pop, we directly get the data instead of the pointer to the object

Example usage: ros-controls/ros2_controllers#1480

@christophfroehlich
Copy link
Contributor Author

👀 Could you have a look please, and add your thoughts? And feel free to ping other developers if you think they can provide valuable input to this topic.
@destogl @bmagyar @firesurfer @fmauch @urfeex @saikishor @RobertWilbrandt

@christophfroehlich
Copy link
Contributor Author

christophfroehlich commented Feb 12, 2025

Summary from WG meeting on February 12th:

  • Rename RealtimeBox to RealtimeThreadsafeBox and use RealtimeMutexes @saikishor
  • Check if we have a correct implementation for getting/setting pointers in the current RealtimeBox code, or prohibit using pointers @saikishor
  • Deprecate RealtimeBuffer and replace its usage with Lock-Free Queue
  • Do we need SPSC in queue, or make MPMC the default? Update references from subscriber is the only use case for SPSC, for the sake of simplicity for the users, is it worth keeping both? We keep both, but emphasize the difference in the documentation.
  • Add documentation of the difference concepts. @saikishor @christophfroehlich

@saikishor
Copy link
Member

Thanks for the consolidated report @christophfroehlich

@firesurfer
Copy link
Contributor

@christophfroehlich Sorry for my late comment. I was awfully occupied the last weeks.

RealtimeBox

  1. I like the renaming to RealtimeThreadsafeBox even though the name becomes a bit long
  2. The Box explicitly handles pointers e.g. void get(const std::function<void(const T &)> & func). The other get/set function overloads are disabled for pointers via SFINEA.
  3. RT Safety: If used with the try_set/try_get methods the box should behave just fine

RealtimeBuffer

I am not sure if replacing it with a (Lock-Free) queue implementation is the right thing to do. The buffer is (from my understanding) intended to move a value from "non-rt" context to the "rt" context and not for queuing in multiple values for processing into the "rt" context. If the overhead of using a queue implementation is neglectable adding a typedef for the RealtimeBuffer with a maximum queue size of 1 might be a good idea.

PS: I just saw the LockFreeQueue is based on boost - a dependency many people try to avoid - so perhaps keeping the Buffer is a good idea

RealtimePublisher

What is missing from my point of view in the list of actions is at least the RealtimePublisher.
The API is still kind of "clunky". In the best case the RT Publisher would be as similar as possible to the rclcpp::Publisher but simply adds a tryPublish method (or any other way of RT safe publishing).
From what I've read btw. are most middleware implementations already RT safe when it comes to the Publisher but it is not guaranteed.

@christophfroehlich
Copy link
Contributor Author

@firesurfer regarding RTPublisher: you are right, but I haven't listed it here because it has a very special purpose and its usage is clear.

regarding boost: we already have the dependency of libboost-dev through filters and pal_statistics lib.

maybe we can leave a RealtimeBuffer specialication of the queue with size of one as you suggested. but only if it simplifies the usage for users, we'll have to look into that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants