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

refactor(api): only restore changed currents #13683

Merged
merged 3 commits into from
Oct 3, 2023

Conversation

ahiuchingau
Copy link
Contributor

Overview

This PR updates the ot3controller.restore_current function so it can optionally take in run currents and hold currents. Instead of unconditionally restoring all current settings for every present axis, now this function can intelligently restore only the ones that were changed.

@ahiuchingau ahiuchingau requested a review from a team as a code owner September 28, 2023 22:20
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #13683 (9f5a540) into edge (b21a501) will not change coverage.
Report is 32 commits behind head on edge.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #13683   +/-   ##
=======================================
  Coverage   71.10%   71.10%           
=======================================
  Files        2438     2438           
  Lines       68526    68526           
  Branches     8090     8090           
=======================================
  Hits        48727    48727           
  Misses      17905    17905           
  Partials     1894     1894           
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
hardware-testing ∅ <ø> (∅)
notify-server 89.13% <ø> (ø)

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

Files Coverage Δ
...entrons/hardware_control/backends/ot3controller.py 68.42% <ø> (ø)
...pentrons/hardware_control/backends/ot3simulator.py 86.44% <ø> (ø)
api/src/opentrons/hardware_control/ot3api.py 79.53% <ø> (ø)

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.

Looks good with a nit

async def restore_current(self) -> AsyncIterator[None]:
"""Save the current."""
old_current_settings = deepcopy(self._current_settings)
async def restore_current(
Copy link
Member

Choose a reason for hiding this comment

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

kind of a nit i guess but really this isn't restore_current anymore, can we call it like motor_current or something

Copy link
Contributor

@fsinapi fsinapi left a comment

Choose a reason for hiding this comment

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

I really like this change!!! Some of the hardware-testing scripts just need to be updated to use the new function correctly.

@@ -252,7 +252,7 @@ async def _force_gauge(
force_output = []
TH.start()
try:
async with api._backend.restore_current():
async with api._backend.motor_current():
Copy link
Contributor

Choose a reason for hiding this comment

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

The set_active_current call below should be merged into here

@@ -152,7 +152,7 @@ async def _drop_tip(api: OT3API, trash: Point) -> None:
async def _partial_pick_up_z_motion(
api: OT3API, current: float, distance: float, speed: float
) -> None:
async with api._backend.restore_current():
async with api._backend.motor_current():
Copy link
Contributor

Choose a reason for hiding this comment

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

The set_active_current call below should be merged into here

@@ -613,7 +613,7 @@ async def move_tip_motor_relative_ot3(
if motor_current is None:
await _move_coro
else:
async with api._backend.restore_current():
async with api._backend.motor_current():
Copy link
Contributor

Choose a reason for hiding this comment

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

The set_active_current call below should be merged into here

@@ -581,7 +581,7 @@ async def move_plunger_absolute_ot3(
if motor_current is None:
await _move_coro
else:
async with api._backend.restore_current():
async with api._backend.motor_current():
Copy link
Contributor

Choose a reason for hiding this comment

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

The set_active_current call below should be merged into here

Copy link
Contributor

@fsinapi fsinapi left a comment

Choose a reason for hiding this comment

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

nice

@ahiuchingau ahiuchingau merged commit 889d25f into edge Oct 3, 2023
21 of 25 checks passed
@ahiuchingau ahiuchingau deleted the api_restore-current-func branch October 3, 2023 21:32
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