new realtime_buffer-like class that works for reading and writing to … #73
Conversation
|
@matthew-reynolds could you give this a second review please? |
There was a problem hiding this comment.
I haven't looked at the tests, but I had a bunch of thoughts on the main implementation here. I tried to get ros2_controllers JTC converted to this, but I hit a roadblock with the WriteBarrier::push() calling swap(), I think that's probably the most critical change. The rest is mostly stylistic things or potential ways to simplify.
| { | ||
| public: | ||
| using MemoryBarrierType = MemoryBarrier<T>; | ||
|
|
There was a problem hiding this comment.
What happened to the DirectAccessType alias here? Using the full decltype(my_barrier)::MemoryBarrierType::DirectAccess<> each time is a bit cumbersome, it would be nice to have the decltype(my_barrier)::DirectAccessType
There was a problem hiding this comment.
If we move DirectAccess out of MemoryBarrier and into the top-level realtime_tools namespace, as I suggested below, this comment is irrelevant.
| @@ -0,0 +1,468 @@ | |||
| /* | |||
There was a problem hiding this comment.
Let's name this file .h for consistency with the rest. I know the ROS2 style is to prefer .hpp for C++ stuff, but I think matching the rest of the repo is probably the higher priority for now
There was a problem hiding this comment.
As Ros2 style is to prefer .hpp I think if anything the other files should be migrated over to .hpp in a seperate PR. I'm not totally committed to it, but that's my thoughts.
| { | ||
|
|
||
| /// @brief Attempt a lock on a resource but fail if already locked | ||
| class try_lock : public std::unique_lock<std::mutex> |
There was a problem hiding this comment.
I think we should stick with CamelCase for these. Even though they're pretty thin wrappers around STL stuff, and STL stuff uses snake_case, I find it very disorienting to have user-defined classes that don't match the normal naming conventions.
There was a problem hiding this comment.
Same for the structs, template params, using aliases, etc below.
There was a problem hiding this comment.
I prefer consistency as well. I did default to STL snake_case because it was based on unique_lock but your argument is sound so it can be changed to TryLock.
|
|
||
| /// @brief Read data out of the realtime thread | ||
| /// Swap RT buffer for non-RT. Copy the new data into val and reset the new_data_available flag. | ||
| // todo: since this read also swaps, should it be called read_and_swap() or pop() or pull() |
There was a problem hiding this comment.
I don't think we need a rename. The consumer should always try to swap, I think that's well enough understood.
| : mem_(&mem_barrier) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
The WriteBarrier is missing the owns_mem and destructor from ReadBarrier
| } // namespace realtime_tools | ||
|
|
||
|
|
||
| #endif // REALTIME_TOOLS__REALTIME_BARRIER_HPP_ |
There was a problem hiding this comment.
Should be 2 spaces after end of line, before comment. I think this fix is needed in a handful of places
| /// default constructor will create a new memory barrier for use. There is an alternate | ||
| /// constructor if you want to create your own MemoryBarrier explicitly. | ||
| template<class T, class locking_strategy = wait_lock> | ||
| class WriteBarrier |
There was a problem hiding this comment.
Does it make sense to maybe refactor this to have the ReadBarrier and WriteBarrier both inherit from some base class? They're almost identical except for the push/pull.
I feel that inheriting from MemoryBarrier instead of composing it would make this simpler. What do you think?
There was a problem hiding this comment.
I had originally gone in this direction. I can't recall why I switched to composition. It may be because MemoryBarrier is the single object that facilitates passing in one direction, but the ReadBarrier and WriteBarrier are each just one facade of the MemoryBarrier. Like MB is the whole coin, where RB and WB are the heads-side and tails-side. The interfaces are very similar but not identical. IRC The idea is you create one MemoryBarrier, then you get a RB on one thread and a WB on the other thread from the MB. So in that sense a MemoryBarrier is a stand-alone object not owned by either RB or WB.
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
Co-authored-by: Matt Reynolds <[email protected]>
|
@guru-florida ping |
|
This is partly waiting on action from me, the jumbo dump of comments was a bit unmanagable. |
|
That would be greatly appreciated Matt! Feel free to make all the grammar and comment changes and put into 1 commit. I can then review any code changes with you separately. IRC there were 2 good bug fixes you had.
|
|
So what the state of this change?? |
|
AFAIK only following through with the comments mentioned above. |
|
Any advance on your side??? i really want to use this class :) |
|
I can review again this week and make sure the 2 issues are fixed. It's been a while since I was in the code. |
|
@guru-florida I change the merge target to master and we can then back port it to Foxy if needed. P.S. maybe you will have to use |
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
=========================================
Coverage ? 51.62%
=========================================
Files ? 14
Lines ? 769
Branches ? 359
=========================================
Hits ? 397
Misses ? 40
Partials ? 332
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
From what I can tell, there are some issues with using realtime_tools::RealtimeBuffer in our ros2_control hardware interfaces. It seems this RealtimeBuffer is only useful for writing to a realtime based on the use of the new_data_available internal variable. For reading state from the realtime thread we dont have an existing RealtimeBuffer.
The internal implementation of the "swapping double buffer pointers" works for both modes, only the interface (read vs write) to this swapping buffer needs to change. Therefore I refactored the swapping double buffers out into the MemoryBarrier class, which also defines a sub-type called DirectAccess<> to protect direct access to the buffers.
The realtime thread is expected to use the DirectAccess class to get direct access to the buffers without memory copies. However, the non-realtime thread should use either WriteBarrier or ReadBarrier. These two classes control the direction of data flow to/from the realtime thread, respectively. IMO It makes sense that the reads and writes be through separate class objects and not via some bi-directional interface.
This code is based on a git gist I did here. It also has some more explanation.
This class could fully replace functionality of realtime_buffer, and perhaps it should except that it's interface isn't compatible and will break dependant packages. I leave that up to discussion. We can keep both and confuse new users, or perhaps we could move existing realtime_buffer to a realtime_tools::legacy namespace and then rename "barriers" here to "buffer". At least dependant packages would have a quick fix. Thoughts?