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(shared-data,-protocol-designer): add foundation for plate reader support in PD #17147

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Dec 19, 2024

Overview

This PR introduces plate reader to PD, still hidden behind a feature flag. Namely, it adds the ability to add plate reader to the initial deck state and fixes logic for ModuleLabel rendering for all modules. Of note, in this dev work, I discovered a bug in the plate reader's shared-data definition, in which the top-level x and y dimensions were flipped, so those values are swapped here as well.

Closes AUTH-1077

Test Plan and Hands on Testing

  • create or import a Flex protocol (example)
  • select slot B1 to edit hardware/labware
  • verify correct modules populate, and hovering each module option produces the correctly positioned ModuleLabel on the deck
  • repeat with center and right column slots, verifying that plate reader option displays properly on the right column slot
  • with modules added, navigate to protocol timeline and click various module steps
  • verify that ModuleLabel is correctly positioned
Screen.Recording.2024-12-18.at.8.00.30.PM.mov

Changelog

  • add logic for handling plate reader addition to deck and adding compatible labware
  • add logic for correct positioning of ModuleLabel for all modules in any slot position. An implication here is passing a slot prop to determine slot-specific corner offset from slot from a module's definition
  • improve time complexity of DeckSetup util getModuleModelsBySlot
  • fix plate reader x and y dimensions (spoke with plate reader devs to ensure this was correct)
  • fix tests

Review requests

see test plan

Risk assessment

low. behind feature flag and major changes only affect UI for module labels

… support in PD

This PR introduces plate reader to PD, still hidden behind a feature flag. Namely, it adds the ability to add plate reader to the initial deck state and fixes logic for ModuleLabel rendering for all modules. Of note, in this dev work, I discovered a bug in the plate reader's shared-data definition, in which the top-level x and y dimensions were flipped, so those values are swapped here as well.
@ncdiehl11 ncdiehl11 self-assigned this Dec 19, 2024
@ncdiehl11 ncdiehl11 requested review from koji and jerader December 19, 2024 01:02
@ncdiehl11 ncdiehl11 marked this pull request as ready for review December 19, 2024 01:02
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner December 19, 2024 01:02
@ncdiehl11 ncdiehl11 removed the request for review from a team December 19, 2024 01:02
Comment on lines +14 to +15
"xDimension": 155.3,
"yDimension": 95.5,
Copy link
Collaborator

Choose a reason for hiding this comment

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

wow nice catch! there is plate reader support in the app in 8.2. it'd probably be good to post that you found this bug and swapped the values in the protocol execution slack channel so sw folks who worked on plate reader support in app/api are aware

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I did run this by exec/plat and they concur that it was in fact a bug. They're aware of the value swap in this PR

@@ -94,6 +94,7 @@ export const RECOMMENDED_LABWARE_BY_MODULE: { [K in ModuleType]: string[] } = {
],
[ABSORBANCE_READER_TYPE]: [
'opentrons_flex_lid_absorbance_plate_reader_module',
Copy link
Collaborator

Choose a reason for hiding this comment

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

this plate isn't showing up, it might be included in the PD_DO_NOT_LIST or something

Screenshot 2024-12-19 at 08 19 07

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I know, we don't permit explicit loading of the lid, because we don't treat it like a normal labware. Rather than use move labware commands, the api expects open/close lid commands, which will move the implicitly loaded lid "labware" object.

Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

looks amazing, great work! A few questions and comments:

  1. we aren't supporting MoaM for this launch?
  2. there's that one plate that isn't showing up: opentrons_flex_lid_absorbance_plate_reader_module - is that expected?
  3. great work finding that bug in the def!
  4. the module tags look a lot better now. feel free to make a follow up ticket and put it in the backlog for us to further fix them in the future. but i think what you have now is sufficient

@ncdiehl11
Copy link
Collaborator Author

ncdiehl11 commented Dec 19, 2024

looks amazing, great work! A few questions and comments:

  1. we aren't supporting MoaM for this launch?
  2. there's that one plate that isn't showing up: opentrons_flex_lid_absorbance_plate_reader_module - is that expected?
  3. great work finding that bug in the def!
  4. the module tags look a lot better now. feel free to make a follow up ticket and put it in the backlog for us to further fix them in the future. but i think what you have now is sufficient

@jerader thanks for the review:

  1. We totally are and I forgot to remove that check. I will do that now
  2. I just spoke with exec— that lid def is never used by the public API. It is only used internally to know the dimensions of picking up the lid for the gripper. In fact, we actually should disallow loading labware initially onto the plate reader, since it will have to be removed before initializing/reading.
  3. thanks!
  4. thanks, I will leave for now

@jerader
Copy link
Collaborator

jerader commented Dec 19, 2024

  1. there's that one plate that isn't showing up: opentrons_flex_lid_absorbance_plate_reader_module - is that expected?
  1. I just spoke with exec— that lid def is never used by the public API. It is only used internally to know the dimensions of picking up the lid for the gripper. In fact, we actually should disallow loading labware initially onto the plate reader, since it will have to be removed before initializing/reading.

@ncdiehl11 , cool, lets remove it from the array then? because right now it is included in the list of recommended labware

@ncdiehl11 ncdiehl11 requested a review from a team as a code owner December 19, 2024 16:04
@ncdiehl11 ncdiehl11 merged commit 481a1f0 into edge Dec 19, 2024
57 checks passed
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.

2 participants