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(hardware_control): Fixing liquid probe bugs found in end to end testing #15619

Merged
merged 22 commits into from
Jul 16, 2024

Conversation

ryanthecoder
Copy link
Contributor

@ryanthecoder ryanthecoder commented Jul 10, 2024

Overview

These changes came from testing the end to end implementation of liquid level detection on the ot3 and fixing the bugs that came up.

Test Plan

Changed the unit tests to match new functionality.
Tested all new methods for liquid level detection on the ot3.

Changelog

Changes include:

  • Deleting unnecessary checks in ot3controller.py
  • Editing ot3api.py to issue the correct probe movements
  • Standardizing the attribute names for liquid_presence_detection
  • Conditionally adding a require_liquid_presence command before aspirates
  • Redoing movement in liquid_probe so that it actually takes a sensible path
  • Editing tests accordingly

Review requests

We are aware of a bug that even though we handle LLD exceptions at the protocol_context level, protocol_engine commands will still be marked as failed, so all future commands will fail. As far as I know for this PR, this only affects detect_liquid_presence on a well with no liquid. Another PR will be coming soon that addresses this.

Risk assessment

Medium? I'm not sure.

@ryanthecoder ryanthecoder requested a review from a team as a code owner July 10, 2024 21:34
Copy link
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/

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.

Some style and consistency stuff to fix up. Also, let's get a nicer description in here.

api/lldfunctionexamples.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/core/engine/instrument.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_api/core/engine/instrument.py Outdated Show resolved Hide resolved
@aaron-kulkarni aaron-kulkarni marked this pull request as draft July 11, 2024 17:48
@Opentrons Opentrons deleted a comment from github-actions bot Jul 15, 2024
@Opentrons Opentrons deleted a comment from github-actions bot Jul 15, 2024
@Opentrons Opentrons deleted a comment from github-actions bot Jul 15, 2024
@Opentrons Opentrons deleted a comment from github-actions bot Jul 15, 2024
@Opentrons Opentrons deleted a comment from github-actions bot Jul 15, 2024
…capture (#15629)

This PR is an automated snapshot update request. Please review the
changes and merge if they are acceptable or find your bug and fix it.

Co-authored-by: aaron-kulkarni <[email protected]>
@aaron-kulkarni aaron-kulkarni marked this pull request as ready for review July 15, 2024 17:18
@aaron-kulkarni aaron-kulkarni requested a review from a team as a code owner July 15, 2024 17:18
@Opentrons Opentrons deleted a comment from github-actions bot Jul 15, 2024
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.

This all looks good to me assuming others' pending comments have been resolved. Some small final notes:

Comment on lines -102 to -107
current_well = CurrentWell(
pipette_id=pipette_id,
labware_id=labware_id,
well_name=well_name,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yep, that'd do it. Nice catch.

api/src/opentrons/protocol_engine/execution/pipetting.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_engine/execution/pipetting.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.12%. Comparing base (3479875) to head (833b607).
Report is 49 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #15619   +/-   ##
=======================================
  Coverage   60.12%   60.12%           
=======================================
  Files         190      190           
  Lines       10627    10627           
=======================================
  Hits         6390     6390           
  Misses       4237     4237           
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)

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

@aaron-kulkarni aaron-kulkarni merged commit 29ebcba into edge Jul 16, 2024
23 checks passed
@aaron-kulkarni aaron-kulkarni deleted the fixing-liquid-probe branch July 16, 2024 19:08
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.

4 participants