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

[Citadel] [TPE] Add velocity cmd #169

Merged
merged 45 commits into from
Jul 6, 2020
Merged

Conversation

claireyywang
Copy link
Contributor

@claireyywang claireyywang commented Jun 1, 2020

This PR is consisted of a few parts

  1. creates new AngularVelocityCmd and LinearVelocityCmd components
  2. adds new optional WorldVelocityCmdFeatureList for SetFreeGroupWorldVelocity feature
  3. adds functions that updates angular and linear velocities of Free Group entity in Physics.cc
  4. creates a new demo world velocity_demo.sdf, which is basically diff_drive.sdf but with different names. I created a separate sdf file to avoid confusion, but open to just use diff_drive.sdf.
  5. creates new systems plugin named velocity_demo that generates angular/linearVelocityCmd on model.

Use the following cmds for testing

$ ign gazebo -v 2 velocity_demo.sdf --physics-engine ignition-physics-tpe-plugin

# in a separate terminal window
$ ign topic -t "/model/vehicle_green/cmd_vel" -m ignition.msgs.Twist -p "linear: {x: 1.0}, angular: {z: 0.1}"
$ ign topic -t "/model/vehicle_blue/cmd_vel" -m ignition.msgs.Twist -p "linear: {x: 0.5}, angular: {z: 0.05}"

then hit play button on ignition gazebo window.

Demo gif see below

velocity demo

@claireyywang claireyywang added enhancement New feature or request 🔮 dome Ignition Dome 🏰 citadel Ignition Citadel physics Involves Ignition Physics labels Jun 1, 2020
@claireyywang claireyywang self-assigned this Jun 1, 2020
Signed-off-by: claireyywang <[email protected]>
@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #169 into ign-gazebo3 will decrease coverage by 0.39%.
The diff coverage is 15.78%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #169      +/-   ##
===============================================
- Coverage        65.33%   64.93%   -0.40%     
===============================================
  Files              127      129       +2     
  Lines             6311     6349      +38     
===============================================
  Hits              4123     4123              
- Misses            2188     2226      +38     
Impacted Files Coverage Δ
src/systems/physics/Physics.cc 32.58% <5.88%> (-1.26%) ⬇️
...e/ignition/gazebo/components/AngularVelocityCmd.hh 100.00% <100.00%> (ø)
...de/ignition/gazebo/components/LinearVelocityCmd.hh 100.00% <100.00%> (ø)
src/SimulationRunner.cc 86.36% <0.00%> (-1.19%) ⬇️

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 26f9173...f9064e9. Read the comment docs.

@chapulina chapulina removed the 🔮 dome Ignition Dome label Jun 2, 2020
Signed-off-by: claireyywang <[email protected]>
Copy link
Contributor Author

@claireyywang claireyywang left a comment

Choose a reason for hiding this comment

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

[ignore]

claireyywang and others added 2 commits June 25, 2020 19:05
Signed-off-by: claireyywang <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Copy link
Contributor

@iche033 iche033 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 adding the integration test. I expanded it a little to check that the robot is moving in the right direciton: 1b88b37

Just left another minor comment. Otherwise changes look good to me.

src/systems/velocity_control/VelocityControl.cc Outdated Show resolved Hide resolved
@claireyywang
Copy link
Contributor Author

@osrf-jenkins run tests please

@claireyywang claireyywang requested a review from iche033 July 1, 2020 23:41
@claireyywang
Copy link
Contributor Author

@iche033 Thanks for adding the additional test! I removed the unused Command struct in f9064e9. The homebrew CI failures and warnings don't seem to be related to this PR. Ready for another look whenever you are, thanks!

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good to me!

@claireyywang
Copy link
Contributor Author

@osrf-jenkins run tests please

@claireyywang claireyywang merged commit 69a76e4 into ign-gazebo3 Jul 6, 2020
@claireyywang claireyywang deleted the claire/add_velocity_cmd branch July 6, 2020 18:58
@jacobperron
Copy link

jacobperron commented Jul 10, 2020

It looks like this change introduced test failures to ignition_gazebo-ci-ign-gazebo3-homebrew-amd64:

@jacobperron
Copy link

It looks like the PeerTracker failure is a known flake (#8), but I'm not sure about the INTEGRATION_physics_system test.

@claireyywang
Copy link
Contributor Author

It looks like the PeerTracker failure is a known flake (#8), but I'm not sure about the INTEGRATION_physics_system test.

Im trying to reproduce the physics_system test fail on my local. I'll update here when I get a result.

@claireyywang
Copy link
Contributor Author

weird, I wasn't to get the same test fail on my local build. I reran the homebrew test to see if the failure pops up again

@claireyywang
Copy link
Contributor Author

@jacobperron INTEGRATION_physics_system seems to be another flaky test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel enhancement New feature or request physics Involves Ignition Physics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants