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

Added IO Gripper Control and Configuration Messages, Services, and Actions #164

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

Conversation

sachinkum0009
Copy link

This pull request introduces new msgs, srvs and actions for control_msgs package. These changes are aimed at enhancing gripper control and configuration functionality.

control_msgs/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,6 @@
bool open
Copy link
Member

Choose a reason for hiding this comment

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

What if you want to control the percentage of the closure?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea. I will update it.

Copy link
Member

@destogl destogl Jan 2, 2025

Choose a reason for hiding this comment

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

@saikishor in this case you cannot control the percentage of the closure. As there are IOs controlled behind it and usually pneumatic gripper that might have different states, but those are then different IOs, in most cases those have only 2 states, open and close and not a chance to detect anything in between.

What here might be interesting, that we have different states besides open and close as we have 2 closed states and then we can set the target state of the gripper - bit this smells a lot to the configuration, so I am not sure if we should go this way, but rather we should keep only two states that we can command: open and close.

@@ -0,0 +1 @@
bool state
Copy link
Member

Choose a reason for hiding this comment

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

If it's just state, why not a percentage of closure instead of binary state

Copy link
Member

Choose a reason for hiding this comment

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

We don't know the percentage, usually don't. We can typically have discrete sensors for open/close, but not other sensors. Assuming that there is a case where this can be detected we would need to add additional interface for it. So I would wait until we have this use-case.

@@ -0,0 +1,4 @@
string config_name
Copy link
Member

Choose a reason for hiding this comment

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

How do you configure just based on string?. I think It is missing some documentation

Copy link
Member

Choose a reason for hiding this comment

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

@sachinkum0009 please add in general documentation about all fields in the message. Add those as comments at the begin of the message.

Copy link
Author

Choose a reason for hiding this comment

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

general documentation about all fields in the message added.

Copy link
Member

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

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

In my experience almost every single field in messages should be documented thoroughly otherwise we are asking for trouble.

uint8 state should be documented further by adding an "enum" to the message, of states to represent.

I'd also like some justification as to why this is needed and cannot be covered by existing messages.

Copy link
Member

Choose a reason for hiding this comment

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

This custom Gripper action maybe should be renamed to IOGripperCommand.

@bmagyar the reason to have a separate action here is that this kind of gripper has only open/close states, and nothing can be controlled much. Usually, those are the pneumatic ones so some cylinders are moving somehow, but we can not control intermediate states, i.e., "positions".

Looking at the existing Actions and having, e.g., JointState as input to the action (as done in ParallelGripperAction could be possible, but confusing and inconvenient. Possibly as a secondary interface, but as of now I don't see the usability of it, as we would command something implicitly that we cannot control.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I have updated the interface names.

Copy link
Member

@destogl destogl Jan 2, 2025

Choose a reason for hiding this comment

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

@bmagyar
There is no similar Action as of now. The IO Gripper might be quite complex, to have different configurations that are independent of open/close functionality. For example, configuration to grasp narrower and wider objects. In this case, the configuration has to be set before the grasping (and planning) as the collision model of the gripper changes by moving a degree of freedom through configuration.

Independently of configuration, the open/close stays the same, i.e., moving some other joints, but overall kinematics changes depending on the gripper configuration.

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 for the action.

@saikishor
Copy link
Member

@sachinkum0009 Can you fix the pre-commit?

@sachinkum0009
Copy link
Author

Sorry forget to run pre-commit.

control_msgs/action/IOGripperCommand.action Outdated Show resolved Hide resolved
control_msgs/action/SetIOGripperConfig.action Outdated Show resolved Hide resolved
Comment on lines 10 to 13
# - IDLE (0): The reconfiguration process is idle, not performing any action.
# - FIND_CONFIG (1): Finding the configuration settings.
# - SET_COMMAND (2): Setting the command based on the configuration.
# - CHECK_STATE (3): Checking the state after setting the command.
Copy link
Member

Choose a reason for hiding this comment

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

Please adjust as above.

Also, do we really need “find” state, I mean this takes a few microseconds so we will never get this feedback or see this. Or am I understanding something wrong?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I agree the "find" state should not take long and can be removed.

control_msgs/srv/SetIOGripperConfig.srv Outdated Show resolved Hide resolved
@destogl destogl changed the title Added Gripper Control and Configuration Messages, Services, and Actions Added IO Gripper Control and Configuration Messages, Services, and Actions Jan 11, 2025
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.

5 participants