feat(api): Heater Shaker Firmware Level Emulator #11121

DerekMaggio · 2022-07-13T17:25:39Z

Overview

Add firmware level emulator and tests for Heater Shaker

Changelog

Create HeaterShakerEmulator class
Create RPM simulation
Add Heater/Shaker settings
Add Heater/Shaker to monorepo docker compose file

Review requests

@sanni-t & @pmoegenburg can you do a deep dive into whether or not my emulation accurately reproduces the functionality of the hardware Heater/Shaker
This is my first PR since being gone on paternity leave, I probably made some stupid errors and style mistakes

Risk assessment

Low

remove files Fix some parsing errors Formatting Change rpm_per_tick

codecov · 2022-07-13T17:34:35Z

Codecov Report

Merging #11121 (cfe7d93) into heater-shaker-firmware-emulator-wip (b4a11af) will not change coverage.
The diff coverage is 100.00%.

@@                         Coverage Diff                          @@
##           heater-shaker-firmware-emulator-wip   #11121   +/-   ##
====================================================================
  Coverage                                73.76%   73.76%           
====================================================================
  Files                                     2077     2077           
  Lines                                    57372    57372           
  Branches                                  5741     5741           
====================================================================
  Hits                                     42318    42318           
  Misses                                   13816    13816           
  Partials                                  1238     1238

Flag	Coverage Δ
notify-server	`89.17% <ø> (ø)`

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

Impacted Files	Coverage Δ
api/src/opentrons/drivers/utils.py	`93.54% <ø> (ø)`
...e_control/emulation/scripts/run_module_emulator.py	`75.00% <100.00%> (ø)`
...c/opentrons/hardware_control/emulation/settings.py	`100.00% <100.00%> (ø)`
...pentrons/hardware_control/emulation/simulations.py	`97.87% <100.00%> (ø)`

DerekMaggio · 2022-07-14T14:35:10Z

I confirmed that what I run the docker-compose.yaml file in the monorepo everything starts correctly and I am able to hit the /modules endpoint successfully.

sanni-t

This is great! Tested the docker-built emulator with dev server and it works! (simply running emulator app with make -C api emulator results in an unrelated error but I think it's been like that for a while now)

Two things I noticed-

Sending a deactivate_shaker makes the heater-shaker in emulator unresponsive for a few seconds. It eventually responds with shake deactivated though so was wondering if that's an intentional silence.
deactivate_heater is not yet implemented. Left out intentionally?

Thanks so much for implementing this so quickly!

Formatting and linting

DerekMaggio · 2022-07-15T14:57:35Z

So after much fruitless investigation, I have no idea why the emulator does not handle the deactivate_shaker command from the Heater/Shaker Hardware Controller.

Had a meeting with Robot Services and they are going to take over the task of getting the command to work.

DerekMaggio · 2022-07-15T14:59:40Z

@sanni-t please note that I have changed the base from edge to heater-shaker-firmware-emulator-wip.
I don't want to merge this to edge until it is completely working

api/src/opentrons/hardware_control/emulation/heater_shaker.py

api/tests/opentrons/hardware_control/integration/test_heatershaker.py

DerekMaggio · 2022-07-19T19:33:11Z

@sanni-t can I get a re-review?

SyntaxColoring · 2022-07-19T22:59:24Z

api/tests/opentrons/hardware_control/integration/test_heatershaker.py

+    await heatershaker.wait_next_poll()
+    await heatershaker.start_set_temperature(50.0)
+
+    # Have to wait for next poll because target temp will not update until then


Is this the intended API contract of the Heater-Shaker hardware controller? If I do hs.start_set_temperature(...) and then do t = hs.target_temperature, I'm not guaranteed to get back what I just set?

If yes: I think this is surprising, and we need to document it in the HeaterShaker class. Doesn't have to happen in this PR, but I'd like confirmation/consensus.

If no: this indicates a bug, I guess?

You know, it makes sense that target temp should be set and it would be just current temp that hasn't update yet.

Looking into it now.

I left out a line. Fixing it in a followup commit

sanni-t · 2022-07-20T14:48:38Z

The behavior is almost there!
Two things that need to be cleaned up:

On the real hardware, a home command (G28) will only respond back once homing is finished. If we try to do that with the ramp-down behavior of deactivate then it'll take quite a long time, unlike the real hardware that goes from shaking to homed in just 2-3 seconds. So I think it'll be okay to not use the speed ramp down for homing. Instead we can add a delay of 2-3 seconds, set the target and current speeds to zero, and respond with G28 OK.
Ramping down of current temperature on deactivating heater (you probably already know). This one will respond to the gcode immediately after deactivation and then continue ramping down to an ambient temperature.

Based off of #11121 (comment)

DerekMaggio · 2022-07-20T18:20:52Z

@sanni-t I addressed your comments here

Is it okay to use a regular sleep in the _home function of the emulator?

sanni-t · 2022-07-20T18:40:25Z

@sanni-t I addressed your comments here

🙌

Is it okay to use a regular sleep in the _home function of the emulator?

That is a good point. It won't matter for manual testing but it would for the unit tests & automated tests. What do we do for unit tests that need to wait for, say, speed to ramp up? Are we changing the duration of a tick to something very small? In that case, maybe we could modify the tick function to ramp down with rpm_per_tick of something large, like 500.. only when target is 0.. Just a thought

sanni-t · 2022-07-20T18:55:23Z

api/src/opentrons/hardware_control/emulation/heater_shaker.py

+        return f"M241 STATUS:{self._latch_status.value.upper()}"
+
+    def _deactivate_heater(self, command: Command) -> str:
+        self._temperature.set_target(TEMPERATURE_ROOM)


This makes the temperature status respond with a target of room temperature value. We want the target to return as None. It looks like the thermocycler & tempdeck's deactivate emulator simply sets the target to None and current temp to room temperature. So, I'm fine with implementing the same thing here for now. We can revisit them all later.

sanni-t · 2022-07-20T18:58:55Z

That is a good point. It won't matter for manual testing but it would for the unit tests & automated tests. What do we do for unit tests that need to wait for, say, speed to ramp up? Are we changing the duration of a tick to something very small? In that case, maybe we could modify the tick function to ramp down with rpm_per_tick of something large, like 500.. only when target is 0.. Just a thought

Or if that isn't easy to implement, just set the target & current rpm to 0 and send the gcode response. And leave a TODO note to improve this behavior to mimic the hardware's true behavior. We can address all improvements in a follow-up ticket when we're not pressed for time.

sfoster1 · 2022-07-21T14:04:01Z

That is a good point. It won't matter for manual testing but it would for the unit tests & automated tests. What do we do for unit tests that need to wait for, say, speed to ramp up? Are we changing the duration of a tick to something very small? In that case, maybe we could modify the tick function to ramp down with rpm_per_tick of something large, like 500.. only when target is 0.. Just a thought

Or if that isn't easy to implement, just set the target & current rpm to 0 and send the gcode response. And leave a TODO note to improve this behavior to mimic the hardware's true behavior. We can address all improvements in a follow-up ticket when we're not pressed for time.

Let's do the latter. We really need to resist the urge to implement too much behavior here. This is a testing aid and nothing more, and even then because this interacts with a physical system and we're not simulating the world, there's always going to be little places where the behavior doesn't match. We should definitely make the exact format of communications on the wire the same as firmware, like changing an unset target to None; we should definitely not stray into little bits of physical modeling.

DerekMaggio · 2022-07-21T14:38:16Z

@sanni-t @sfoster1
I made it so when you call deactivate_heater it now sets the current temp to TEMPERATURE_ROOM and sets target to None.
I also made the home delay time customizable by adding a home_delay_time setting to the HeaterShakerSettings object

sanni-t

This works as expected! Thank you!

* feat: Heater-Shaker Firmware Emulator remove files Fix some parsing errors Formatting Change rpm_per_tick * Tests * Modify home functionality Formatting and linting * fix rpm emulator g-code and add deactivate test * Fix tests * Add deactivate_heater G-Code * Add start_set_temperature * Update home and deactivate_heater functionality Based off of #11121 (comment) * Linting * fix: Fix deactivate_heater function * Make home_delay_time customizable * Linting Co-authored-by: jbleon95 <[email protected]> Co-authored-by: jbleon95 <[email protected]>

Derek Maggio added 2 commits July 13, 2022 10:21

feat: Heater-Shaker Firmware Emulator

4d0926f

remove files Fix some parsing errors Formatting Change rpm_per_tick

Tests

0b5bf29

DerekMaggio requested review from sfoster1, sanni-t and pmoegenburg July 13, 2022 17:25

DerekMaggio self-assigned this Jul 13, 2022

DerekMaggio marked this pull request as ready for review July 13, 2022 17:25

DerekMaggio requested review from a team as code owners July 13, 2022 17:25

sanni-t reviewed Jul 14, 2022

View reviewed changes

Modify home functionality

dde5f4a

Formatting and linting

DerekMaggio changed the base branch from edge to heater-shaker-firmware-emulator-wip July 15, 2022 14:58

jbleon95 reviewed Jul 19, 2022

View reviewed changes

api/src/opentrons/hardware_control/emulation/heater_shaker.py Outdated Show resolved Hide resolved

SyntaxColoring reviewed Jul 19, 2022

View reviewed changes

api/tests/opentrons/hardware_control/integration/test_heatershaker.py Show resolved Hide resolved

jbleon95 and others added 2 commits July 19, 2022 13:49

fix rpm emulator g-code and add deactivate test

a6c497f

Fix tests

1ed6115

Add deactivate_heater G-Code

0926e14

SyntaxColoring reviewed Jul 19, 2022

View reviewed changes

Add start_set_temperature

4beaaca

Update home and deactivate_heater functionality

26f98cd

Based off of #11121 (comment)

Linting

8f694cf

sanni-t reviewed Jul 20, 2022

View reviewed changes

DerekMaggio closed this Jul 21, 2022

DerekMaggio reopened this Jul 21, 2022

Derek Maggio added 2 commits July 21, 2022 07:36

fix: Fix deactivate_heater function

3fb1547

Make home_delay_time customizable

8f46bc9

DerekMaggio requested a review from sanni-t July 21, 2022 14:37

Linting

cfe7d93

sanni-t approved these changes Jul 21, 2022

View reviewed changes

DerekMaggio merged commit a98742a into heater-shaker-firmware-emulator-wip Jul 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Heater Shaker Firmware Level Emulator #11121

feat(api): Heater Shaker Firmware Level Emulator #11121

DerekMaggio commented Jul 13, 2022

codecov bot commented Jul 13, 2022 •

edited

Loading

DerekMaggio commented Jul 14, 2022

sanni-t left a comment •

edited

Loading

DerekMaggio commented Jul 15, 2022

DerekMaggio commented Jul 15, 2022

DerekMaggio commented Jul 19, 2022

SyntaxColoring Jul 19, 2022

DerekMaggio Jul 20, 2022

DerekMaggio Jul 20, 2022

DerekMaggio Jul 20, 2022

sanni-t commented Jul 20, 2022

DerekMaggio commented Jul 20, 2022

sanni-t commented Jul 20, 2022 •

edited

Loading

sanni-t Jul 20, 2022

sanni-t commented Jul 20, 2022 •

edited

Loading

sfoster1 commented Jul 21, 2022

DerekMaggio commented Jul 21, 2022 •

edited

Loading

sanni-t left a comment

feat(api): Heater Shaker Firmware Level Emulator #11121

feat(api): Heater Shaker Firmware Level Emulator #11121

Conversation

DerekMaggio commented Jul 13, 2022

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Jul 13, 2022 • edited Loading

Codecov Report

DerekMaggio commented Jul 14, 2022

sanni-t left a comment • edited Loading

Choose a reason for hiding this comment

DerekMaggio commented Jul 15, 2022

DerekMaggio commented Jul 15, 2022

DerekMaggio commented Jul 19, 2022

SyntaxColoring Jul 19, 2022

Choose a reason for hiding this comment

DerekMaggio Jul 20, 2022

Choose a reason for hiding this comment

DerekMaggio Jul 20, 2022

Choose a reason for hiding this comment

DerekMaggio Jul 20, 2022

Choose a reason for hiding this comment

sanni-t commented Jul 20, 2022

DerekMaggio commented Jul 20, 2022

sanni-t commented Jul 20, 2022 • edited Loading

sanni-t Jul 20, 2022

Choose a reason for hiding this comment

sanni-t commented Jul 20, 2022 • edited Loading

sfoster1 commented Jul 21, 2022

DerekMaggio commented Jul 21, 2022 • edited Loading

sanni-t left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 13, 2022 •

edited

Loading

sanni-t left a comment •

edited

Loading

sanni-t commented Jul 20, 2022 •

edited

Loading

sanni-t commented Jul 20, 2022 •

edited

Loading

DerekMaggio commented Jul 21, 2022 •

edited

Loading