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

[SW-893] odometry velocities reported in the wrong frame #416

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

[SW-893] odometry velocities reported in the wrong frame #416

wants to merge 9 commits into from

Conversation

khughes-bdai
Copy link
Collaborator

@khughes-bdai khughes-bdai commented Jun 25, 2024 •
edited
Loading

Change Overview

As brought up in #29, the odometry twist should be reported in the body frame, not the odom frame. This will make the spot driver easier to use with other ROS tools that expect this data to be in this format.

Testing Done (TODO)

  • walk the robot in a straight line from a variety of different heading angles. ensure that the twist component has linear velocity only in the x dimension.
    • tested this on robot. didn't seem to work as i was seeing significant velocity components in both x and y while walking the robot in a straight line. so either I'm not understanding something about how the ExpressVelocityInNewFrame function works (or maybe it just doesn't work) or i have some other random bug.
  • DMM's rtabmap pipeline still works as expected
  • update gtests to account for this new change (they won't pass currently)

@khughes-bdai Graphite App
Copy link
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @khughes-bdai and the rest of your teammates on Graphite Graphite

@coveralls
Copy link

coveralls commented Jun 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9664069179

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 52.893%
Files with Coverage Reduction New Missed Lines %
spot_driver/src/conversions/robot_state.cpp 3 93.93%
Totals Coverage Status
Change from base Build 9568975144: 0.06%
Covered Lines: 1883
Relevant Lines: 3560
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 25, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9664577407

Details

  • 13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 52.906%
Files with Coverage Reduction New Missed Lines %
spot_driver/src/conversions/robot_state.cpp 3 93.95%
Totals Coverage Status
Change from base Build 9568975144: 0.08%
Covered Lines: 1884
Relevant Lines: 3561
💛 - Coveralls

@coveralls
Copy link

coveralls commented Sep 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11446337408

Details

  • 19 of 20 (95.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 51.184%
Changes Missing Coverage Covered Lines Changed/Added Lines %
spot_driver/src/conversions/robot_state.cpp 18 19 94.74%
Files with Coverage Reduction New Missed Lines %
spot_driver/src/conversions/robot_state.cpp 1 94.47%
Totals Coverage Status
Change from base Build 11445280253: 0.1%
Covered Lines: 1945
Relevant Lines: 3800
💛 - Coveralls

@khughes-bdai khughes-bdai mentioned this pull request Dec 18, 2024
Merged
3 tasks
khughes-bdai added a commit that referenced this pull request Dec 19, 2024
@khughes-bdai
[SW-1764] allow use of preferred_odom_frame parameter (#538)
3c2f3c9 
## Change Overview

The `preferred_odom_frame` parameter is supposed to let you chose whether "odom" (default) or "vision" frame is used as the base frame in the `/odometry` topic. On main right now this is hardcoded to "odom" in the launchfile

This PR allows you to actually change this parameter. It also introduces some logic to deal with getting the velocity of the body in either the "odom" or "vision" frame, depending on which choice is selected. There is also some minor launchfile cleanup here. 

related to: #29, and takes a lot of work from #416

## Testing Done

- [x] default launch: nothing changes, and "SpotName/odom" is the parent frame in `/SpotName/odometry` topic
- [x] with `preferred_odom_frame: "vision"` parameter set via config file: "vision" is the parent frame in `/SpotName/odometry` 
- [x] modified unit tests to test cases of getting body velocity in the vision frame
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

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

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

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

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

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@khughes-bdai @coveralls