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

feat(api): segment 96-channel tip pickup and dropoff moves #13669

Merged
merged 19 commits into from
Oct 20, 2023

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Sep 27, 2023

Overview

The 96 channel pipette can execute tip handling more efficiently if we split the tip motor's moves into two segments. This pr changes both pickup and dropoff so that we will do this by default when using a 96 channel.

This also includes a significant refactor of the dataclasses that pass information around for tip pickup and dropoff, since it would make these changes easier.

Changelog

  • all pickup and dropoff will use a more generic TipActionSpec, which will contain a list of TipActionMoveSpec objects
    • refactored plan_check_pick_up_tip and plan_check_drop_tip to accommodate this
  • setting the currents during the larger 'pick up tip' or 'drop tip' command, rather than within each press, droptipmove, or tip motor move. I did this because the currents appeared to be the same for each of these actions anyway, please let me know if this could potentially mess anything up.
  • for the 96 channel, moving the tip motors will use a singular common helper function- this gets rid of the _motor_pick_up_tip helper.
  • added new configs to shared data to help with this
    • prep_move_distance and prep_move_speed for the first half of tip action moves, which will be slightly faster than when directly approaching the tiprack
  • changed tiprack_up and tiprack_down to end_tip_action_retract_distance_mm and connect_tiprack_distance_mm(I still don't love this name I'm open to feedback here), in an effort to hopefully be more self-explanatory now that the code has grown and there's a bunch of other configs and variables with the word 'tiprack' in them

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #13669 (759620c) into edge (5dc45fd) will decrease coverage by 0.11%.
Report is 68 commits behind head on edge.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13669      +/-   ##
==========================================
- Coverage   70.75%   70.65%   -0.11%     
==========================================
  Files        2469     2477       +8     
  Lines       69482    69780     +298     
  Branches     8375     8454      +79     
==========================================
+ Hits        49162    49302     +140     
- Misses      18345    18492     +147     
- Partials     1975     1986      +11     
Flag Coverage Δ
app 68.14% <ø> (+<0.01%) ⬆️
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 45.34% <ø> (-0.41%) ⬇️
shared-data 74.00% <100.00%> (-0.22%) ⬇️
step-generation 84.99% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
api/src/opentrons/hardware_control/ot3api.py 79.47% <ø> (ø)
...pentrons_shared_data/pipette/pipette_definition.py 94.00% <100.00%> (+0.16%) ⬆️

... and 102 files with indirect coverage changes

@caila-marashaj caila-marashaj changed the title refactor(api): split 96 pickup and dropoff feat(api): split 96 pickup and dropoff Sep 27, 2023
@caila-marashaj caila-marashaj changed the title feat(api): split 96 pickup and dropoff feat(api): segment 96-channel tip pickup and dropoff moves Sep 27, 2023
@caila-marashaj caila-marashaj marked this pull request as ready for review September 27, 2023 17:47
@caila-marashaj caila-marashaj requested review from a team as code owners September 27, 2023 17:47
Copy link
Contributor

@ahiuchingau ahiuchingau left a comment

Choose a reason for hiding this comment

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

MoveManager can take in a list of targets, i think we can make use of that here

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Looks good iif it works, thoguh I'm wondering if maybe we're beyond the point of plan_check_drop_tip handling both motor and force drop being helpful

@caila-marashaj caila-marashaj force-pushed the multi-move-96-pickup-dropoff branch from a489f8d to 545480d Compare October 3, 2023 21:20
@caila-marashaj caila-marashaj force-pushed the multi-move-96-pickup-dropoff branch from 3651d8d to 4b0b660 Compare October 12, 2023 16:48
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

I like the impulse to refactor this code, but this has lost a lot of management of motor currents.

For force-pickup, we use a much lower current than normal; we need to set the low current (uinstrument.pick_up_configurations.current on the Z axis when pressing into the tip, and the default Z run current when raising out of the tip, because the low current may not be enough to move the Z.

For force-drop, we need to be using a much higher current on the plunger when interacting with the shroud (instrument.drop_configurations.current)

await self._motor_pick_up_tip(realmount, spec.pick_up_motor_actions)
else:
await self._force_pick_up_tip(realmount, spec)
async with self._backend.motor_current(
Copy link
Member

Choose a reason for hiding this comment

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

We can't set the current like this, I think (know). We purposefully set the current of the Z much lower than normal when driving into tips to pick them up so we can use the motors as a force clutch. We should not use the same current when we're going back up after the main press.

plunger_prep_pos=instrument.plunger_positions.bottom,
plunger_currents={
TipActionSpec(
currents={
Axis.of_main_tool_actuator(
mount
): instrument.plunger_motor_current.run
Copy link
Member

Choose a reason for hiding this comment

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

this current isn't the right one - in pick up tip, for force pickups, it's the current of the z that's important, not the current of the plunger. Note that after this refactor, there's no reference to instrument.pick_up_configurations.current outside of Axis.Q.

is_96_chan,
)
currents = {
Axis.of_main_tool_actuator(mount): instrument.plunger_motor_current.run
Copy link
Member

Choose a reason for hiding this comment

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

this current is wrong; when dropping tip, we use a much higher current on the plunger motor than normal because we need to apply enough force to pip the tips off. That's instrument.drop_configurations.current.

@caila-marashaj caila-marashaj force-pushed the multi-move-96-pickup-dropoff branch from b456e31 to 0564353 Compare October 16, 2023 23:06
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Comments inline. Two big things

  • We definitely are not getting the currents correct, still. During a force press pickup, the z current is what defines the tip attach force and what we need to control during the downward stroke. During a force tip drop, the plunger current defines the applied force and what we need to control during the plunger downward stroke
  • We're doing this thing where we decide if the pipette is a 96 twice, separately, in ot3api and in the pipette handler. We should do it in only one of those places and I think the right place is the ot3api, by gantry load. Then, let's make the functions in the pipette handler separate - one for 96, one for low throughput. One fewer place to change if we ever need to.

TipActionMoveSpec
] # can be presses, gear pickup, or gear dropoff
shake_off_moves: List[Tuple[top_types.Point, Optional[float]]] # this is the same
z_distance_to_tiprack: Optional[float] = None
Copy link
Member

Choose a reason for hiding this comment

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

can these be moves?

api/src/opentrons/hardware_control/ot3api.py Show resolved Hide resolved
api/src/opentrons/hardware_control/ot3api.py Outdated Show resolved Hide resolved
@caila-marashaj caila-marashaj force-pushed the multi-move-96-pickup-dropoff branch from 1537efb to e160ecb Compare October 18, 2023 20:33
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Okay, I'm liking this a bit more, but I think we need to establish some test criteria. Why don't we say:

  • Pick up a tip on a single and multi and check the canbus logs to make sure we're setting the current from config
  • Drop a tip on a single and multi and check the canbus logs to make sure we're setting the current from config
  • Check that 96 pickup and drop works with the segmented moves

I'm most concerned about LT regressions as you can probably tell.

@caila-marashaj caila-marashaj merged commit 1e91e5a into edge Oct 20, 2023
39 of 46 checks passed
@caila-marashaj caila-marashaj deleted the multi-move-96-pickup-dropoff branch October 20, 2023 17:42
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.

3 participants