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

Implementing joint state publishing for 3d #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

D-Pino
Copy link
Member

@D-Pino D-Pino commented Aug 4, 2020

No description provided.

@D-Pino D-Pino requested a review from sarika93 August 4, 2020 21:24
Copy link
Member

@sarika93 sarika93 left a comment

Choose a reason for hiding this comment

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

Not done reviewing (need to consider your class structure some more) but there are enough things here to address for now.

An additional note: in general, when using other people's work, it should be referenced. Include a link to the repo where you got the URDFs from in your readme.

robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/urdf/lrmate200ic.xacro Show resolved Hide resolved
@D-Pino D-Pino requested a review from sarika93 August 5, 2020 20:02
Copy link
Member

@sarika93 sarika93 left a comment

Choose a reason for hiding this comment

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

Most requested changes are related to restructuring your class. Let me know if you have any questions about the motivation for doing this, or if anything isn't clear.

robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
robot_calibration_3d/src/joint_state_publisher.cpp Outdated Show resolved Hide resolved
@D-Pino D-Pino requested a review from sarika93 August 6, 2020 20:42
last_update_time_ += state_update_interval_duration;

// update count to return
count++;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing any reason for this to be tracked outside of the class; especially as the number of joint angle updates we want overall is stored in the class. Instead, make this a private class variable that you can update every time an update is made. Update this function to have return type void and take no input arguments.

ros::Rate loop_rate(10);

// start counting updates
int count = 0;
Copy link
Member

Choose a reason for hiding this comment

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

See comment in joint_state_publisher.cpp about moving this there instead.

Comment on lines 75 to 76
// fill array with 6 new random angles
// TODO: retrieve angle limits from URDF, rather than hardcoding limits as is done in this function
Copy link
Member

Choose a reason for hiding this comment

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

Move these comments to the updateJointStateMessage function, for greater clarity.

@@ -0,0 +1,89 @@
<?xml version="1.0"?>
<robot xmlns:xacro="http://wiki.ros.org/xacro">
<xacro:include filename="$(find fanuc_resources)/urdf/common_colours.xacro"/>
Copy link
Member

Choose a reason for hiding this comment

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

Currently looking for the fanuc_resources package. Update to robot_calibration_3d

joint_states_message_.name.push_back("joint_" + std::to_string(i));

// initialise the joint state array of the message with random values for first update
// TODO: retrieve angle limits from URDF, rather than hardcoding limits as is done in this function
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as below; this comment should be moved to the function, so you don't have to duplicate this comment every time you use this function.

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.

2 participants