Skip to content

Add wheel slip message definition#205

Merged
scpeters merged 4 commits intoign-msgs7from
ivanpauno/wheel-slip-msg
Mar 9, 2022
Merged

Add wheel slip message definition#205
scpeters merged 4 commits intoign-msgs7from
ivanpauno/wheel-slip-msg

Conversation

@ivanpauno
Copy link
Contributor

@ivanpauno ivanpauno commented Dec 9, 2021 •
edited
Loading

Summary

Adds a build sleep message definition, which would be used to reconfigure the wheel slip systems parameters.
The slip compliance parameters used here are unitless, as explained in the wheel slip system.
Maybe we should be more explicit about that in the msg definition (?).

Test it

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

@ivanpauno ivanpauno added the enhancement New feature or request label Dec 9, 2021
@ivanpauno ivanpauno requested a review from scpeters December 9, 2021 21:34
@ivanpauno ivanpauno requested a review from caguero as a code owner December 9, 2021 21:34
@ivanpauno ivanpauno self-assigned this Dec 9, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Dec 9, 2021
@ivanpauno
Copy link
Contributor Author

ivanpauno commented Dec 9, 2021

@scpeters I'm targetting ign-msgs7 because I'm using edifice locally for development, but maybe that was an incorrect choice.
What version of ignition should I be targetting?

@ivanpauno ivanpauno marked this pull request as draft December 9, 2021 21:36
@codecov
Copy link

codecov bot commented Dec 9, 2021 •
edited
Loading

Codecov Report

Merging #205 (8f4c837) into ign-msgs7 (972b240) will not change coverage.
The diff coverage is n/a.

❗ Current head 8f4c837 differs from pull request most recent head 858d8e6. Consider uploading reports for the commit 858d8e6 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-msgs7     #205   +/-   ##
==========================================
  Coverage      85.60%   85.60%           
==========================================
  Files              9        9           
  Lines            924      924           
==========================================
  Hits             791      791           
  Misses           133      133

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 972b240...858d8e6. Read the comment docs.

@ivanpauno ivanpauno mentioned this pull request Dec 9, 2021
Merged
8 tasks
@scpeters
Copy link
Member

scpeters commented Dec 9, 2021

@scpeters I'm targetting ign-msgs7 because I'm using edifice locally for development, but maybe that was an incorrect choice.
What version of ignition should I be targetting?

we generally try to target the oldest version that will support the feature, but I'm not sure which version that will be yet. I think Edifice is fine for now, though it will EOL in March 2022

@scpeters
Copy link
Member

scpeters commented Dec 9, 2021

one thing to keep in mind is that we have just added a WheelSlip.msg to gazebo_msgs in ros-simulation/gazebo_ros_pkgs#1331 for publishing instantaneous wheel slip, so we may want to rename the message proposed here to wheel_slip_parameters or wheel_slip_configuration to avoid confusion

@ivanpauno
Copy link
Contributor Author

ivanpauno commented Jan 4, 2022

one thing to keep in mind is that we have just added a WheelSlip.msg to gazebo_msgs in ros-simulation/gazebo_ros_pkgs#1331 for publishing instantaneous wheel slip, so we may want to rename the message proposed here to wheel_slip_parameters or wheel_slip_configuration to avoid confusion

Addressed in a44f73e

scpeters
scpeters reviewed Jan 11, 2022
View reviewed changes
@ivanpauno
Copy link
Contributor Author

ivanpauno commented Jan 12, 2022

When working in a demo of this using parameters, I realized in that case I need a slightly different message definition a5b39fd.

Maybe we should rename the one being added here WheelSlipParametersCmd, what do you think?

jacobperron
jacobperron approved these changes Feb 23, 2022
View reviewed changes
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.

LGTM

ivanpauno added 4 commits March 9, 2022 10:52
@ivanpauno
Add wheel slip message definition
bc3a19f 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
wheel_slip.proto -> wheel_slip_parameters.proto
be0d1cf 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Address peer review comments
c27d68e 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Update message definition
858d8e6 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/wheel-slip-msg branch from 5b2dd17 to 858d8e6 Compare March 9, 2022 13:53
@ivanpauno ivanpauno marked this pull request as ready for review March 9, 2022 21:07
@scpeters scpeters merged commit eff9354 into ign-msgs7 Mar 9, 2022
@scpeters scpeters deleted the ivanpauno/wheel-slip-msg branch March 9, 2022 21:08
chapulina
chapulina reviewed Mar 9, 2022
View reviewed changes
proto/ignition/msgs/wheel_slip_parameters_cmd.proto
string link_name = 3;

double slip_compliance_lateral = 4;
double slip_compliance_longitudinal = 5;
Copy link
Contributor

@chapulina chapulina Mar 9, 2022

Choose a reason for hiding this comment

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

Sorry for the late comment, but while reviewing the forward-port, I noticed that most fields on this message aren't documented.

Copy link
Contributor Author

@ivanpauno ivanpauno Mar 9, 2022

Choose a reason for hiding this comment

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

I can add docs in a new PR

Copy link
Contributor Author

@ivanpauno ivanpauno Mar 10, 2022

Choose a reason for hiding this comment

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

See #227

@ivanpauno ivanpauno mentioned this pull request Mar 10, 2022
Merged
7 tasks
@osrf-triage
Copy link

osrf-triage commented Mar 25, 2022

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

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

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

Reviewers

@jacobperron jacobperron jacobperron approved these changes

@scpeters scpeters scpeters approved these changes

@caguero caguero Awaiting requested review from caguero caguero is a code owner

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995

+1 more reviewer

@chapulina chapulina chapulina left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ivanpauno ivanpauno

Labels

🏢 edifice Ignition Edifice enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ivanpauno @scpeters @osrf-triage @jacobperron @chapulina @j-rivero