Move to start calibration position #11478
Conversation
| pipette_id=params.pipetteId, | ||
| deck_coordinates=slot_center_deck, | ||
| direct=True, | ||
| additional_min_travel_z=0, |
There was a problem hiding this comment.
How should this parameter additional_min_travel_z be used? I put 0 in there temporarily, but I'm not sure what a reasonable value for this is
There was a problem hiding this comment.
Since this is an optional param, let's not set it and rely on motion_planning/ to handle any arc movement z minimums -- which is what this is used for. You can check out opentrons/motion_planning/ and protocol_engine/state/motion.py for further info.
Codecov Report
@@ Coverage Diff @@
## edge #11478 +/- ##
==========================================
+ Coverage 75.01% 76.95% +1.94%
==========================================
Files 2059 679 -1380
Lines 57481 11775 -45706
Branches 5999 2989 -3010
==========================================
- Hits 43122 9062 -34060
+ Misses 12950 1767 -11183
+ Partials 1409 946 -463
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Have a few initial comments. Also, I know we kept changing what the command was actually going to do, but I thought we had landed on making a generalized "move to location" command.
We should probably have this command take in an enum of sorts to specify what location to actual move to i.e.:
- probe position
- attach/de-attach probe position
We can add in an optional slot param as well and default to center slot for the probe position specifically. Can pair on this if you want.
api/Makefile
Outdated
| @@ -208,4 +208,4 @@ term: | |||
|
|
|||
| .PHONY: plot-session | |||
| plot-session: | |||
| $(python) util/plot_session.py $(plot_type) $(plot_type).pdf | |||
| $(python) util/plot_session.py $(plot_type) $(plot_type).pdf | |||
There was a problem hiding this comment.
Accidental change here?
There was a problem hiding this comment.
I would do:
git checkout origin/edge api/Makefile
git checkout origin/edge api/src/opentrons/protocol_engine/commands/__init__.py
To undo these stray formatting changes.
| direct=True, | ||
| additional_min_travel_z=0, | ||
| ) | ||
| return CalibrationSetUpPositionResult() |
There was a problem hiding this comment.
I think we want to return a position here, right?
| slot_center = self._state_view.labware.get_slot_center_position( | ||
| params.slot_name | ||
| ) | ||
| slot_center_deck = DeckPoint(x=slot_center.x, y=slot_center.y, z=slot_center.z) |
There was a problem hiding this comment.
Let's add a safety margin to the z component of the slot_center_deck. I think 3mm is probably OK for now.
| pipette_id=params.pipetteId, | ||
| deck_coordinates=slot_center_deck, | ||
| direct=True, | ||
| additional_min_travel_z=0, |
There was a problem hiding this comment.
Since this is an optional param, let's not set it and rely on motion_planning/ to handle any arc movement z minimums -- which is what this is used for. You can check out opentrons/motion_planning/ and protocol_engine/state/motion.py for further info.
| @@ -226,6 +226,7 @@ def get_slot_position(self, slot: DeckSlotName) -> Point: | |||
| def get_slot_center_position(self, slot: DeckSlotName) -> Point: | |||
| """Get the (x, y, z) position of the center of the slot.""" | |||
| slot_def = self.get_slot_definition(slot) | |||
| print("slot def =", slot_def) | |||
There was a problem hiding this comment.
accidentally left print statement here.
There was a problem hiding this comment.
Still have a print statement here.
| from opentrons.protocol_engine.execution import MovementHandler | ||
| from opentrons.protocol_engine.state.state import StateView | ||
|
|
||
| CalibrationSetUpPositionCommandType = Literal["CalibrationSetUpPosition"] |
There was a problem hiding this comment.
| CalibrationSetUpPositionCommandType = Literal["CalibrationSetUpPosition"] | |
| CalibrationSetUpPositionCommandType = Literal["calibrationSetUpPosition"] |
api/src/opentrons/protocol_engine/commands/calibration/calibration_set_up_position.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/calibration/calibration_set_up_position.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/calibration/calibration_set_up_position.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/calibration/calibration_set_up_position.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/calibration/calibration_set_up_position.py
Outdated
Show resolved
Hide resolved
api/tests/opentrons/protocol_engine/commands/calibration/test_calibration_set_up_position.py
Outdated
Show resolved
Hide resolved
caila-marashaj
force-pushed
the
move-to-start-calibration-position
branch
from
a62fee4
to
14c1c0a
Compare
caila-marashaj
requested review from
a team,
brenthagen and
ahiuchingau
and removed request for
a team
| """Calibration set up position command parameters.""" | ||
|
|
||
| slot_name: CalibrationPositions = Field( | ||
| CalibrationPositions.probe_position, |
There was a problem hiding this comment.
Let's make this a required parameter Field(..., description=...)
api/Makefile
Outdated
| @@ -208,4 +208,4 @@ term: | |||
|
|
|||
| .PHONY: plot-session | |||
| plot-session: | |||
| $(python) util/plot_session.py $(plot_type) $(plot_type).pdf | |||
| $(python) util/plot_session.py $(plot_type) $(plot_type).pdf | |||
api/src/opentrons/protocol_engine/commands/calibration/move_to_location.py
Show resolved
Hide resolved
| @property | ||
| def location(self) -> DeckPoint: | ||
| """Assign location value to CalibrationPositions with a 3mm safety margin.""" | ||
| if self.value == 5: |
There was a problem hiding this comment.
We should use the LabwareView class from the protocol engine to get the initial slot coordinates and then apply our own offsets as necessary. LabwareView has the functions get_slot_center_position and get_slot_definition which will be very useful.
We can then be more agnostic about what robot we might be 'moving to'
| @@ -175,6 +175,22 @@ async def move_relative( | |||
| position=DeckPoint(x=point.x, y=point.y, z=point.z), | |||
| ) | |||
|
|
|||
| # async def get_gantry_position(self, pipette_id: str) -> Point: | |||
There was a problem hiding this comment.
Commented out function, what was the point of this?
| @@ -226,6 +226,7 @@ def get_slot_position(self, slot: DeckSlotName) -> Point: | |||
| def get_slot_center_position(self, slot: DeckSlotName) -> Point: | |||
| """Get the (x, y, z) position of the center of the slot.""" | |||
| slot_def = self.get_slot_definition(slot) | |||
| print("slot def =", slot_def) | |||
There was a problem hiding this comment.
Still have a print statement here.
| def offset(self) -> Point: | ||
| """Return offset values for the given position.""" | ||
| if self.value == 5: | ||
| return Point(x=float(10), y=float(0), z=float(3)) |
There was a problem hiding this comment.
Why are these wrapped in floats? Also can we make these DeckPoints instead (see https://github.com/Opentrons/opentrons/pull/11567/files) where we're trying to avoid types from hardware controller or top level types.
| pipette_id: str, | ||
| position_id: str, | ||
| ) -> SavedPositionData: | ||
| """Return a SavedPositionData object.""" |
There was a problem hiding this comment.
As practice, we generally keep mock functions inside the test themselves if it's only being used by one test.
| from opentrons.protocol_engine.types import DeckPoint | ||
|
|
||
|
|
||
| probe_position = SavedPositionData( |
There was a problem hiding this comment.
When you move the test functions, you can also move these global variables to be inside the test as well.
api/src/opentrons/protocol_engine/commands/calibration/__init__.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/calibration/move_to_location.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/commands/calibration/move_to_location.py
Outdated
Show resolved
Hide resolved
…_.py Co-authored-by: Max Marrone <[email protected]>
…_location.py Co-authored-by: Max Marrone <[email protected]>
…_location.py Co-authored-by: Max Marrone <[email protected]>
caila-marashaj
force-pushed
the
move-to-start-calibration-position
branch
from
a7a5c0c
to
d72d81c
Compare
New command for moving the gantry before and after calibration. Parameter
CalibrationSetUpPositionslets the user move either to the middle of the deck for starting to probe, or the front for putting on and taking off the probe attachment. I'm using Deck Slot 2 for the front, and Deck Slot 5 for the middle, can anyone just confirm that these deck slots are the correct ones for those positions?Note: I have a comment in the files changed section attached to an argument value I wasn't sure about- please take a look!