Skip to content

Conversation

@soham2560
Copy link

@soham2560 soham2560 commented Mar 6, 2025

Brief

In this PR

Note: Refer this comment for exact motivation

@mergify

This comment was marked as resolved.

@soham2560 soham2560 marked this pull request as ready for review March 7, 2025 02:46
@soham2560
Copy link
Author

I do have some questions about this implementation though

let me know if anything is to be done about this

@christophfroehlich
Copy link
Member

I do have some questions about this implementation though

* the odom transforms seems to be publishing to a non-standard topic `~/tf_odometry`
  https://github.com/ros-controls/ros2_controllers/blob/11ed083dbc1126d8db03d90c32395ee18e6e4be4/mecanum_drive_controller/src/mecanum_drive_controller.cpp#L165

* there is no unstamped cmd vel topic, I believe we need that for humble? (also the stamped cmd vel topic is `~/reference` by default)

let me know if anything is to be done about this

The non-default topics are used by other controllers here to, one has to remap them if to be used in the "standard" way.
About unstamped cmd_vel, you are maybe right. Have a look how the switch is implemented in diff_drive_controller for example.

@soham2560
Copy link
Author

@christophfroehlich I have added stamped vel support (refer 97e8ba9), can you have a look?

Copy link
Member

@christophfroehlich christophfroehlich 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 the changes. I tested the package locally, it seems to be fine

@christophfroehlich christophfroehlich merged commit eacbfa0 into ros-controls:mergify/bp/humble/pr-512 Mar 7, 2025
1 check passed
@christophfroehlich
Copy link
Member

@soham2560 please install pre-commit and run the checks the next time before opening a PR ;)

@soham2560
Copy link
Author

@soham2560 please install pre-commit and run the checks the next time before opening a PR ;)

oh I completely missed that, had run it but it skipped al the files since they were already commited, just checked, should've used --all-files flag in that scenario, thanks for the pointer will keep it in mind

@soham2560 soham2560 deleted the fix/mergify/bp/humble/pr-512 branch March 8, 2025 02:52
@soham2560
Copy link
Author

The non-default topics are used by other controllers here to, one has to remap them if to be used in the "standard" way.

@christophfroehlich apologies for commenting here, but is remapping possible on humble if the spawner does not support the --ros-args arg like in ros-controls/ros2_control#1713

@saikishor
Copy link
Member

@christophfroehlich apologies for commenting here, but is remapping possible on humble if the spawner does not support the --ros-args arg like in ros-controls/ros2_control#1713

You can remap by adding the remapping arg to ros2_control_node.

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.

3 participants