Skip to content

Improvements, fixes#19

Open
kriegljan wants to merge 11 commits intoipa320:masterfrom
kriegljan:improve
Open

Improvements, fixes#19
kriegljan wants to merge 11 commits intoipa320:masterfrom
kriegljan:improve

Conversation

@kriegljan
Copy link
Collaborator

Several bug fixes, improvements. API breaking: changes urdf finger joints and uses std::empty services instead of custom empty services. Applies support for wsg50-210 and possibly wsg50-68 by introducing a max_width parameter.

@kriegljan kriegljan mentioned this pull request Apr 26, 2023
@kriegljan
Copy link
Collaborator Author

@simonschmeisser what do you think of these changes?

<link name="${prefix}jaw_base"/>
<link name="${prefix}finger_base"/>

<joint name="${prefix}finger_left_joint" type="prismatic">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<joint name="${prefix}finger_left_joint" type="prismatic">
<joint name="${prefix}finger_left" type="prismatic">

we typically use finger_left for the joint and finger_left_frame for the link but I'll check if that is standardized somewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@simonschmeisser any update here?

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears not to be standardized. I'm fine with either one

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that finger_left is the default in WSG50ROSNode.cpp line 52 or so

#define MAXWIDTH 110.0
#endif
// #ifndef MAXWIDTH
// #define MAXWIDTH 110.0 -> use parameter instead to allow using wsg50-210 as well
Copy link
Collaborator

Choose a reason for hiding this comment

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

You probably want to remove that instead of commenting it out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, actually, I like the information why MAX_WIDTH is not set here, like it is done for all the other parameters of the WSG50-110.


// Gripper Default Values
//
#define MAXWIDTH 110.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need the maxwidth here and will this collide with a 210/68 gripper size?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is used in L.646 as default value, if you do not set the parameter max_width. Since that is the majority of cases for this driver, I kept it..

ros::param::param<std::string>("~ip", ip, DEFAULTIP);
ros::param::param<std::string>("~port", port, DEFAULTPORT);
ros::param::param<std::string>("~joint_name", jointName, DEFAULTJOINTNAME);
ros::param::param<std::string>("~opening_joint_name", openingJointName, DEFAULTOPENINGJOINTNAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ros::param::param<std::string>("~opening_joint_name", openingJointName, DEFAULTOPENINGJOINTNAME);

is this still used?

Copy link
Contributor

Choose a reason for hiding this comment

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

the macro in line 53 can also be removed

<link name="${prefix}jaw_base"/>
<link name="${prefix}finger_base"/>

<joint name="${prefix}finger_left_joint" type="prismatic">
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears not to be standardized. I'm fine with either one

@@ -638,13 +634,15 @@ int main(int argc, char** argv)
//
std::string ip, port;
std::string jointName, openingJointName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::string jointName, openingJointName;
std::string jointName;

<link name="${prefix}jaw_base"/>
<link name="${prefix}finger_base"/>

<joint name="${prefix}finger_left_joint" type="prismatic">
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that finger_left is the default in WSG50ROSNode.cpp line 52 or so

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