clubcapra / markhor

🐐 Capra-Markhor is a ROS-based solution for managing and operating Club Capra's second rescue robot. 🐐
9 stars 1 forks source link

Add visualisation of the TPV by publishing joint state message #56

Open benmalenfant opened 1 year ago

benmalenfant commented 1 year ago

This new node publishes the joint_state from tpv_pos_x and tpv_pos_y (from the arduino motor controller) I tried my best to get the arduino to publish them but it caused communication instability

Files changed include the PR 55, changed file count should go down when we merge it

Leuchak commented 1 year ago

NOTE: once again, I'll just offer a suggestion. It's not against the pull request, but asking what you thing of a Object oriented approach.

There are some improvements that can be made:

  1. Code structure: The code can be structured better by separating the callback function into a classes. This will make the code more modular and easier to maintain.
  2. Code comments: The code can benefit from more comments to explain the purpose of each function an variable. It can be cumbersome, but it helps.

Example:

#include "ros/ros.h"
#include "sensor_msgs/JointState.h"
#include "std_msgs/Float64.h"
#include "math.h"

// Define named constants instead of magic numbers
const double X_REDUCTION = 0.25;
const double DEG_TO_RAD = M_PI / 180.0;

// Define a class for the joint state publisher
class JointStatePublisher
{
public:
    JointStatePublisher()
    {
        // Initialize the joint state message
        joint_state_msg_.name.push_back("tpv_base_j");
        joint_state_msg_.name.push_back("tpv_tilt_j");
        joint_state_msg_.position.push_back(0.0);
        joint_state_msg_.position.push_back(0.0);

        // Create the joint state publisher
        state_pub_ = nh_.advertise<sensor_msgs::JointState>("joint_states", 1);

        // Create the subscribers
        sub_x_ = nh_.subscribe("tpv_pos_x", 1, &JointStatePublisher::callback_x, this);
        sub_y_ = nh_.subscribe("tpv_pos_y", 1, &JointStatePublisher::callback_y, this);
    }

    void run()
    {
        ros::Rate loop_rate(10);

        while (ros::ok())
        {
            // Update the joint state message
            joint_state_msg_.header.stamp = ros::Time::now();

            // Publish the joint state message
            state_pub_.publish(joint_state_msg_);

            ros::spinOnce();

            loop_rate.sleep();
        }
    }

private:
    // Define the node handle
    ros::NodeHandle nh_;

    // Define the joint state publisher and message
    ros::Publisher state_pub_;
    sensor_msgs::JointState joint_state_msg_;

    // Define the subscribers and callback functions
    ros::Subscriber sub_x_;
    ros::Subscriber sub_y_;

    void callback_x(const std_msgs::Float64::ConstPtr& msg)
    {
        joint_state_msg_.position[0] = msg->data * DEG_TO_RAD * X_REDUCTION;
    }

    void callback_y(const std_msgs::Float64::ConstPtr& msg)
    {
        joint_state_msg_.position[1] = msg->data * DEG_TO_RAD;
    }
};

int main(int argc, char **argv)
{
    ros::init(argc, argv, "talker");

    // Create the joint state publisher object and run it
    JointStatePublisher joint_state_publisher;
    joint_state_publisher.run();

    return 0;
}

This updated version separates the joint state publisher into a class and adds some comments to explain the code.

again, I'm not asking to change the code. I'm asking what you think. I'd like us to develop our codding style. that we write it down and give examples.