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

Enhanced documentation for composeMultiArrayMessage function #3270

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

attu0
Copy link

@attu0 attu0 commented Jan 25, 2025

Description
I have enhanced the documentation for the composeMultiArrayMessage function in common.cpp within the moveit_servo module.

Added a detailed Doxygen-compatible comment block for the function to clarify its purpose, parameters, return value, and usage.
The changes address the issue related to the lack of documentation for handling controllers that accept std_msgs::msg::Float64MultiArray messages.
This update improves developer experience and aligns with MoveIt 2's goal of becoming an industry-standard robotics framework.

Related Issue: #3243

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Looks great to me, just one little nitpick. I've been meaning to write some documentation myself for a while now

moveit_ros/moveit_servo/src/utils/common.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@sea-bass sea-bass 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 this! There is already a docstring in the common.hpp file (which is where docstrings go instead of the .cpp files, unless the function is not in the header at all):

/**
* \brief Create a Float64MultiArray message from given joint state
* @param servo_params The configuration used by servo, required for selecting position vs velocity.
* @param joint_state The joint state to be added into the Float64MultiArray.
* @return The Float64MultiArray message.
*/
std_msgs::msg::Float64MultiArray composeMultiArrayMessage(const servo::Params& servo_params,
const KinematicState& joint_state);

Could you please take these improvements and combine them with the existing one?

Comment on lines +259 to +261
* @brief Composes a Float64MultiArray message for controllers requiring Float64MultiArray input.
*
* This function converts the given joint state into a Float64MultiArray message.
Copy link
Contributor

Choose a reason for hiding this comment

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

these two sentences virtually say the same thing -- you probably can combine these into just a single line.

* Example:
* @code
* auto message = composeMultiArrayMessage(servo_params, joint_state);
* controller.publish(message);
Copy link
Contributor

@sea-bass sea-bass Jan 25, 2025

Choose a reason for hiding this comment

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

It may not be obvious to most people that controller in this case is actually a ROS 2 publisher. Maybe call this variable controller_pub or something like that, or add a brief sentence explaining that the code snippet is publishing to a controller topic?

Also looking at other examples, it appears you may need to indent what is inside the @code / @endcode?

For example:

* @code
* planning_scene_monitor::LockedPlanningSceneRO ls(planning_scene_monitor);
* moveit::core::RobotModelConstPtr model = ls->getRobotModel();
* @endcode

Which renders to: https://moveit.picknik.ai/main/api/html/classplanning__scene__monitor_1_1LockedPlanningSceneRO.html#details

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