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

Add wheel slip message definition #205

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Add wheel slip message definition #205

merged 4 commits into from
Mar 9, 2022

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 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

@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

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
proto/ignition/msgs/wheel_slip_parameters.proto Outdated Show resolved Hide resolved
proto/ignition/msgs/wheel_slip_parameters.proto Outdated Show resolved Hide resolved
proto/ignition/msgs/wheel_slip_parameters.proto Outdated Show resolved Hide resolved
proto/ignition/msgs/wheel_slip_parameters.proto Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Contributor Author

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
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 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

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

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

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

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

@chapulina chapulina chapulina left review comments

@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

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.
5 participants
@ivanpauno @scpeters @osrf-triage @jacobperron @chapulina