Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add wheel slip user command #1241

Merged
merged 42 commits into from Mar 28, 2022
Merged

Add wheel slip user command #1241

merged 42 commits into from Mar 28, 2022

Conversation

ivanpauno
Copy link
Contributor

@ivanpauno ivanpauno commented Dec 9, 2021

🎉 New feature

Summary

Depends on gazebosim/gz-msgs#205.

Add an user command to reconfigure the wheel slip parameters.
Combined with gazebosim/ros_gz#70, it will allow reconfigurability from ROS similar to what the gazebo_ros_pkgs wheel slip plugin provided.

Ideally, we would have a concept similar to parameters in ignition instead of this.

Test it

Manual testing:

# terminal 1, launch trisphere world
ign gazebo trisphere_cycle_wheel_slip.sdf`
# terminal 2, modify the wheel slip parameters of the rear left wheel
ign service -s /world/wheel_slip/wheel_slip --reqtype ignition.msgs.WheelSlipParametersCmd --reptype ignition.msgs.Boolean -r "
model_name: \"trisphere_cycle0\"
link_name: \"wheel_rear_left\"
slip_compliance_lateral: 2
slip_compliance_longitudinal: 2
" --timeout 1000

80dad69 can be reverted to see a log each time the wheel slip system detected a cmd.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@ivanpauno ivanpauno added the enhancement New feature or request label Dec 9, 2021
@ivanpauno ivanpauno self-assigned this Dec 9, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Dec 9, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Dec 9, 2021
@ivanpauno
Copy link
Contributor Author

cc @scpeters.

I'm targetting edifice as I commented here, but maybe that was an incorrect choice.
Let me know if I should be targetting a different ignition version.

Ideally, we would have a concept similar to parameters in ignition instead of this.

I started to take a look of how to do that, but I'm finally be working on a different task for 1 or 2 weeks. I'm still planning to take a deeper look at how to implement parameters in ignition after that.

I had already had implemented this on Monday when we discussed how to implement parameters in Ignition, so I'm opening this for evaluation in case that (ab)using the user commands services to provide this functionality seems like an acceptable approach.
This is much less work that supporting parameters in ignition, and supporting to bridge services between ignition and ROS 2 is something we're going to need anyway.

@chapulina chapulina moved this from Inbox to In progress in Core development Dec 9, 2021
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Dec 9, 2021
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/systems/user_commands/UserCommands.cc Outdated Show resolved Hide resolved
Comment on lines +20 to +24
#include <ignition/gazebo/config.hh>
#include <ignition/gazebo/Export.hh>
#include <ignition/gazebo/components/Component.hh>
#include <ignition/gazebo/components/Factory.hh>
#include <ignition/gazebo/components/Serialization.hh>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alphabetize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, you mean like

Suggested change
#include <ignition/gazebo/config.hh>
#include <ignition/gazebo/Export.hh>
#include <ignition/gazebo/components/Component.hh>
#include <ignition/gazebo/components/Factory.hh>
#include <ignition/gazebo/components/Serialization.hh>
#include <ignition/gazebo/components/Component.hh>
#include <ignition/gazebo/components/Factory.hh>
#include <ignition/gazebo/components/Serialization.hh>
#include <ignition/gazebo/config.hh>
#include <ignition/gazebo/Export.hh>

When having nested folders, I group items in each subfolder and I only alphaorder each group.
i.e. I put first all header files in the parent folder (in alpha order, in this case the items in ignition/gazebo are config.hh, Export.hh), after that I put the subfolders in alpha order (only components here) and the items of the subfolder in alpha order as well (i.e. Component.hh, Factory.hh, Serialization.hh).

I'm also not sure if lowercase letters are ordered after uppercase letters (like ascii code ordering, a goes after Z), or if I should ignore casing when ordering.
In the first case it should be:

Suggested change
#include <ignition/gazebo/config.hh>
#include <ignition/gazebo/Export.hh>
#include <ignition/gazebo/components/Component.hh>
#include <ignition/gazebo/components/Factory.hh>
#include <ignition/gazebo/components/Serialization.hh>
#include <ignition/gazebo/Export.hh>
#include <ignition/gazebo/components/Component.hh>
#include <ignition/gazebo/components/Factory.hh>
#include <ignition/gazebo/components/Serialization.hh>
#include <ignition/gazebo/config.hh>

Core development automation moved this from In progress to In review Dec 10, 2021
@chapulina chapulina moved this from In review to In progress in Core development Dec 20, 2021
Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about the user command interface, but I've left some feedback below.

src/systems/wheel_slip/WheelSlip.cc Outdated Show resolved Hide resolved
src/systems/wheel_slip/WheelSlip.cc Outdated Show resolved Hide resolved
src/systems/wheel_slip/WheelSlip.cc Outdated Show resolved Hide resolved
@jacobperron
Copy link

As we discussed offline, I think implementing this as a command (instead of a parameter) makes sense. I guess it ultimately depends on how we differentiate commands vs parameters; until then, it's difficult to say how the wheel slip configuration should be exposed.

What does it look like when users want to interact with the command (e.g. from the command-line)? An example would be nice (even if it is not functional yet). I'm wondering if we really need to bridge this via ros_ign or if we should just recommend users make calls to ignition directly?

@ivanpauno ivanpauno marked this pull request as ready for review February 3, 2022 19:26
Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation of wheel slip plugin in gazebo_ros_pkgs exposes a pair of parameters to set wheel slip parameters for all wheels : https://github.com/ros-simulation/gazebo_ros_pkgs/blob/dcda27f7ac710bdd6a8aba2692e3b3b3ac1d9c4b/gazebo_plugins/include/gazebo_plugins/gazebo_ros_wheel_slip.hpp#L31-L41, along with the ones used to set values for individual wheels.

Should we port that functionality here ? Basically equivalent to link_name : * , which would update values for all wheels in the ecm ?

Also, could you please add a test case for this ?

@ivanpauno
Copy link
Contributor Author

Also, could you please add a test case for this ?

Will do.

Should we port that functionality here ? Basically equivalent to link_name : * , which would update values for all wheels in the ecm ?

I think that's a good idea, using link_name : * works for me

@ivanpauno
Copy link
Contributor Author

Should we port that functionality here ? Basically equivalent to link_name : * , which would update values for all wheels in the ecm ?

Done in 76e14b7.

Copy link
Contributor

@adityapande-1995 adityapande-1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the wildcard addition to link names ! Can we add some simple tests for this PR ? Otherwise looks good.

src/systems/user_commands/UserCommands.cc Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Contributor Author

Thanks for the wildcard addition to link names ! Can we add some simple tests for this PR ? Otherwise looks good.

I was trying to create a test, but I couldn't find a way how.
The wheel slip parameters are internally stored by the WheelSlip plugin.
I don't think there's a way to get a pointer to a system and introspect it, but maybe there is (?).

The other thing I wanted to test is to see if I could check that the generated WheelSlipCmds that the UserCommands plugins used to communicate with the slip plugin were correct.
I couldn't make that happen either, as in the same preUpdate() step the user commands system generates the component and the wheel slip system later consumes it.

I will only add a test that shows that the service calls work, but if anyone has a better idea of how to test this I can try it.

@adityapande-1995
Copy link
Contributor

I agree, it is not possible here to test that the WheelSlip plugin has set the the correct parameters. Otherwise looks good to me. THe CI will require a release of ign-msgs I guess.

@ivanpauno
Copy link
Contributor Author

@adityapande-1995 I have modified how entities are identified based on feedback here.
Implementation is in 2d98ab9.
Could you review again?

ivanpauno and others added 3 commits March 21, 2022 17:16
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Aditya Pande <aditya050995@gmail.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
…nrobotics/ign-gazebo into ivanpauno/wheel-slip-cmd

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Contributor Author

The failing jobs seem unrelated.

@chapulina @adityapande-1995 can this be merged?

@chapulina
Copy link
Contributor

The failing jobs seem unrelated.

This test failure looks related:

157: [ RUN      ] UserCommandsTest.WheelSlip
157: �[31m[Err] [C:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-common\src\SystemPaths.cc:377] �[m�[31mUnable to find file with URI [�[m�[31mC:///Jenkins/workspace/ign_gazebo-pr-win/ws/ign-gazebo/test/worlds/trisphere_cycle_wheel_slip.sdf�[m�[31m]�[m�[31m
157: �[m�[31m[Err] [C:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-common\src\SystemPaths.cc:467] �[m�[31mCould not resolve file [�[m�[31mC:/Jenkins/workspace/ign_gazebo-pr-win/ws/ign-gazebo/test/worlds/trisphere_cycle_wheel_slip.sdf�[m�[31m]�[m�[31m
157: �[m�[31m[Err] [C:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-gazebo\src\Server.cc:158] �[m�[31mFailed to find world [�[m�[31mC:/Jenkins/workspace/ign_gazebo-pr-win/ws/ign-gazebo/test/worlds/trisphere_cycle_wheel_slip.sdf�[m�[31m]�[m�[31m
157: �[mC:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-gazebo\test\integration\user_commands.cc(1045): error: Expected: (nullptr) != (ecm), actual: (nullptr) vs NULL

I haven't been actively reviewing this, but please feel free to merge once CI is happy and it has an approval. Feel free to ping me if you need another set of eyes after the first approval.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno closed this Mar 23, 2022
Core development automation moved this from In review to Done Mar 23, 2022
@ivanpauno ivanpauno reopened this Mar 23, 2022
Core development automation moved this from Done to Inbox Mar 23, 2022
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Contributor Author

This test failure looks related:

Sorry, I didn't pay attention to the windows job.
Now it's failing again, but the new problem actually seems unrelated.

19606cd should've fixed the previous issue (all the other tests in that file are being skipped on Windows).

@chapulina chapulina moved this from Inbox to In review in Core development Mar 25, 2022
@chapulina
Copy link
Contributor

@ivanpauno @jacobperron @scpeters , heads up that Edifice (ign-gazebo5) will EOL next week. Do you think this will get merged before then?

@ivanpauno
Copy link
Contributor Author

@ivanpauno @jacobperron @scpeters , heads up that Edifice (ign-gazebo5) will EOL next week. Do you think this will get merged before then?

It would be good to have it merged before, so we only need to forward port to newer releases.
The PR is ready and CI was passing (I updated the PR with the base branch again, but it should still pass 🤞), so it only needs ✔️ and to get merged.

@adityapande-1995 could you take a look again?

@scpeters
Copy link
Member

CI looks fine to me

@scpeters scpeters dismissed ahcorde’s stale review March 28, 2022 18:28

changes have been addressed

@ivanpauno ivanpauno merged commit adeb304 into ign-gazebo5 Mar 28, 2022
@ivanpauno ivanpauno deleted the ivanpauno/wheel-slip-cmd branch March 28, 2022 21:31
Core development automation moved this from In review to Done Mar 28, 2022
@chapulina chapulina moved this from Done to Highlights in Core development Apr 1, 2022
chapulina added a commit that referenced this pull request Apr 13, 2022
* GzSceneManager: Prevent crash 💥 when inserted from menu (#1371)

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: ahcorde <ahcorde@gmail.com>

Co-authored-by: ahcorde <ahcorde@gmail.com>

* Prepare version 6.7.0 (#1373)

* Prepare version 6.7.0

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* Populate GUI plugins that are empty (#1375)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fix visualization python tutorial (#1377)

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Add xyz and rpy offset to published odometry pose (#1341)

* Added xyz and rpy offset to published pose

Signed-off-by: Aditya <aditya050995@gmail.com>

* Added headless rendering tutorial (#1386)

* Added headless rendering tutorial

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update tutorials/headless_rendering.md

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Update tutorials/headless_rendering.md

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Mention ogre2

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Added note about software rendering

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* add 'on linux systems'

Signed-off-by: Nate Koenig <nate@openrobotics.org>

* Add xyz and rpy offset to published odometry pose (#1341)

* Added xyz and rpy offset to published pose

Signed-off-by: Aditya <aditya050995@gmail.com>
Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Aditya Pande <aditya050995@gmail.com>

* Allow to turn on/off lights (#1343)

Signed-off-by: ahcorde <ahcorde@gmail.com>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add gazebo Entity id to rendering sensor's user data (#1381)

Adds the gazebo::Entity id to a rendering::Sensor's UserData object. The main use case is so that we can get back the corresponding gazebo Entity in the rendering thread when processing sensors.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Nate Koenig <nkoenig@users.noreply.github.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* Don't mark entities with a ComponentState::NoChange component as modified (#1391)

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

* Disable ModelCommandAPI_TEST.RgbdCameraSensor on macOS (#1397)

Disabling test since it's very flaky.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

* Disable PeerTracker.PeerTrackerStale on macOS (#1398)

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

* Toggle Light visuals (#1387)

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Prevent hanging when world has only non-world plugins (#1383)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* Component inspector: refactor Pose3d C++ code into a separate class (#1400)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* add initial_position param to joint controller system (#1406)

Similar to the <initial_position> parameter in the JointTrajectoryController system, this PR adds an <initial_position> parameter to the JointPositionController to let users specifies the initial target pos for a joint.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* Fix JointStatePublisher topic name for nested models (#1405)

The joint-state-publisher system currently assumes it's always attached to the top level model and hence generates an incorrect topic name for nested models. This PR updates it to generate the correct topic name.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* Added user command to set multiple entities (#1394)

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Disable tests that are expected to fail on Windows (#1408)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Fortress: Install Ogre 2.2, simplify docker (#1395)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* SceneBroadcaster: only send changed state information for change events (#1392)

Signed-off-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add wheel slip user command (#1241)

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

* Distortion camera integration test (#1374)

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Add the Model Photo Shoot system, port of Modelpropshop plugin from Gazebo classic (#1331)

This system can be used to generate thumbnails of models. In comparison to the Modelpropshop plugin in Gazebo classic, this adds the ability to generate thumbnails after randomizing the joint positions of the model.

Signed-off-by: Marco A. Gutierrez <marco@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

* Referring to Fuel assets within a heightmap (#1419)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* Disable sensors in sensors system when battery is drained (#1385)

Added a new parameter <disable_on_drained_battery> to the sensors system. If set to true, sensors will be disabled if the model is out of battery.

It listens to the battery state of charge from the battery plugin, and if the charge reaches zero, all child sensors and nested sensors are set to be inactive.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* 🏁🎈 5.4.0 (#1420)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

* ServerConfig accepts an sdf::Root DOM object (#1333)

Signed-off-by: Nate Koenig <nate@openrobotics.org>

Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Michael Carroll <michael@openrobotics.org>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

* Preparing 6.8.0 release (#1425)

* Prepareing 6.8.0 release

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* Update changelog after merging forward port

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

* Add Gaussian noise to Odometry Publisher (#1393)

* Added gaussian noise and odometry with covariance publisher

Signed-off-by: Aditya <aditya050995@gmail.com>

Co-authored-by: Louise Poubel <louise@openrobotics.org>

* Fix deprecation warnings for ModelPhotoShoot (#1437)

Signed-off-by: Louise Poubel <louise@openrobotics.org>

Co-authored-by: ahcorde <ahcorde@gmail.com>
Co-authored-by: Jose Luis Rivero <jrivero@osrfoundation.org>
Co-authored-by: Aditya Pande <aditya050995@gmail.com>
Co-authored-by: Nate Koenig <nkoenig@users.noreply.github.com>
Co-authored-by: Nate Koenig <nate@openrobotics.org>
Co-authored-by: Ian Chen <ichen@osrfoundation.org>
Co-authored-by: Ashton Larkin <42042756+adlarkin@users.noreply.github.com>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>
Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Co-authored-by: William Lew <WilliamMilesLew@gmail.com>
Co-authored-by: Marco A. Gutiérrez <marcogg@marcogg.com>
Co-authored-by: Michael Carroll <michael@openrobotics.org>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

@j-rivero j-rivero removed this from Highlights in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants