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(emulation): emulation refactor #8587

Merged
merged 16 commits into from
Nov 9, 2021
Merged

Conversation

amitlissack
Copy link
Contributor

@amitlissack amitlissack commented Oct 23, 2021

Overview

This PR refactors how our module emulation works.

This is part 1. While the connection model has changed, the emulator module discovery has NOT changed. The hardware controller still relies on environment variables to connect to emulated modules.

This is part 2

Changelog

  • Module emulators are no longer be socket servers, but clients.
  • Added Proxy class to treat emulator client connections as a resource to be acquired by a driver client connection. Data is routed from driver socket to emulator socket.
  • One Proxy server per module type.
  • Added ModuleStatusServer that notifies clients of available emulators over a socket. This is the emulator's equivalent of aionotify module discovery on the robot.

Review requests

While almost all of the changes only relate to emulation, there are a few changes to hardware controller that will require some testing:

  • Slight refactors of ModuleControl class. Specifically how USB objects are constructed.
  • Module classes explicitly disconnect during clean up .

Risk assessment

Very low

@amitlissack amitlissack requested review from DerekMaggio and a team October 23, 2021 14:17
@codecov
Copy link

codecov bot commented Oct 23, 2021

Codecov Report

Merging #8587 (5b2191b) into edge (80093b3) will increase coverage by 0.13%.
The diff coverage is 88.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #8587      +/-   ##
==========================================
+ Coverage   75.22%   75.36%   +0.13%     
==========================================
  Files        1208     1830     +622     
  Lines       37931    48595   +10664     
  Branches     2792     4706    +1914     
==========================================
+ Hits        28535    36622    +8087     
- Misses       8995    11173    +2178     
- Partials      401      800     +399     
Flag Coverage Δ
api 84.71% <88.48%> (+0.06%) ⬆️
g-code-testing 97.28% <89.58%> (?)
robot-server 93.70% <ø> (+0.25%) ⬆️

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

Impacted Files Coverage Δ
api/src/opentrons/hardware_control/simulator.py 85.41% <ø> (-0.20%) ⬇️
...rons/hardware_control/emulation/scripts/run_app.py 71.42% <71.42%> (ø)
...e_control/emulation/scripts/run_module_emulator.py 75.00% <75.00%> (ø)
...ardware_control/emulation/module_server/helpers.py 76.36% <76.36%> (ø)
...hardware_control/emulation/module_server/server.py 80.00% <80.00%> (ø)
...pi/src/opentrons/hardware_control/emulation/app.py 88.00% <86.66%> (+10.22%) ⬆️
g-code-testing/g_code_parsing/g_code_engine.py 92.50% <88.37%> (ø)
.../src/opentrons/hardware_control/emulation/proxy.py 91.76% <91.76%> (ø)
...hardware_control/emulation/module_server/client.py 94.11% <94.11%> (ø)
...entrons/hardware_control/emulation/run_emulator.py 96.00% <96.00%> (ø)
... and 649 more

@amitlissack amitlissack force-pushed the emulation-refactor-phase-2 branch from 12fff3b to 6ae4a1e Compare October 23, 2021 14:21
@amitlissack amitlissack changed the title refactor(emulation): Emulation refactor phase 2 refactor(emulation): (WIP) emulation refactor phase 2 Oct 23, 2021
@amitlissack amitlissack force-pushed the emulation-refactor-phase-2 branch from e5ffa15 to b1c2391 Compare October 27, 2021 16:29
@amitlissack amitlissack changed the title refactor(emulation): (WIP) emulation refactor phase 2 refactor(emulation): emulation refactor phase 2 Oct 27, 2021
@amitlissack amitlissack marked this pull request as ready for review October 27, 2021 17:52
@amitlissack amitlissack requested a review from a team as a code owner October 27, 2021 17:52
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.

Nice, I like it. It's a little weird to me that you have to write code to specify which modules get loaded but I guess that's in the followup pr and also not hard to change.

@amitlissack amitlissack changed the title refactor(emulation): emulation refactor phase 2 refactor(emulation): emulation refactor Oct 29, 2021
@amitlissack amitlissack requested a review from a team as a code owner November 4, 2021 17:38
@amitlissack amitlissack requested review from larvazquez and a team and removed request for larvazquez November 9, 2021 14:20
Copy link
Contributor

@DerekMaggio DerekMaggio left a comment

Choose a reason for hiding this comment

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

Reviewed this last week and forgot to approve. My bad

amit lissack and others added 13 commits November 9, 2021 12:01
run_emulator
* Module control refactors

* new package.

* start module control integration.

* tests for module control integration.

* implement connection listener.

* module server integrated into module control.

* lint

* g-code-testing

* redo docker compose to deal with separate emulator apps.

* usb port.

* expand module emulation settings.

* update docker readme.

* format-js

* lint

* go back to threadings. I don't want to debug windows nonsense.

* fix bug.

* redo gcode testing's emulator setup.

* documentation.

* clean up.
@amitlissack amitlissack force-pushed the emulation-refactor-phase-2 branch from 3198269 to c58e5eb Compare November 9, 2021 18:30
amitlissack added 2 commits November 9, 2021 14:40
* faster g-code-tests in ci

* reduce hold time.

* use dev folder in s3
@amitlissack amitlissack merged commit b97f1bb into edge Nov 9, 2021
@amitlissack amitlissack deleted the emulation-refactor-phase-2 branch November 9, 2021 22:03
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