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(protocol-engine): absorbance plate reader engine load behavior and lid movement command #15599

Merged
merged 28 commits into from
Aug 9, 2024

Conversation

ahiuchingau
Copy link
Contributor

@ahiuchingau ahiuchingau commented Jul 8, 2024

Overview

Covers PLAT-447 PLAT-381 PLAT-210 PLAT-209 PLAT-204 PLAT-205 PLAT-206 PLAT-215 PLAT-193 PLAT-194 PLAT-397 PLAT-398 PLAT-406 PLAT-407
This PR introduces the fundamental load module and labware presence behavior for the plate reader and it's associated lid.
This PR implements the open_lid and close_lid commands for the plate reader.

Changelog

  • add plate reader lid definition as a labware
  • add lid dock addressable areas to the deck definition
  • add plate reader lid definition as legacyFixture to be loaded as deck fixed labware
  • update deck fixed labware logic to account for deck configuration, addressable areas, etc
  • load the module lid labware when the protocol engine is created so long as a plate reader exists in the deck configuration
  • load the module lid in for analysis whenever a load module command occurs for the plate reader
  • add protocol engine action AddAbsorbanceReaderLidAction to pass the lid's labware id to the absorbance plate reader substate in the case of analysis and virtualized modules
  • add open_lid and close_lid protocol engine command

Tests

  • Ensure plate reader lid is loaded into protocol engine on start, and references to opening and closing pass analysis
  • Ensure that open and close lid remove and replace the lid of the plate reader as expected during a run
  • Ensure the Deck Configuration provides the addressable areas needed for referencing the two primary locations on the plate reader

],
"parameters": {
"format": "irregular",
"quirks": [],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

add a new quirk: unloadable_by_user
Add engine logic to check the quirk

Copy link
Contributor

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

@CaseyBatten
Copy link
Contributor

Some notes on behavior changes in this latest commit:

  • I have added the Plate Reader Lid to the legacyFixtures of the ot3_standard.json v5 file with their "slots" aligning to the addressable area of the Reader they are physically associated with
  • DeckFixedLabware has had it's location parameter update to accept any labware location, not just deck slot locations
  • When create_protocol_engine handles deck fixed labware, it now can account for these kinds of "addressable area" based labware items, and will only load them if the deck configuration provides a fixture that matches
  • The Modules state store is now passed deck fixed labware as well
  • When a plate reader module is loaded, and the state store initializes it's substate, the plate reader will now assign it's lid labware id based on which lids are available in the deck fixed labware list that match the modules expected mounting position
  • When in analysis, the Protocol Core API will manually load a plate reader lid labware onto the newly loaded plate reader module, allowing us to perform with the same behavior as a run even with the absence of the deck configuration
  • Module behavior for the Plate Readers now exclusively utilizes the plate reader addressable areas, as opposed to slots D3 and D4
  • Open and Close lid functions also operate exclusively on addressable areas

After exploring a series of implementation methods to get us where we want to be with this, this series of changes made the most sense. Let us know your thoughts on this and if any apparent problems would arise from these refactors.

@CaseyBatten CaseyBatten marked this pull request as ready for review July 24, 2024 20:50
@CaseyBatten CaseyBatten requested review from a team as code owners July 24, 2024 20:50
@CaseyBatten CaseyBatten requested review from mjhuff and removed request for a team July 24, 2024 20:50
@CaseyBatten CaseyBatten changed the title feat(protocol-engine): absorbance plate reader lid movement command feat(protocol-engine): absorbance plate reader engine load behavior and lid movement command Jul 24, 2024
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.31%. Comparing base (61d23fc) to head (9c26a9a).
Report is 2 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #15599   +/-   ##
=======================================
  Coverage   63.31%   63.31%           
=======================================
  Files         300      300           
  Lines       15773    15773           
=======================================
  Hits         9986     9986           
  Misses       5787     5787           
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)
shared-data 73.40% <ø> (ø)

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

Copy link
Contributor

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

this was set to 2 21 for testing but should remain at 2 20
@ecormany
Copy link
Contributor

Took a quick look at docstrings and they're good for now! We can make any edits this fall when we write a full documentation page for the reader (RTC-488).

Comment on lines 33 to 42
pickUpOffset: Optional[LabwareOffsetVector] = Field(
None,
description="Offset to use when picking up labware. "
"Experimental param, subject to change",
)
dropOffset: Optional[LabwareOffsetVector] = Field(
None,
description="Offset to use when dropping off labware. "
"Experimental param, subject to change",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@CaseyBatten
Do we still need this?, since we know the exact location of the lid per the definition?

Copy link
Contributor

Choose a reason for hiding this comment

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

We utilized this previously for D3/D4 movement estimations (third column in general) before the addressable area hookups were in place, can definitely be removed now.

Comment on lines 104 to 108
# TODO (AA): we probably don't need this, but let's keep it until we're sure
user_offset_data = LabwareMovementOffsetData(
pickUpOffset=params.pickUpOffset or LabwareOffsetVector(x=0, y=0, z=0),
dropOffset=params.dropOffset or LabwareOffsetVector(x=0, y=0, z=0),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we still need this for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same reasoning as stated above, can be removed now.

Comment on lines 90 to 94
# TODO (AA): we probably don't need this, but let's keep it until we're sure
user_offset_data = LabwareMovementOffsetData(
pickUpOffset=params.pickUpOffset or LabwareOffsetVector(x=0, y=0, z=0),
dropOffset=params.dropOffset or LabwareOffsetVector(x=0, y=0, z=0),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

same, not sure if needed anymore?

@@ -63,32 +61,32 @@ async def execute(
if abs_reader is not None:
result = await abs_reader.start_measure(wavelength=params.sampleWavelength)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Are we guaranteed to be initialized by the time we get here? or do we need to raise some error otherwise?
  • Do we want to check the plate presence here before beginning a read? since we cant read if there's no plate.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be covered in a separate PR related to the Read behavior live data hookups, engine validations, etc.

@vegano1
Copy link
Contributor

vegano1 commented Aug 6, 2024

@CaseyBatten

This is great, thank you for getting this over the edge!

Left a few comments on

  • removing pickUpOffset and dropOffset from open/close lid protocol engine commands
  • Additional validation is needed before measuring like initializing + checking plate presence.

@vegano1
Copy link
Contributor

vegano1 commented Aug 8, 2024

@CaseyBatten
The changes look good,
let's remove the no longer used pickUpOffset/dropOffset + fix merge conflicts and get this sucker merged :)

@vegano1
Copy link
Contributor

vegano1 commented Aug 9, 2024

@CaseyBatten

This looks good, thank you for making the changes and fixing the merge conflicts.

  • The front-end tests failing seems unrelated to the changes made
  • We will need to update the API command-schema snapshot as the test_command_schema_has_not_changed test is failing

Copy link
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

These fundamental engine behaviors as well as the framework for Plate Reader interaction allow the use of the plate reader in a protocol and ensure the handling of the lid as an object with trackable state in the context of engine behavior.

Comment on lines +102 to +107
labware.append(
DeckFixedLabware(
labware_id=labware_id,
definition=definition,
location=addressable_area_location,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of the lids to the fixed labware loading mechanism is how we manage to ensure a lid exists within the engine context before an explicit load module command has occurred, utilizing the deck configuration.

Comment on lines +835 to +840
{
"id": "absorbanceReaderV1LidA3",
"slot": "absorbanceReaderV1A3",
"labware": "opentrons_flex_lid_absorbance_plate_reader_module",
"displayName": "Plate Reader Lid"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

The addition of this (and all similar plate reader lids) ensures a lid "fixture" labware is tagged via the addressable areas loaded onto the deck. This mechanism gives us the tools to inject labware into the engine context without explicit load commands given only the provided configuration of the deck itself.

Comment on lines +70 to +72
new_location = self._state_view.modules.absorbance_reader_dock_location(
mod_substate.module_id
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure we handle the Lid dock as an addressable area, resolved via the module which created it.

Comment on lines +78 to +85
addressableAreaName=self._state_view.modules.ensure_and_convert_module_fixture_location(
deck_slot=self._state_view.modules.get_location(
params.moduleId
).slotName,
deck_type=self._state_view.config.deck_type,
model=absorbance_model,
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure we handle the module reader slot as an addressable area, resolved through the module and location in which the module was loaded.

@CaseyBatten CaseyBatten merged commit 58a7d19 into edge Aug 9, 2024
59 checks passed
@vegano1 vegano1 deleted the abs96_move-lid-command branch August 9, 2024 17:43
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