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,hardware): add hepa/uv config/control commands to api. #14489

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

vegano1
Copy link
Contributor

@vegano1 vegano1 commented Feb 13, 2024

Overview

We added can messages to control the hepa/uv in this pr (Opentrons/opentrons:#14452), now we want to use those messages from the hardware controller and API. This pr adds the hooks to these can messages so they can be used from repl.

Closes: RET-1424

Test Plan

  • Make sure we can control the hepa/uv module using ot3api from repl
  • Make sure we can query the hepa/uv module using ot3api from repl

Changelog

  • Added hooks to OT3API and OT3Controller to enable control/query of Hepa/UV state.

Review requests

Risk assessment

low, unreleased

Todo

  • Add unit tests

@vegano1 vegano1 requested a review from a team as a code owner February 13, 2024 22:56
@vegano1 vegano1 requested a review from sfoster1 February 13, 2024 22:56
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

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

Comparison is base (c142da1) 67.79% compared to head (7109242) 67.82%.
Report is 33 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14489      +/-   ##
==========================================
+ Coverage   67.79%   67.82%   +0.02%     
==========================================
  Files        2518     2519       +1     
  Lines       72017    72082      +65     
  Branches     9244     9244              
==========================================
+ Hits        48826    48887      +61     
- Misses      20989    20993       +4     
  Partials     2202     2202              
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)
hardware 57.81% <94.11%> (+0.27%) ⬆️

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

Files Coverage Δ
...entrons/hardware_control/backends/ot3controller.py 68.48% <ø> (ø)
...pentrons/hardware_control/backends/ot3simulator.py 86.23% <ø> (ø)
api/src/opentrons/hardware_control/ot3api.py 79.87% <ø> (ø)
api/src/opentrons/hardware_control/types.py 96.55% <ø> (ø)
...ns_hardware/firmware_bindings/messages/payloads.py 95.88% <100.00%> (ø)
...rons_hardware/hardware_control/hepa_uv_settings.py 93.84% <93.84%> (ø)

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.

Functionality looks good but let's get some tests on the hepa_uv_settings.py

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.

Also we need some stubs in ot3simulator

return await set_hepa_uv_state_fw(self._messenger, light_on, timeout_s)

async def get_hepa_uv_state(self) -> Optional[HepaUVState]:
res = await get_hepa_uv_state_fw(self._messenger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you not just returning res here because of linter complaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its to separate opentrons_hardware dataclasses from opentrons.hardware_control ones.

return await self._backend.get_hepa_fan_state()

async def set_hepa_uv_state(
self, turn_on: bool = False, timeout_s: int = 900
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this long of a timeout?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah 900s (15m) is the default uv light timeout

Copy link
Member

Choose a reason for hiding this comment

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

can we call it like dose_duration or something?

@vegano1 vegano1 requested a review from sfoster1 February 15, 2024 21:39
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 to me, though I agree with Peter that we maybe need a better name for timeout - it looks at first glance like it's a time-before-error parameter like other timeouts.

return await self._backend.get_hepa_fan_state()

async def set_hepa_uv_state(
self, turn_on: bool = False, timeout_s: int = 900
Copy link
Member

Choose a reason for hiding this comment

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

can we call it like dose_duration or something?

@vegano1 vegano1 merged commit cb0d4e6 into edge Feb 16, 2024
23 checks passed
@vegano1 vegano1 deleted the RET-1424_add-hepa-uv-hw-controller branch February 16, 2024 17:37
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