Update odometry implementation in diff_drive #1854
Conversation
github-actions
bot
requested review from
Maverobot,
bijoua29,
christophfroehlich,
erickisos,
fmauch,
harderthan and
saikishor
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1854 +/- ##
==========================================
- Coverage 85.20% 84.97% -0.24%
==========================================
Files 148 148
Lines 14321 14359 +38
Branches 1225 1230 +5
==========================================
- Hits 12202 12201 -1
- Misses 1694 1731 +37
- Partials 425 427 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
diff_drive_controller/include/diff_drive_controller/odometry.hpp
Outdated
Show resolved
Hide resolved
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
Let's discuss my points first, and merge the outcome. If necessary, let's open a new PR afterwards for the other controllers.
diff_drive_controller/include/diff_drive_controller/odometry.hpp
Outdated
Show resolved
Hide resolved
diff_drive_controller/include/diff_drive_controller/odometry.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Christoph Fröhlich <[email protected]>
|
Sorry for the delay in fixing the failing checks here! I was busy shifting Linux distros and had tried to make the commits that caused the checks to fail through the web. |
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
Still not sure about the tryUpdateOpenLoop method, maybe @saikishor or anyone else has an opinion about that.
Co-authored-by: Christoph Fröhlich <[email protected]>
|
@christophfroehlich pre-commit isn't working for me for some reason, but besides that I have made the changed you requested. If possible, could you clone the pr, and push a fix for the pre-commit? Thanks! |
christophfroehlich
left a comment
christophfroehlich
left a comment
There was a problem hiding this comment.
Thanks for the latest cleanup! LGTM
diff_drive_controller/include/diff_drive_controller/odometry.hpp
Outdated
Show resolved
Hide resolved
diff_drive_controller/include/diff_drive_controller/odometry.hpp
Outdated
Show resolved
Hide resolved
diff_drive_controller/include/diff_drive_controller/odometry.hpp
Outdated
Show resolved
Hide resolved
| catch (const std::runtime_error &) | ||
| { | ||
| odometry_message_.header.stamp = time; | ||
| odometry_message_.pose.pose.position.x = odometry_.getX(); | ||
| odometry_message_.pose.pose.position.y = odometry_.getY(); | ||
| odometry_message_.pose.pose.orientation.x = orientation.x(); | ||
| odometry_message_.pose.pose.orientation.y = orientation.y(); | ||
| odometry_message_.pose.pose.orientation.z = orientation.z(); | ||
| odometry_message_.pose.pose.orientation.w = orientation.w(); | ||
| odometry_message_.twist.twist.linear.x = odometry_.getLinear(); | ||
| odometry_message_.twist.twist.angular.z = odometry_.getAngular(); | ||
| realtime_odometry_publisher_->try_publish(odometry_message_); | ||
| // Handle exceptions when the time source changes and initialize publish timestamp | ||
| previous_publish_timestamp_ = time; | ||
| should_publish = true; | ||
| } |
There was a problem hiding this comment.
Is this block meant to catch the error of previous_publish_timestamp as unintialized ? if so, we should initialize it in on_configure. If not, this block may not be reachable.
There was a problem hiding this comment.
This isn't really a part of the changes made in this PR, perhaps create a separate issue? A similar pattern exists in other controllers as well, so the change will need to be propagated to them as well.
If you create another issue, please also link this suggestion in it.
There was a problem hiding this comment.
You are right, this existed before and the diff is just not properly rendered by the UI. @Juliaj please open a (first) issue with this.
|
@Amronos, thanks for working on this PR. It would be great to add some tests for the odometry changes. If you can't accommodate this within this PR, please consider creating a GitHub issue so the community can pick it up. |
Co-authored-by: Julia Jia <[email protected]>
Thanks a lot for the review! I don't really have the time to work on tests for this PR rn, and so have gone ahead and created #2033. |
3 participants
Fixes #271.
Fixes #357.
I have updated the odometry implementation of the
diff_drive_controllerto fix the above two issues alongside improving the code's readability. This implementation is similar to that ofomni_wheel_drive_controller.I have created some new methods and deprecated the older ones.
Should I also make similar changes to other controllers? If yes, should I do that in a separate PR?