-
Notifications
You must be signed in to change notification settings - Fork 183
feat(api): save gripper jaw width to robot fs #18532
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
Conversation
7d73c67
to
cd7a2c9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18532 +/- ##
=======================================
Coverage 24.75% 24.75%
=======================================
Files 3284 3284
Lines 285464 285464
Branches 28663 28663
=======================================
Hits 70662 70662
Misses 214781 214781
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
@@ -56,6 +56,7 @@ def reset_gripper(self) -> None: | |||
self._gripper = new_gripper | |||
|
|||
async def reset(self) -> None: | |||
# TODO(cm): do we want to reset the gripper jaw width here as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Request
do we want to reset the gripper jaw width where we do the calibration data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but that's not here - it's in reset_instrument_offset
. this reset
is part of the instrument load mechanics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, Are you asking if we should clear what's on the filesystem, or just what's in-memory? What are the implications of clearing what's in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say maybe we clean out old gripper jaw width files during instrument calibration. Would we ever want to keep more than one of these files at a time anyways? It would kind of be beneficial to keep some old ones so that if a previously used gripper gets re-installed it's old jaw width data gets reloaded, but maybe that should "always be remeasured" anyways when doing calibration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just what's on the filesystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and like a nice speedup. Let's make sure that it all works during gripper calibration, both when the calibration is the first thing you're doing after adding a new gripper (i.e. you just unboxed the gripper and have never used it on this machine before) and during a rerun.
@@ -56,6 +56,7 @@ def reset_gripper(self) -> None: | |||
self._gripper = new_gripper | |||
|
|||
async def reset(self) -> None: | |||
# TODO(cm): do we want to reset the gripper jaw width here as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but that's not here - it's in reset_instrument_offset
. this reset
is part of the instrument load mechanics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat idea!
Changes requested for the combination of a couple of things:
- We're reusing jaw width across grippers?
- After jaw width is saved once, it's reused forever and never cleared or overwritten?
I just skimmed through this, so let me know if I'm misunderstanding.
@@ -56,6 +56,7 @@ def reset_gripper(self) -> None: | |||
self._gripper = new_gripper | |||
|
|||
async def reset(self) -> None: | |||
# TODO(cm): do we want to reset the gripper jaw width here as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, Are you asking if we should clear what's on the filesystem, or just what's in-memory? What are the implications of clearing what's in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good, left small comment
Mistaken about model needing a serial number field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool! Left a few comments regarding file management and stuff.
@@ -56,6 +56,7 @@ def reset_gripper(self) -> None: | |||
self._gripper = new_gripper | |||
|
|||
async def reset(self) -> None: | |||
# TODO(cm): do we want to reset the gripper jaw width here as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say maybe we clean out old gripper jaw width files during instrument calibration. Would we ever want to keep more than one of these files at a time anyways? It would kind of be beneficial to keep some old ones so that if a previously used gripper gets re-installed it's old jaw width data gets reloaded, but maybe that should "always be remeasured" anyways when doing calibration?
This comment was marked as resolved.
This comment was marked as resolved.
69e0903
to
277536c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but let's add that one test
api/src/opentrons/hardware_control/instruments/ot3/instrument_calibration.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this with fresh eyes, I have some late feedback. Sorry to throw this in now. We can talk about in-person and see what you think.
def reset_jaw_width_calibration(self, to_default: bool) -> None: | ||
"""Temporarily reset the gripper jaw width measurement.""" | ||
if to_default: | ||
loaded_jaw_width_data = load_gripper_jaw_width(gripper_id=None) | ||
else: | ||
loaded_jaw_width_data = load_gripper_jaw_width(gripper_id=self._gripper_id) | ||
self.update_jaw_open_position_from_closed_position( | ||
jaw_at_closed=loaded_jaw_width_data.encoder_position_at_jaw_closed | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the waters here are getting muddied for which methods change things temporarily vs. persistently, and for at what point persistent data is loaded.
e.g. this docstrings says it "temporarily resets the gripper jaw width measurement." What it actually does now, though, is maybe change the data persistently?
reset_jaw_width_calibration(to_default=True)
- which calls
self.update_jaw_open_position_from_closed_position(some default loaded_jaw_width_data)
- which calls
self.save_gripper_jaw_width_data(some default loaded_jaw_width_data)
There's similar iffiness in @property has_jaw_width_calibration
, which calls self.update_jaw_open_position_from_closed_position()
to update the in-memory data from the on-filesystem data, and, again self.update_jaw_open_position_from_closed_position()
can write to the filesystem. So reading gripper.has_jaw_width_calibration
can actually change state, both in-memory state and on-filesystem state.
39f7d2b
to
0e6d75c
Compare
0e6d75c
to
ebe3acc
Compare
Overview
Gripper speedup alert 💀 🚨 🚧⚠️
Due to mechanical tolerances in the 'grip' axis on the robot, each individual gripper needs to run a calibration routine in order to determine its max width and be able to establish position tracking. This routine has the gripper jaw open all the way until the limit switch is triggered, and then close all the way. The software assumes that the distance between the two gripper paddles is 60mm when closed. It then uses the encoder value at the position the gripper is found to be closed to calculate the max jaw width of the specific gripper that's on the robot.
The problem is that this value lives in the hardware controller, and therefore is wiped from existence any time
OT3API
is reset. We can reduce the number of times this routine happens by storing the gripper jaw calibration result to the robot file system and looking it up, if the gripper doesn't already have it.Changelog
gripper_jaw_width_data
. This will store the encoder value at the gripper's closed positionGripper::has_jaw_width_calibration
, look for data in this file, and return true + update theGripper
instance at hand if it's thereTesting
/data/robot
with agripper_jaw_width_data
folder and also data files if not already present