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(shared-data): load pick up current properly #14350

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Jan 24, 2024

While we properly handled most kinds of pipette config, there's one we didn't: pick up current. It gets special treatment now because we can properly set different currents for different numbers of tips, rather than being a value. So, we need to mark it as a setting that needs to be iterated over a dictionary in its leaf value rather than directly set.

This also fixes an issue that caused the OT-2 to not boot when loading a pipette configuration that had a pick up current customization, since this code is called when loading a pipette.

It may be closing the barn after the horse escapes, but to prevent similar issues preventing boot, I also tossed in a catch all except that logs a loud message but won't prevent starting the server.

Testing

  • With this PR, modify a pipette's pick up current and check that the system reboots properly and the values are preserved across reboots
  • Downgrade an OT-2 to 6.3.0, modify a pipette's pick-up current, and then update to something with this PR
    • note that this isn't really necessary because 6.3.0, 7.0.1, 7.1.0, and edge all produce exactly the same config override file

Closes RESC-187

While we properly handled _most_ kinds of pipette config, there's one we
didn't: pick up current. It gets special treatment now because we can
properly set different currents for different numbers of tips, rather
than being a value. So, we need to mark it as a setting that needs to be
iterated over a dictionary in its leaf value rather than directly set.

This also fixes an issue that caused the OT-2 to not boot when loading a
pipette configuration that had a pick up current customization, since
this code is called when loading a pipette.

It may be closing the barn after the horse escapes, but to prevent
similar issues preventing boot, I also tossed in a catch all except that
logs a loud message but won't prevent starting the server.

Closes RESC-187
@sfoster1 sfoster1 requested a review from a team January 24, 2024 21:02
@sfoster1 sfoster1 requested a review from a team as a code owner January 24, 2024 21:02
Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (46195f5) 68.05% compared to head (7bfcdac) 68.08%.
Report is 3 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14350      +/-   ##
==========================================
+ Coverage   68.05%   68.08%   +0.03%     
==========================================
  Files        2508     2508              
  Lines       71594    71645      +51     
  Branches     9106     9126      +20     
==========================================
+ Hits        48721    48780      +59     
- Misses      20757    20761       +4     
+ Partials     2116     2104      -12     
Flag Coverage Δ
app 65.62% <ø> (+0.02%) ⬆️
protocol-designer 34.88% <ø> (ø)
shared-data 75.25% <75.00%> (+0.05%) ⬆️
step-generation 86.90% <ø> (ø)

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

Files Coverage Δ
...n/opentrons_shared_data/pipette/model_constants.py 100.00% <ø> (ø)
...rons_shared_data/pipette/mutable_configurations.py 87.09% <75.00%> (+0.83%) ⬆️

... and 7 files with indirect coverage changes

Copy link
Contributor

@ahiuchingau ahiuchingau left a comment

Choose a reason for hiding this comment

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

Makes sense!

@sfoster1 sfoster1 merged commit ab4e749 into edge Jan 25, 2024
51 of 52 checks passed
@sfoster1 sfoster1 deleted the resc-187-fix-pipette-config branch January 25, 2024 18:51
ncdiehl11 pushed a commit that referenced this pull request Feb 1, 2024
While we properly handled _most_ kinds of pipette config, there's one we
didn't: pick up current. It gets special treatment now because we can
properly set different currents for different numbers of tips, rather
than being a value. So, we need to mark it as a setting that needs to be
iterated over a dictionary in its leaf value rather than directly set.

This also fixes an issue that caused the OT-2 to not boot when loading a
pipette configuration that had a pick up current customization, since
this code is called when loading a pipette.

It may be closing the barn after the horse escapes, but to prevent
similar issues preventing boot, I also tossed in a catch all except that
logs a loud message but won't prevent starting the server.

Closes RESC-187
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.

3 participants