new realtime_buffer-like class that works for reading and writing to …

[guru-florida]

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?

[bmagyar]

@matthew-reynolds could you give this a second review please?

[bmagyar]

@guru-florida ping

[matthew-reynolds]

This is partly waiting on action from me, the jumbo dump of comments was a bit unmanagable. I'll try to put up a PR against the feature branch this weekend.

[guru-florida]

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.

This is partly waiting on action from me, the jumbo dump of comments was a bit unmanagable. I'll try to put up a PR against the feature branch this weekend.

[YoavFekete]

So what the state of this change?? any help needed to complete it ??

[bmagyar]

AFAIK only following through with the comments mentioned above.

[YoavFekete]

guru-florida

Any advance on your side??? i really want to use this class :) and i am sure i am not the only one

[guru-florida]

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.

[destogl]

@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 pre-commit to fix the formatting.

[codecov-commenter]

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@e2bb82b). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #73   +/-   ##
=========================================
  Coverage          ?   51.62%           
=========================================
  Files             ?       14           
  Lines             ?      769           
  Branches          ?      359           
=========================================
  Hits              ?      397           
  Misses            ?       40           
  Partials          ?      332           

Flag	Coverage Δ
unittests	51.62% <0.00%> (?)

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.