refactor(api): ot3: add gripper critical points #11024
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #11024 +/- ##
=======================================
Coverage 73.78% 73.78%
=======================================
Files 2076 2076
Lines 57309 57332 +23
Branches 5726 5731 +5
=======================================
+ Hits 42283 42304 +21
- Misses 13788 13790 +2
Partials 1238 1238
Flags with carried forward coverage won't be shown. Click here to find out more.
|
sfoster1
force-pushed
the
RET-1091-no-more-required-enalbe
branch
from
849e971 to
6eea14c
Compare
sfoster1
force-pushed
the
RET-1019-gripper-critical-points
branch
2 times, most recently
from
573fcb2 to
f3633c6
Compare
There was a problem hiding this comment.
Initial thoughts about naming for offsets. We can push these decisions to later as well, or to a wider group but if we're only ever using one pin to calibrate then we should think about reducing the number of sensors as well as only have 1 pin available to add to the gripper in the mechanical design -- otherwise I could see it getting confusing for users.
| self._jaw_center_offset = ( | ||
| Point(*self._config.jaw_center_offset_from_base) + base_offset | ||
| ) | ||
| self._front_pin_offset = ( |
There was a problem hiding this comment.
can we call this _front_calibration_pin_offset and _back_calibration_pin_offset respectively?
There was a problem hiding this comment.
Also if we're only ever going to use one pin to calibrate, should we decide which one and only track that offset? Is the gripper going to be redesigned to only have 1 pin?
There was a problem hiding this comment.
we should keep these until we bring up the actual calibration process.
| """ | ||
| # TODO: add critical point implementation | ||
| return Point(0, 0, 0) | ||
| if cp_override == CriticalPoint.GRIPPER_FRONT_CALIBRATION_PIN: |
There was a problem hiding this comment.
If we do decide to just pick one location, can we call this: CriticalPoint.GRIPPER_CALIBRATION?
| ) | ||
| self._back_pin_offset = ( | ||
| Point(*self._config.pin_two_offset_from_base) + base_offset | ||
| ) | ||
| self._calibration_offset = gripper_cal_offset |
There was a problem hiding this comment.
Can we change this to: calibrated_point or something? Makes me think that this is a default configuration rather than a value we got from calibrating the thing.
There was a problem hiding this comment.
it's not a point though, it's the offset you get from calibration that's added into the rest of the critical point offsets. i'm not sure what else to call it, but maybe comments would help?
There was a problem hiding this comment.
hmm comments are fine for now, I can't think of a better name at the moment other than maybe _mount_to_calibration_pin_offset which is kinda long lol.
| if not self._gripper: | ||
| return Point(0, 0, 0) | ||
| else: | ||
| if self._gripper and cp_override != CriticalPoint.MOUNT: |
There was a problem hiding this comment.
Discussed in a DM, but we shouldn't have a Mount critical point for the gripper since the mount w/o a gripper attached cannot move independently of the XY axes and also doesn't have Z movement.
For now, add some gripper-specific critical points to the enum that the gripper handles, and use them to calculate position offsets for motion. JAW_CENTER is the default, and will be what gets used in most calls when a gripper is present; the calibration points will be used for calibration.
- It's no longer valid to use the MOUNT cp on a gripper - There are actual exceptions for using a gripper cp on a pipette and - vice versa
sfoster1
force-pushed
the
RET-1019-gripper-critical-points
branch
from
8d885d7 to
21b6c37
Compare
For now, add some gripper-specific critical points to the enum that the
gripper handles, and use them to calculate position offsets for motion.
JAW_CENTER is the default, and will be what gets used in most calls when
a gripper is present; the calibration points will be used for calibration.
These new critical points will cause an error if used with pipettes, and the pipette critical points will cause an error if used with the gripper.
It's no longer valid to move the mount critical point for the gripper, since that criticalpoint is attached directly to the carriage.
These values are tested on a proto build.