feat(api): add internal xy belt calibration method #12204
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #12204 +/- ##
=======================================
Coverage 73.79% 73.79%
=======================================
Files 2203 2203
Lines 60602 60602
Branches 6245 6245
=======================================
Hits 44724 44724
Misses 14441 14441
Partials 1437 1437
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Think we've got a couple math issues here, put it inline.
We should also maybe make calibrate_belts use opentrons.util.linal.solve_attitude, but this implementation once fixed will also work. However, if we want to keep this implementation, we should refactor lines 882-889 into their own function that takes a list of actual points and a list of nominal points and returns the attitude and test it... which is basically linal.solve_attitude
|
|
||
| return ( | ||
| ((point_c.x - point_a.x) / nominal_x), | ||
| ((point_b.x - point_a.x) / nominal_y), |
There was a problem hiding this comment.
This doesn't look correct, right? It doesn't make sense to divide an x distance by a y distance, or a y distance by an x distance.
| return ( | ||
| ((point_c.x - point_a.x) / nominal_x), | ||
| ((point_b.x - point_a.x) / nominal_y), | ||
| ), (((point_c.y - point_a.y) / nominal_x), ((point_b.y - point_a.y) / nominal_y)) |
There was a problem hiding this comment.
In general, I think the math looks good. However, if you want to utilize the capacitive probe to perform these functions, you'll actually need to add the probe length before doing that.
Take a look at calibrate_pipette for reference. Also @sfoster1 , I am assume we'll want to make sure we're not baking in any offsets from instrument calibration?
If this is the case, we should also make sure to reset the instrument offset (which again can be found in the calibrate_pipette function.
| The actual and nominal centers of the specified slot. | ||
| """ | ||
| nominal_center = _get_calibration_square_position_in_slot(slot) | ||
| z_height = await find_deck_height(hcapi, mount, nominal_center) |
There was a problem hiding this comment.
Are we using a pipette to find the deck height? As this stands now, the probe length is not taken into account in this function.
There was a problem hiding this comment.
That should be handled by the machine->deck transformation right? Or is this an old comment
There was a problem hiding this comment.
This is specifically regarding probing the deck w/ a capacitive probe.
There was a problem hiding this comment.
And yes it's taken care of now in calibrate_belts
|
If this test should only be performed using the pipette mounts, we should also throw an error if a gripper mount is passed in. |
| """ | ||
| if mount == OT3Mount.GRIPPER: | ||
| raise RuntimeError("Must use pipette mount, not gripper") | ||
| try: |
There was a problem hiding this comment.
rather than have a whole bunch of stuff inside a try block, let's put all that stuff in a function and just call the function in a try block
| if mount == OT3Mount.GRIPPER: | ||
| raise RuntimeError("Must use pipette mount, not gripper") | ||
| try: | ||
| await hcapi.reset_instrument_offset(mount) |
There was a problem hiding this comment.
I have three problems with this:
First, if we want to reset instrument offset to default, we need to pass to_default=True here; otherwise, this will just reload the current saved instrument offset.
Second, if we want to reset the instrument offset to default before calibration, we need to reload it from disk afterwards with an await hcapi.reset_instrument_offset(mount) in the finally-block.
Finally, we don't actually need to do reset the instrument offset here because we're calculating a linear transform, so any offset applied equally to all points will fall out.
I think on balance we should just remove this call, but if we want to keep it we need to fix the to_default and not resetting it afterwards thing.
Overview
Run automatic calibration for the gantry x and y belts attached to the specified mount. Returned linear transform matrix is determined via the actual and nominal center points of the back right (A), front right (B), and back left (C) slots.
Test Plan
Changelog
Review requests
Risk assessment