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.

Sign up for GitHub

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

Jump to bottom

Wheelslip plugin parameter addition #2518

Draft
wants to merge 3 commits into
base: gz-sim8
Choose a base branch
from
Draft

Wheelslip plugin parameter addition #2518

wants to merge 3 commits into from

Conversation

knmcguire
Copy link

@knmcguire knmcguire commented Aug 6, 2024 •
edited
Loading

🎉 New feature

Helps with #1845

Summary

Adds parameter change functionality using the gz param to the wheel slip system plugin. This is in preparation to be able to write a working tutorial that is described in this #1845. This PR is based on the associated branch of that issue namely ign-gazebo6...ivanpauno/parameters-component-wheel-slip-demo6 from @ivanpauno but adjusted for gz-sim8

Test it

I've based these instructions on the instructions given in #1845 by scpeters adjusted to gz-sim8

Launch the trisphere cycle world

gz sim trisphere_cycle_wheel_slip.sdf

Another parameter check out the available parameters:

gz param -r /world/wheel_slip -l
Expected outcome 

Listing parameters, registry namespace [/world/wheel_slip]...

systems.wheel_slip.trisphere_cycle1.wheel_front            [gz_msgs.WheelSlipParametersCmd]
systems.wheel_slip.trisphere_cycle1.wheel_rear_right            [gz_msgs.WheelSlipParametersCmd]
systems.wheel_slip.trisphere_cycle0.wheel_rear_right            [gz_msgs.WheelSlipParametersCmd]
systems.wheel_slip.trisphere_cycle1.wheel_rear_left            [gz_msgs.WheelSlipParametersCmd]
systems.wheel_slip.trisphere_cycle0.wheel_rear_left            [gz_msgs.WheelSlipParametersCmd]
systems.wheel_slip.trisphere_cycle0.wheel_front            [gz_msgs.WheelSlipParametersCmd]

Get the parameter of one of the wheels of the tricycles

gz param -r /world/wheel_slip -g -n systems.wheel_slip.trisphere_cycle1.wheel_front
Expected outcome 
Getting parameter [systems.wheel_slip.trisphere_cycle1.wheel_front] for registry namespace [/world/wheel_slip]...
Message (gz.msgs.WheelSlipParametersCmd) was retrieved with non-fully qualified name. This behavior is deprecated in msgs10
Parameter type [gz_msgs.WheelSlipParametersCmd]

------------------------------------------------
slip_compliance_lateral: 1
slip_compliance_longitudinal: 1
------------------------------------------------

Set one parameter to a different number:

gz param -r /world/wheel_slip -s -n systems.w
heel_slip.trisphere_cycle1.wheel_front -t gz_msgs.WheelSlipParametersCmd -m "
  slip_compliance_lateral: 1
  slip_compliance_longitudinal: 2"
Expected outcome 

Setting parameter [systems.wheel_slip.trisphere_cycle1.wheel_front] for registry namespace [/world/wheel_slip]...
Parameter successfully set!

Check the parameter again:

gz param -r /world/wheel_slip -g -n systems.wheel_slip.trisphere_cycle1.wheel_front
Expected outcome 
Getting parameter [systems.wheel_slip.trisphere_cycle1.wheel_front] for registry namespace [/world/wheel_slip]...
Message (gz.msgs.WheelSlipParametersCmd) was retrieved with non-fully qualified name. This behavior is deprecated in msgs10
Parameter type [gz_msgs.WheelSlipParametersCmd]

------------------------------------------------
slip_compliance_lateral: 1
slip_compliance_longitudinal: 2
-----------------------------------------------

Remaining Questions.

I've put the PR into draft for now as there are a couple things to solve first or what I need help with:

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.

@knmcguire
add parameters to wheelslip system plugin
57ba925 
Signed-off-by: knmcguire <kimberly@bitcraze.io>
@knmcguire
wheelslip update to latest gazebo changes
a998454 
Signed-off-by: knmcguire <kimberly@bitcraze.io>
@knmcguire
wheel slip different way to get parameter message
b588acf 
Signed-off-by: knmcguire <kimberly@bitcraze.io>
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Aug 6, 2024
@knmcguire knmcguire mentioned this pull request Aug 6, 2024
Open
@azeey
Copy link
Contributor

azeey commented Aug 6, 2024

@knmcguire just a heads up...since we are in feature freeze you'll probably not get a review on this for a while. We really appreciate you creating the PR and I hope you can wait to iterate on the PR till then.

@knmcguire
Copy link
Author

@azeey Oh yes no worries I was aware and as you see there are still things to fix before this PR is ready to be properly reviewed and merged. I was just in a flow of implementing the params and see this also as a way to show people how to implement parameters as a plugin (or perhaps how not to do it if I did it completely the wrong way 😄 ).

I'll rework this a bit and once the freeze is over I can mark it ready

@azeey
Copy link
Contributor

azeey commented Aug 7, 2024

Sounds good. Thanks!

scpeters
scpeters reviewed Sep 18, 2024
View reviewed changes
Copy link
Member

@scpeters scpeters 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 opening this draft PR; I was just looking at this demo branch as well

src/systems/wheel_slip/WheelSlip.cc
@@ -248,29 +255,41 @@ void WheelSlipPrivate::Update(EntityComponentManager &_ecm)
for (auto &linkSurface : this->mapLinkSurfaceParams)
{
auto &params = linkSurface.second;
const auto * wheelSlipCmdComp =
_ecm.Component<components::WheelSlipCmd>(linkSurface.first);
Copy link
Member

Choose a reason for hiding this comment

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

Deleting this line removes the ability to set wheel slip parameters via the ECM. I think it would be better to keep the ECM support (since it is more deterministic) but to add the gz-transport parameters interface in addition to it

src/systems/wheel_slip/WheelSlip.cc
// Here the scoped name starts with "wheel_slip."
// In `ConfigureParameters()` that doesn't happen!
auto paramName = std::string("systems.") + scopedName;
//transport::parameters::ParameterResult value;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//transport::parameters::ParameterResult value;

this line can be removed

@knmcguire
Copy link
Author

Ah awesome. It's still in draft since I wanted to wait for the dust to settle of all the tutorial partying 🥳

Once i'm back from my holidays next week I'll look into making the PR more final. @scpeters if there are any of the remaining questions that I've noted in the first post that you can give some pointers for to solve then that would help me out quite a bit 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters left review comments

@mjcarroll mjcarroll Awaiting requested review from mjcarroll mjcarroll will be requested when the pull request is marked ready for review mjcarroll is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
🎵 harmonic Gazebo Harmonic
Projects
Core development
Status: In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@knmcguire @azeey @scpeters