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

fix(api): only add tip if pickup successful #15143

Closed

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented May 9, 2024

Overview

Accompanies https://opentrons.atlassian.net/browse/EXEC-432

Currently in ot3api during a call to pick_up_tip, a tip is cached to the present pipette regardless of whether the pickup succeeded. This was fine, because if the pickup failed, then the current run would end and all instrument information gathered over the course of the run would be erased.

If we want to recover from this and keep the same instances of Pipette, though, we should just verify the tip presence before caching information in the api.

This will probably want to change in the future as we build out tip_handler and do more to accommodate partial tip pickup, but I think this is fine for now.

@caila-marashaj caila-marashaj requested a review from a team as a code owner May 9, 2024 15:07
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

I'm kind of confused about how this was operating before. If there was no tip present, I'm presuming OT3API.pick_up_tip() raised an exception—is that correct? If so, where did the exception propagate out of? Since add_tip_to_instr() was one of the last things pick_up_tip() did, I don't see how the tip pickup could have failed in a way that add_tip_to_instr() still ran.

@caila-marashaj
Copy link
Contributor Author

I'm kind of confused about how this was operating before. If there was no tip present, I'm presuming OT3API.pick_up_tip() raised an exception—is that correct? If so, where did the exception propagate out of? Since add_tip_to_instr() was one of the last things pick_up_tip() did, I don't see how the tip pickup could have failed in a way that add_tip_to_instr() still ran.

The hardware api itself doesn't check- whatever is calling it will call verify_tip_presence afterward- this is how tip_presence.py does it for the protocol engine. The slightly messy thing about this is that the tip information gets held in a Pipette object, which belongs to the api.

So as it exists now, the parts of the code that would normally be verifying a tip pickup at the end, don't have the power to cache the tip info themselves.

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.

Like the comment inline said, I don't think this is the right way to solve the problem, because

  • it can cause an exception where one was not caused before (if the tip pickup failed, there won't be a tip, and prep_for_aspirate() will raise)
  • even if that wasn't true, it changes an invariant that previously held (after you call pick_up_tip, the pipette will have a tip)

I think this actually might be the first case where what we talked about, having essentially a second api that's more geared toward the engine, might be appropriate. What if we do the following:

  • add a new function, execute_pick_up_tip(mount: ...) -> None
  • this function's job is to command the motors to do the tip-pick-up sequence
    • also maybe check whether the tip is attached via the sensors; or we can keep the current second call
  • and nothing else. it doesn't set any kind of internal tip-presence state
  • the engine implementation for pick up tip, and no other caller, is changed so it first calls this function; checks if it succeeded (i.e either with a try/catch, or by subsequently calling verify_tip_presence(); calls add_tip() with the tip details; and calls prepare_for_aspirate()

At that point we don't have to worry about any other callers, we don't have to worry about knock-on effects, and we've at least taken a step toward this second api concept (the next step would be to start specifying tip lengths in move_to, and remove the add_tip call).

status = await self.get_tip_presence_status(
mount, InstrumentProbeType.PRIMARY
)
if status == TipStateType.PRESENT:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this, honestly. If this check fails, then we never add_tip. Then, if this was called with prep_after=True, we'll try to prepare_for_aspirate(); that will then raise an exception if a tip isn't present. So now we raise an exception in a way that we didn't before. And prep_after is default True, so lots of things will call it with that set.

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.

None yet

3 participants