Add a service to trigger functionality

[liamhan0905]

🎉 New feature

Summary

Extended the trigger publisher functionality to allow calling a service. For this example, I've reset the pose of the robot as well as set the cmd_vel value to 0, hence stopping the robot. This will allow others to use triggered publisher for calling arbitrary number of services as an "output" from the plugin.

service.demo.mp4

Test it

I have modified the tutorial/triggered_publisher.md tutorial to add this functionality. The world file, examples/words/triggered_publisher.sdf also reflects this.

• checkout liam/add-serv-to-trig-pub branch and rebuild
• run ign gazebo triggered_publisher.sdf
• run ign topic -t "/start" -m ignition.msgs.Empty -p " " to run the original triggered publisher example.
• run ign topic -t "/reset_robot" -m ignition.msgs.Empty -p " " to reset the robot (calling the reset pose service).
  • metadata related to this plugin can be found in the world file. E.g. modifying the reset position
• to run the gtests
  • cd into /build/ignition-gazebo6/bin and run ./INTEGRATION_triggered_publisher

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[liamhan0905]

Added 3 more tests for the triggered_publisher.cc.

• Test for service that's unavailable -> timeout
• Test for one input and one service
• Test for one input and multiple service

All tests have been passed

[mabelzhang]

Did a static pass. Mostly style.

Note to self: Haven't looked at test/integration/triggered_publisher.cc yet.

[mabelzhang]

Bionic CI is failing on some style items

  /github/workspace/src/systems/triggered_publisher/TriggeredPublisher.cc:747:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/test/integration/triggered_publisher.cc:656:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
  /github/workspace/test/integration/triggered_publisher.cc:698:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]

[liamhan0905]

@mabelzhang Thank you so much for the thorough review! And sorry about all the silly syntax/grammar mistakes haha But please keep providing me all these feedback so that I can learn (:

Question

• What did you mean by static pass on your comment?

I'll push the updated syntax-fix later tonight and work on the fix for the service tomorrow.

[codecov]

Codecov Report

Merging #1611 (55e3e4d) into ign-gazebo6 (69e7757) will increase coverage by 0.03%.
The diff coverage is 84.28%.

❗ Current head 55e3e4d differs from pull request most recent head a88d88e. Consider uploading reports for the commit a88d88e to get more accurate results

@@               Coverage Diff               @@
##           ign-gazebo6    #1611      +/-   ##
===============================================
+ Coverage        64.69%   64.73%   +0.03%     
===============================================
  Files              321      321              
  Lines            26062    26122      +60     
===============================================
+ Hits             16862    16911      +49     
- Misses            9200     9211      +11     

Impacted Files	Coverage Δ
.../systems/triggered_publisher/TriggeredPublisher.hh	100.00% <ø> (ø)
.../systems/triggered_publisher/TriggeredPublisher.cc	83.28% <84.28%> (-0.38%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mabelzhang]

Thanks for iterating! Last style comments.

Some tests on CI are failing but this PR isn't changing any files other than this one plugin.

One test seems flaky locally for me:

$ ./build/ignition-gazebo6/bin/INTEGRATION_triggered_publisher 
...
/home/developer/ign_fortress/src/ign-gazebo/test/integration/triggered_publisher.cc:728: Failure
Expected equality of these values:
  pubCount
    Which is: 10
  recvCount
    Which is: 9
[  FAILED  ] TriggeredPublisherTest.InputMessagesTriggerServices (6074 ms)
...
[  FAILED  ] 1 test, listed below:
[  FAILED  ] TriggeredPublisherTest.InputMessagesTriggerServices

I ran the tests locally twice in a row, and it failed once, then passed. I ran it more times, and then it always passed. Maybe it was something to do with timing the first time I ran it, but I still took a closer look at that test and made some comments.

The main thing is, we don't want to be adding flaky tests. If they are already flaky at the time of creation, then we should fix that. Can you see if it's ever flaky for you?

[mabelzhang]

Are these return values being checked anywhere? Are they needed?
If they're returning a value for a reason, they should be checked in the test. If they aren't there for any specific reason, then they aren't needed.

[liamhan0905]

hmm I'm not getting any error locally on my side. I ran at least 30 times with no error. I also tried testing it with 100 and 500 publications instead of just 10 (which is the value I used for all the tests). And I didn't get any errors.

Reading the error msg, the pubCount is 10 but the recvCount is 9. Which means that one publication wasn't handled by the callback. The question is whether it was the first publication that missed the callback or from one of the following publications. If the service server was NOT setup on time, it should've thrown a timeout error. Did you see any error msg like this when you ran it locally?

It's weird that it only occurred on one of the test when all the subsequent test have similar structure. So I would've expected similar behavior on other tests... But unless we reproduce the same error, I think it might be hard to know what could've caused it.

[liamhan0905]

update:

It was caused by callback method looping faster than the DoWork method. Hence, the triggerSrv variable could've been set twice in the callback while only invoked once in the DoWork method. Both the callback and DoWork methods are running on separate thread. For example, the callback method might have looped twice and set the triggerSrv twice. But if the DoWork was still in progress, it'll only call the service one. And triggerSrv is just a boolean value. So it doesn't have a record of iteration.

To fix this, I reverted back using the serviceCount, similar to the publishCount. This will prevent any skipping. In an extreme case, let's say that callback looped 4 times while DoWork looped once. At next iteration, the DoWork will call the service whatever the serviceCount value is and not skip anything. In most case, serviceCount should be 1. But in cases where callback is faster. the serviceCount >1.

This approach will be yield least flaky test

[mabelzhang]

Good find! Thanks for troubleshooting the flaky test! It seems reasonable what you did. It was hard to reproduce for me even the first time, so I can't really test it reliably and say it's better. It passes locally for me. Hopefully it doesn't become flaky on CI.

I think I wasn't clear about which indentations I was talking about in the last review, so I pushed a few nit style changes in fced8fb. Faster than going back and forth.

Thanks for removing the ID as well.

I'm wrapping up for the week. Just waiting for CI. Should be good to go next week.

[mabelzhang]

ABI checker is failing in the checks below
https://build.osrfoundation.org/job/ignition_gazebo-abichecker-any_to_any-ubuntu_auto-amd64/5635/API_5fABI_20report/

Removed Symbols 7

EntityComponentManager.hh, libignition-gazebo6.so.6.12.0
namespace ignition::gazebo::v6
EntityComponentManager::HasPeriodicComponentChanges ( ) const
_ZNK8ignition6gazebo2v622EntityComponentManager27HasPeriodicComponentChangesEv


Gui.hh, libignition-gazebo6-gui.so.6.12.0
namespace ignition::gazebo::v6::gui
createGui ( int& _argc, char** _argv, char const* _guiConfig, char const* _defaultGuiConfig, bool _loadPluginsFromSdf, char const* _sdfFile, int _waitGui, char const* _renderEngine )
_ZN8ignition6gazebo2v63gui9createGuiERiPPcPKcS7_bS7_iS7_

runGui ( int& _argc, char** _argv, char const* _guiConfig, char const* _sdfFile, int _waitGui, char const* _renderEngine )
_ZN8ignition6gazebo2v63gui6runGuiERiPPcPKcS7_iS7_


Model.hh, libignition-gazebo6.so.6.12.0
namespace ignition::gazebo::v6
Model::CanonicalLink ( EntityComponentManager const& _ecm ) const
_ZNK8ignition6gazebo2v65Model13CanonicalLinkERKNS1_22EntityComponentManagerE


ServerConfig.hh, libignition-gazebo6.so.6.12.0
namespace ignition::gazebo::v6
ServerConfig::LogRecordPeriod ( ) const
_ZNK8ignition6gazebo2v612ServerConfig15LogRecordPeriodEv

ServerConfig::SetLogRecordPeriod ( std::chrono::_V2::steady_clock::duration const& _period )
_ZN8ignition6gazebo2v612ServerConfig18SetLogRecordPeriodERKNSt6chrono8durationIlSt5ratioILl1ELl1000000000EEEE


Util.hh, libignition-gazebo6.so.6.12.0
namespace ignition::gazebo::v6
entityFromMsg ( EntityComponentManager const& _ecm, ignition::msgs::Entity const& _msg )
_ZN8ignition6gazebo2v613entityFromMsgERKNS1_22EntityComponentManagerERKNS_4msgs6EntityE

Problems with Constants, Low Severity 1

1	The constant GZ_DISTRIBUTION with value "Fortress" has been removed.	The value of this constant may no longer be properly handled by new-version library functions.

Can you merge from the target branch and see if that fixes it?

[mabelzhang]

ABI check is the last thing before approval and merge. The other test failures look irrelevant since this PR only touches triggered publisher.

[mabelzhang]

Thanks - ABI check is passing.

Sorry I hadn't seen this earlier, but these gcc warnings will need to be fixed:
https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/9625/gcc/
This is under GitHub check ignition_gazebo-ci-pr_any-ubuntu_auto-amd64.

I think you can either comment out rep or remove the name of the variable.

[liamhan0905]

I removed the rep but is there a way to check the gcc warnings locally before pushing? Also, I'm not sure how to navigate to GitHub check ignition_gazebo-ci-pr_any-ubuntu_auto-amd64.

https://build.osrfoundation.org/job/ignition_gazebo-ci-pr_any-ubuntu_auto-amd64/9628/

[mabelzhang]

Thanks for iterating!

I think the warnings should show up at compile time? I'm not sure. I haven't run the latest version locally.

CI looks good. Make sure to use Squash Merge.

Addressed

[liamhan0905]

Brilliant! Thanks for all the thorough review Mabel!