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

Safer grip and add ungrip #646

Merged
merged 13 commits into from
Mar 29, 2024
Merged

Safer grip and add ungrip #646

merged 13 commits into from
Mar 29, 2024

Conversation

ykyohei
Copy link
Contributor

@ykyohei ykyohei commented Mar 21, 2024

Description

  • Add some documentations
  • Make grip safer
  • Add ungrip

Motivation and Context

https://github.com/simonsobs/chwp-discussions/discussions/17

How Has This Been Tested?

We need to test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@ykyohei ykyohei requested review from bbixler500 and jlashner March 21, 2024 15:36
@ykyohei ykyohei marked this pull request as ready for review March 21, 2024 23:34
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

A few comments from me related to documentation and the handling of session data in the new ungrip code. I'm also wondering if this has been tested yet.

socs/agents/hwp_gripper/agent.py Outdated Show resolved Hide resolved
socs/agents/hwp_gripper/agent.py Show resolved Hide resolved
socs/agents/hwp_gripper/agent.py Outdated Show resolved Hide resolved
socs/agents/hwp_gripper/agent.py Outdated Show resolved Hide resolved
socs/agents/hwp_gripper/agent.py Outdated Show resolved Hide resolved
@ykyohei
Copy link
Contributor Author

ykyohei commented Mar 27, 2024

I finished incorporating the comments from @BrianJKoopman and testing grip and ungrip in satp3.
I will ask @bbixler500 to review again because I changed the grip function little bit.

  • Move warm-grip-distance from -5 mm in the first step
  • Move in by 0.1 mm step, until warm-limit switch trigger, warm-grip-distance +1 mm at maximum
  • Changed (and hopefully simplified) the method to detect warm-limit switch triggering.
  • Added adjustment process. I would like to implement this process because satp3 limit switches are known to be slightly mis-aligned from ideal location little bit. You can do adjustment-distance = [0, 0, 0] if you do not need adjustment.

@ykyohei ykyohei requested a review from bbixler500 March 27, 2024 20:11
Copy link
Contributor

@bbixler500 bbixler500 left a comment

Choose a reason for hiding this comment

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

Yep looks good to me

@ykyohei ykyohei requested a review from BrianJKoopman March 27, 2024 20:39
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Changes related to my comments look good. @jlashner, did you still want to take a look at this?

Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

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

Hi Kyohei, I read this through and think it looks great, so I approve to merge as long as its been tested!

@ykyohei
Copy link
Contributor Author

ykyohei commented Mar 29, 2024

Thanks! Yes, I tested this in satp3.

@BrianJKoopman BrianJKoopman merged commit f6c0fa7 into main Mar 29, 2024
7 checks passed
@BrianJKoopman BrianJKoopman deleted the gripper_dev branch March 29, 2024 16:09
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.

4 participants