Add multi_omni_wheel_drive_controller #1535
Conversation
github-actions
bot
requested review from
anfemosa,
aprotyas,
christophfroehlich,
destogl,
harderthan,
Maverobot and
mcbed
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Thanks a lot for your contribution.
Why you named your controller multi_omni_wheel_drive_controller? why not just omni_wheel_drive_controller, or omni_drive_controller like our friends at PAL do call it?
The first review round, where I only had a look at the docs and the description. I just added some stylistic comments as well as changes to use standard nomenclature (like state interfaces instead of feedback). Just in case you don't know, you can commit them all at once from the "Files Changed" tab here.
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
I didn't name it |
|
Please review! |
|
May I cite our contribution guidelines:
|
There was a problem hiding this comment.
@Amronos I did a quick partial review of the PR. Thanks for the great work. I've highlighted a couple of things in my review.
Apart from that, there are not many tests for your different robot cases. Please correct me if I'm wrong. It being a very generic controller, i think it would make sense to split the main kinematics logic into a different library within the same package that is completely ROS-Free and then use that API from within the controller
This should help you to write more test cases easily and also gives us a nice self contained library. What do you think?
multi_omni_wheel_drive_controller/multi_omni_wheel_drive_plugin.xml
Outdated
Show resolved
Hide resolved
| <name>multi_omni_wheel_drive_controller</name> | ||
| <version>0.0.0</version> | ||
| <description>TODO: Package description</description> | ||
| <maintainer email="[email protected]">Aarav Gupta</maintainer> |
There was a problem hiding this comment.
Please add all other maintainers here as well + Add yourself as author.
multi_omni_wheel_drive_controller/src/multi_omni_wheel_drive_controller_parameters.yaml
Outdated
Show resolved
Hide resolved
| { | ||
| type: string, | ||
| default_value: "", | ||
| description: "(optional) Prefix to be appended to the tf frames, will be added to odom_id and base_frame_id before publishing. If the parameter is empty, controller's namespace will be used.", |
There was a problem hiding this comment.
Why use the controller's namespace? Do you have any usecase for it?
There was a problem hiding this comment.
I often use the robot_localization package, which allows me to fuse the odometry from this controller with, for example, the data from an IMU. I like the ekf_node from that package to publish on the /odom topic and listen to the /<controller_name>/odom topic.
multi_omni_wheel_drive_controller/src/multi_omni_wheel_drive_controller_parameters.yaml
Outdated
Show resolved
Hide resolved
multi_omni_wheel_drive_controller/src/multi_omni_wheel_drive_controller_parameters.yaml
Outdated
Show resolved
Hide resolved
| publish_rate: | ||
| { | ||
| type: double, | ||
| default_value: 50.0, # Hz | ||
| description: "Publishing rate (Hz) of the odometry and TF messages.", | ||
| } |
There was a problem hiding this comment.
@Amronos does it make sense to have a seperation between Odometry and tf publish rates?
There was a problem hiding this comment.
I suppose it does make some sense to have them separated, but I would also like to see a use case for this.
The whole concept of publish_rate probably requires a larger discussion.
| pose_covariance_diagonal: | ||
| { | ||
| type: double_array, | ||
| default_value: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0], | ||
| description: "Odometry covariance for the encoder output of the robot for the pose. These values should be tuned to your robot's sample odometry data, but these values are a good place to start: ``[0.001, 0.001, 0.001, 0.001, 0.001, 0.01]``.", | ||
| } | ||
| twist_covariance_diagonal: | ||
| { | ||
| type: double_array, | ||
| default_value: [0.0, 0.0, 0.0, 0.0, 0.0, 0.0], | ||
| description: "Odometry covariance for the encoder output of the robot for the speed. These values should be tuned to your robot's sample odometry data, but these values are a good place to start: ``[0.001, 0.001, 0.001, 0.001, 0.001, 0.01]``.", |
There was a problem hiding this comment.
Does it make sense to have a structure like
diagonal_covariance:
pose: ....
twist: ....
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
I try my best to do so whenever I get the time. I have reviewed 3 or 4 PRs here and there. I don't review most of them as I lack the necessary skills/knowledge about the |
Thank you so much for the review!
Will work on both of these things. |
|
@Amronos please next time apply the suggestion directly from our suggestions. If not, we'll have to recheck things and this will be easier for both parties. I hope you understand. Thank you |
I do think that's what I did, by going to the files changed tab, adding suggestions to batch and then commiting. Please do tell if I needed to do something else. |
I apologise. From my mobile it looked different. Thanks for confirming :) |
@saikishor Thinking about this more: For the library, I don't think it makes sense (if there are no additional tests to write) to have a different library within this package, as anyone who will want to be using the kinematics will need to download the Please tell me your opinion on this! |
I understand that if it works for one robot, it should work for others too, but as the package specifies it is a multi_omni_wheel_drive_controller. I expect atleast tests with 3 different base configurations and their odometry properly tested.
I'm talking for the sake of tests to use something like this as a ROS independent library. Sure, if you are fine to do it in future, we can leave it for future, but please add tests for more configurations of the base kinematics and odometry tests. |
|
Thanks a lot for understanding! I will go ahead and add 3-4 more tests but it will take some time as I won't be at home for a few days. |
|
@saikishor I have added the tests requested with 86b351e |
This is a controller for mobile robots with omnidirectional drive. It supports using 3 or more omni wheels spaced at an equal angle from each other in a circular formation.
I have added many tests similar to other controllers and updated the docs.
Please review this controller and tell me if you have any doubts regarding its working.
I will be updating mobile_robot_kinematics.rst for the kinematics of this controller and for that wanted to ask what software has been used to make the diagrams in that doc. 😄