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

Improvements, fixes #19

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

Improvements, fixes #19

wants to merge 11 commits into from

Conversation

ipa-jsk
Copy link
Collaborator

@ipa-jsk ipa-jsk commented Apr 26, 2023

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.

@ipa-jsk ipa-jsk mentioned this pull request Apr 26, 2023
@ipa-jsk
Copy link
Collaborator Author

ipa-jsk commented Apr 26, 2023

@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

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.

@@ -44,6 +41,7 @@

// Gripper Default Values
//
#define MAXWIDTH 110.0

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..

ipa325_wsg50/src/WSG50ROSNode.cpp Outdated Show resolved Hide resolved
@@ -638,13 +634,15 @@ int main(int argc, char** argv)
//
std::string ip, port;
std::string jointName, openingJointName;
double max_width;
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