gz-sim
gz-sim copied to clipboard
Add a service to trigger functionality
🎉 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.
https://user-images.githubusercontent.com/44885838/180519901-f5d5213d-d0ff-4a6d-95f4-19b6358d2d82.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
- cd into
Checklist
- [x] Signed all commits for DCO
- [x] Added tests
- [x] Added example and/or tutorial
- [x] Updated documentation (as needed)
- [ ] Updated migration guide (as needed)
- [ ] Consider updating Python bindings (if the library has them)
- [x]
codecheck
passed (See contributing) - [x] 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.
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
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]
@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 Report
Merging #1611 (55e3e4d) into ign-gazebo6 (69e7757) will increase coverage by
0.03%
. The diff coverage is84.28%
.
:exclamation: 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%) |
:arrow_down: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
@osrf-jenkins run tests
Looks like just style errors before we get to the actual tests
@mabelzhang, @clalancette and I chatted on Thursday and decided to just make this much simpler by using the blocking Request
method instead of non-blocking Request
method. I just pushed everything is ready for a review.
Summary
The reason I decided on with the non-blocking Request method is because I wanted the services to not block other service calls if multiple services are provided for one input (on sdf). Although using a non-blocking Request resolves this, it creates a lot more issues. Request method instantiates a ReqHandler
as shown here. Currently, not all combinations with a single protobuf::Messages
are considered. So when you use non blocking Request, this template specialization is not invoked and causes en error.
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.
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
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.
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?
ABI check is the last thing before approval and merge. The other test failures look irrelevant since this PR only touches triggered publisher.
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.
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/
Brilliant! Thanks for all the thorough review Mabel!