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): update moduleOffsets when loading/calibrating modules to get accurate labware position. #12484

Merged
merged 6 commits into from
Apr 14, 2023

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Apr 13, 2023

Overview

We need to include the calibrated module offsets when calculating the labware position, this way that position is accurate for operations required by the system. This pr will hook in the calibrated data and load it from the disk if it exists, as well as update the existing calibration data when we calibrate a module.

Cloeses: RCORE-666

Test Plan

  • Make sure we can still run the LoadModule protocol engine command
  • Run the LoadModule PE command and make sure we are loading module offsets from disk and returning them in LoadModuleResult
  • Run CalibrateModule PE command to get new calibration values, then run LoadModule PE command and make sure the vectorOffset value in LoadModuleResult returns the new calibrated value.

Changelog

  • Load module calibration when creating protocol engine StateStore
  • Handle CalibrateModule to update the module calibration value
  • Add calibrated offset when calculating the module offsets in the modules.states.get_module_offset function

Review requests

  • Is this the right approach?
  • Am I missing something?

Risk assessment

  • Low, flex work

Todo

  • fix unit tests

@vegano1 vegano1 requested a review from sanni-t April 13, 2023 01:32
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #12484 (9aef193) into edge (44daef5) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12484      +/-   ##
==========================================
- Coverage   73.78%   73.78%   -0.01%     
==========================================
  Files        2247     2247              
  Lines       61774    61771       -3     
  Branches     6452     6452              
==========================================
- Hits        45580    45577       -3     
  Misses      14674    14674              
  Partials     1520     1520              
Flag Coverage Δ
notify-server 89.13% <ø> (ø)

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

Impacted Files Coverage Δ
...pentrons/protocol_engine/create_protocol_engine.py 100.00% <ø> (ø)
.../protocol_engine/resources/module_data_provider.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/state.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/state/modules.py 90.81% <100.00%> (ø)

... and 1 file with indirect coverage changes

@vegano1 vegano1 requested a review from sfoster1 April 13, 2023 14:06
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

I think this way of including the offsets in loadLabwareResult and caching offsets after a calibrateModule command is quite a roundabout way of adding offsets. Also, unless I'm mistaken as to how module_id is used, it looks like you'll never be able to load a module offset from disk because the module_id generated by the engine is completely random and changes every run.
But before that, I have a basic question-
Do we intend to calibrate a module before each protocol run, like LPC, or is the plan to calibrate the module once, like an instrument calibration, save that info to disk, and then use those calibrated offsets each time that module is used in a protocol? Also, do we want to save unique offsets for a module+slot combo or do we want to use the same calibrated offset of a module for all slot locations?

api/src/opentrons/protocol_engine/state/modules.py Outdated Show resolved Hide resolved
api/src/opentrons/protocol_engine/state/modules.py Outdated Show resolved Hide resolved
@vegano1 vegano1 requested a review from sanni-t April 14, 2023 16:18
add state loading when creating module store
@vegano1 vegano1 marked this pull request as ready for review April 14, 2023 19:52
@vegano1 vegano1 requested a review from a team as a code owner April 14, 2023 19:52
vegano1 added 2 commits April 14, 2023 16:02
remove unused f string
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Loading and applying offsets in engine looks good!

@vegano1 vegano1 merged commit 85d9161 into edge Apr 14, 2023
@vegano1 vegano1 deleted the RCORE-666_implement_adding_module_offsets branch April 14, 2023 20:30
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