-
Notifications
You must be signed in to change notification settings - Fork 269
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
Wheelslip plugin parameter addition #2518
base: gz-sim8
Are you sure you want to change the base?
Conversation
Signed-off-by: knmcguire <[email protected]>
Signed-off-by: knmcguire <[email protected]>
Signed-off-by: knmcguire <[email protected]>
@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. |
@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 |
Sounds good. Thanks! |
There was a problem hiding this 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
@@ -248,29 +255,41 @@ void WheelSlipPrivate::Update(EntityComponentManager &_ecm) | |||
for (auto &linkSurface : this->mapLinkSurfaceParams) | |||
{ | |||
auto ¶ms = linkSurface.second; | |||
const auto * wheelSlipCmdComp = | |||
_ecm.Component<components::WheelSlipCmd>(linkSurface.first); |
There was a problem hiding this comment.
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
// Here the scoped name starts with "wheel_slip." | ||
// In `ConfigureParameters()` that doesn't happen! | ||
auto paramName = std::string("systems.") + scopedName; | ||
//transport::parameters::ParameterResult value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//transport::parameters::ParameterResult value; |
this line can be removed
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 😊 |
🎉 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
Another parameter check out the available parameters:
Expected outcome
Get the parameter of one of the wheels of the tricycles
Expected outcome
Set one parameter to a different number:
Expected outcome
Check the parameter again:
Expected outcome
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:
Message (gz.msgs.WheelSlipParametersCmd) was retrieved with non-fully qualified name. This behavior is deprecated in msgs10
// TODO(ivanpauno): WHY THE SCOPED NAME CHANGES BETWEEN HERE AND // ConfigureParameters()? // Here the scoped name starts with "wheel_slip." // In `ConfigureParameters()` that doesn't happen
Checklist
codecheck
passed (See contributing)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.