Add new API for the RealtimePublisher

[saikishor]

This should avoid any dangling threads of the RealtimePublisher and handling lock and unlock is not safe in most of the situation, so, I've added can_publish and try_publish methods

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.48649% with 5 lines in your changes missing coverage. Please review.

Project coverage is 87.06%. Comparing base (b79a4fa) to head (1ef422f).

Files with missing lines	Patch %	Lines
...ools/include/realtime_tools/realtime_publisher.hpp	78.57%	1 Missing and 2 partials ⚠️
realtime_tools/test/realtime_publisher_tests.cpp	91.30%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   86.92%   87.06%   +0.13%     
==========================================
  Files          17       17              
  Lines        1262     1291      +29     
  Branches      102      106       +4     
==========================================
+ Hits         1097     1124      +27     
+ Misses         98       97       -1     
- Partials       67       70       +3     

Flag	Coverage Δ
unittests	87.06% <86.48%> (+0.13%)	⬆️

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

Files with missing lines	Coverage Δ
realtime_tools/test/realtime_publisher_tests.cpp	88.57% <91.30%> (+1.33%)	⬆️
...ools/include/realtime_tools/realtime_publisher.hpp	89.79% <78.57%> (+1.42%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[saikishor]

@Juliaj This is the new API, could you please take a look when you have time :)

[christophfroehlich]

should we start migration notes for this repo?

[Juliaj]

We may still need the deprecated lock() and unblock() to handle the logic in
ForceTorqueSensorBroadcaster::on_configure which seems to be different than publishing a msg. In on_configure , only part of a message (e.g., msg_.header) needs modification without immediate publishing.

 realtime_publisher_->lock();
 realtime_publisher_->msg_.header.frame_id = params_.frame_id;
 realtime_publisher_->unlock(); 

An alternative would be making a similar safer version for modifying message field.

[saikishor]

Thanks for pointing it out. I don't think we would need that, because we can use the get_mutex() and directly use the unique_lock there which is much better than the current approach.

On the other hand, I want the users to not directly access the Msg_ variable and instead pass through the try_publish method by having a local version of message in the controllers etc. In future, I would like to extend it with a publish method where we can use an internal message queue (the size would be defined at the construction time) and this would make sure that the messages are always published. Right now, if it is unable to lock, we can never publish messages.

[Juliaj]

I see, with the new get_mutex(), above code could become

{
  std::unique_lock<std::mutex> lock(realtime_publisher_->get_mutex());
  realtime_publisher_->msg_.header.frame_id = params_.frame_id;
}

[Juliaj]

In future, I would like to extend it with a publish method where we can use an internal message queue (the size would be defined at the construction time) and this would make sure that the messages are always published.

This will be really helpful.

[saikishor]

Yes exactly, just like your code snippet

[Juliaj]

LGTM.

BTW, your fix in stop() for the deadlock issue was made to jazzy branch not master. i am assuming we also need that fix on master.

[saikishor]

LGTM.

BTW, your fix in stop() for the deadlock issue was made to jazzy branch not master. i am assuming we also need that fix on master.

@Juliaj It is already on the master branch
05c13d9

[christophfroehlich]

can we add the gcc pragmas to remove deprecation warnings from building the tests? or should we just remove the tests for the old API?

do you want to backport this to jazzy as well? or better leave the API untouched there? we could also just add the new methods

[saikishor]

can we add the gcc pragmas to remove deprecation warnings from building the tests? or should we just remove the tests for the old API?

do you want to backport this to jazzy as well? or better leave the API untouched there? we could also just add the new methods

Sorry i missed this comment. yes, I would like to backport this to Jazzy

[christophfroehlich]

Please update https://github.com/ros-controls/realtime_tools/blob/master/realtime_tools/doc/realtime_publisher.rst
and release/migration notes if necessary.