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): add module emulation #7353

Merged
merged 11 commits into from
Mar 17, 2021
Merged

feat(api): add module emulation #7353

merged 11 commits into from
Mar 17, 2021

Conversation

amitlissack
Copy link
Contributor

@amitlissack amitlissack commented Feb 18, 2021

Overview

In our HMG-CPX sync we discussed refactoring the hardware controller to go full on asyncio. When I've thought about how to tackle this task I've felt dread over how I'd test it. Would it be mandatory to have an OT2 and all the modules to make any progress?

So I looked into how to emulate the smoothie and modules. Happily, pyserial supports alternative underlying transport methods. Specifically over a socket.

Changelog

This PR introduces very elementary emulation of our three modules. Each module (and eventually smoothie) is a socket server to process GCODE commands. Only commands that read parameters are implemented. The emulators live in opentrons.hardware_control.emulation. An app module can be run to start all modules.

Finally, some tests are added to exercise the emulators: one for each module. A fixture starts the emulators. We create module objects using pyserial's socket url (socket://127.0.0.1:XXXX) to build the module objects.

This PR does NOT enable running the robot-server connecting to the emulators. I would like to support that eventually. I could see a docker configuration that runs the robot-server with emulators and environment set up for fast development. I would use that all the time.

Review requests

Testing: does serial comms still work?

  • This is pretty hastily put together as a proof of concept.
  • Do we want this? If so, should it live in opentron? I could see this being a separate package.
  • I would ignore some of the hackery I added to get Thermocycler tests to run. I feel pretty strongly that the TCPoller will be modified making the hacks unnecessary.
  • The new Thermocycler test fails on Windows. It's using very unix-y file descriptor techniques.

Risk assessment

Small changes to how we create Serial objects.

@amitlissack amitlissack added WIP RFC Software proposal ("request for comment") labels Feb 18, 2021
@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

❗ No coverage uploaded for pull request base (edge@3b7830d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #7353   +/-   ##
=======================================
  Coverage        ?   81.94%           
=======================================
  Files           ?      321           
  Lines           ?    21419           
  Branches        ?        0           
=======================================
  Hits            ?    17551           
  Misses          ?     3868           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b7830d...7759b0f. Read the comment docs.

@amitlissack amitlissack requested review from a team, ahiuchingau and mcous and removed request for a team February 18, 2021 14:24
@amitlissack amitlissack added api Affects the `api` project robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). hmg hardware, motion, and geometry labels Feb 18, 2021
@@ -80,8 +80,8 @@ def _write_to_device_and_return(cmd, ack, device_connection, tag=None):


def _connect(port_name, baudrate):
ser = serial.Serial(
port=port_name,
ser = serial.serial_for_url(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under the covers, if the url doesn't specify a protocol (ie "socket://") pyserial will Serial.

@@ -190,6 +190,8 @@ async def enter_programming_mode(self):


class TCPoller(threading.Thread):
POLLING_FD_PATH = '/var/run/'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved here in order be able to patch in tests. Otherwise need root permission.

@amitlissack amitlissack marked this pull request as ready for review February 18, 2021 16:34
@amitlissack amitlissack requested a review from a team as a code owner February 18, 2021 16:34
@amitlissack amitlissack changed the title feat(api): add hardware emulation feat(api): add module emulation Feb 18, 2021

@pytest.fixture(scope="session")
def emulation_app():
""""""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add explanation for Thread.

Remove commnted out line

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

This is really cool!
It's a great setup. I think the things I mentioned during the review meeting about adding more gcode responses can be handled in follow-up PRs if it works for you.

@amitlissack amitlissack added ready for review and removed RFC Software proposal ("request for comment") WIP labels Feb 23, 2021
@amitlissack amitlissack requested a review from a team February 23, 2021 21:18
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

I think this is a really good start. The idea of eventually moving this to its own repository (with a Docker container) really resonates with me, but I think this will be helpful off the bat for testing out new serial driver code

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

🎮

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.

Really good start, thanks for taking this on! The code looks good to me, but we should definitely test

logger.info(f"Got command {cmd}")
if cmd == GCODE_HOME:
pass
elif cmd == GCODE_MOVE:
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't these set height

Copy link
Contributor Author

@amitlissack amitlissack Feb 25, 2021

Choose a reason for hiding this comment

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

It definitely should. I deliberately did not implement any command that changes the state of any of the emulators.

Looking at the c++ code made think that updating the state is not at all trivial for any of these modules and the smoothie. So left it for another day.

@amitlissack amitlissack force-pushed the api-hardware-emulation branch from ff97f3c to 7759b0f Compare March 17, 2021 16:39
@amitlissack amitlissack merged commit 78869dc into edge Mar 17, 2021
@amitlissack amitlissack deleted the api-hardware-emulation branch March 17, 2021 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project hmg hardware, motion, and geometry robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants