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

fix odometry issue in diff_drive_controller #331

Merged
merged 5 commits into from
May 3, 2022
Merged

fix odometry issue in diff_drive_controller #331

merged 5 commits into from
May 3, 2022

Conversation

roncapat
Copy link
Contributor

Fixes #286 and the issue introduced in #260.

@roncapat
Copy link
Contributor Author

roncapat commented Apr 16, 2022

Some considerations:
updateFromVelocity() takes as input not velocities, but position deltas...
Lots of solutions here to transform a bit the Odometry API, but my idea is:

  • rename updateFromVelocity() to updateFromPositionDeltas()
  • rename update() to updateFromPosition()
  • then, add a proxy method updateFromVelocity() that keeps the same parameters and internally computes dt to avoid to explicitly multiply by update_dt.seconds() as this patch currently does

Or:

  • change the set of parameters for the different Odometry methods, as proposed in [DiffDriveController] use "period" argument in the update method insead of calculating "dt" #271.
  • I believe that doing this will make the Odometry class less robust though. For now, it is something that integrates feedback readings at specified (through parameter time) timepoints. Passing instead time deltas would remove some nice semantic... No way to check if for example the passed timepoint follows or precedes the previously provided timepoint. and of course, modifying the API to pass both (time and time delta) would be redundant.

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Just a minor comment.

Nevertheless, we should rename the method now, or, why not to change it accepts additionally dt?

Should this then make all other steps unnecessary, or am I missing something?

diff_drive_controller/src/diff_drive_controller.cpp Outdated Show resolved Hide resolved
@roncapat
Copy link
Contributor Author

I would go with the renaming scheme I proposed if it's ok for you.

@destogl
Copy link
Member

destogl commented Apr 19, 2022

I would go with the renaming scheme I proposed if it's ok for you.

We can do this if it is really needed. I would have to review the whole controller again to see the consequences.

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

This is already solving a problem which we need fixed. I'll go ahead with merging and @roncapat please submit a second PR with the renaming suggestions.

@bmagyar bmagyar requested a review from destogl May 3, 2022 06:43
@bmagyar bmagyar merged commit 5002208 into ros-controls:master May 3, 2022
mamueluth pushed a commit to StoglRobotics-forks/ros2_controllers that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong integration of velocity feedback in diff_drive_controller
3 participants