feat(api): allow robot to discover thermocycler and return live data #3239
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In addition to allowing for thermocyclers to be discovered by the hardware controller and returned
from a GET /modules call, this adds a synchronous adapter to the module class instances to allow for
communication with peripherals with out clogging the main pipes. Now all attached modules to a
hardware controller instance will be wrapped in a synchronous adapter. Calls to GET
/modules/{serial}/data should return the expected payload in the presence of a connected
thermocycler board. Note: the firmware should shim the serial and model strings until further
development on the FW side.
Closes #2958
b-cooper
added
api
Codecov Report
@@ Coverage Diff @@
## edge #3239 +/- ##
==========================================
+ Coverage 53.71% 54.61% +0.89%
==========================================
Files 699 700 +1
Lines 20470 20930 +460
==========================================
+ Hits 10996 11431 +435
- Misses 9474 9499 +25
Continue to review full report at Codecov.
|
sfoster1
requested changes
Mar 21, 2019
| @@ -312,10 +316,14 @@ def port(self) -> Optional[str]: | |||
| def lid_status(self): | |||
| return self._lid_status | |||
|
|
|||
| def get_device_info(self): | |||
| raise NotImplementedError | |||
| async def get_device_info(self): | |||
There was a problem hiding this comment.
We should add type annotations here, including for the return type, and carefully consider whether returning None is the right thing to do if there's no response (vs raising an exception)
Laura-Danielle
requested changes
Mar 21, 2019
| @@ -60,6 +60,7 @@ def _write_to_device_and_return(cmd, ack, device_connection): | |||
| - return parsed response''' | |||
| log.debug('Write -> {}'.format(cmd.encode())) | |||
| device_connection.write(cmd.encode()) | |||
| log.debug(f"After write -> cmd encoded: {cmd.encode()}, connection: {device_connection}, ack: {ack}") | |||
There was a problem hiding this comment.
Do we want to keep this debug statement here?
There was a problem hiding this comment.
fwiw I think that's a perfectly ok thing to have, it is debug level after all
| @@ -155,7 +153,7 @@ def _wait_for_ack(self): | |||
| self._send_command(SERIAL_ACK, timeout=DEFAULT_TC_TIMEOUT) | |||
|
|
|||
| def _send_command(self, command, timeout=DEFAULT_TC_TIMEOUT): | |||
| command_line = command + ' ' + TC_COMMAND_TERMINATOR | |||
| command_line = command + ' ' + SERIAL_ACK # TC_COMMAND_TERMINATOR | |||
There was a problem hiding this comment.
Should we put this back to TC_COMMAND_TERMINATOR?
| @@ -13,13 +13,14 @@ def __init__(self): | |||
| self._port = None | |||
| self._lid_status = 'open' | |||
|
|
|||
| def open(self): | |||
| async def open(self): | |||
| print(f"FROM OPEN INSIDE SIMULATING DRIVER active: {self._active}, lid_status: {self._lid_status}") | |||
There was a problem hiding this comment.
pls remove print statement here.
| self.disconnect() | ||
| self._poller = TCPoller( | ||
| port, self._interrupt_callback, self._temp_status_update_callback) | ||
|
|
||
| # Check initial device lid state | ||
| _lid_status_res = self._write_and_wait(GCODES['GET_LID_STATUS']) | ||
| self._lid_status = _lid_status_res.split()[-1].lower() | ||
| log.debug(f"Connected before get lid status write: {self._poller}") |
There was a problem hiding this comment.
Do we need this debug statement?
There was a problem hiding this comment.
Debug statements are ok if they're logged at debug level
| self._lid_status = _lid_status_res.split()[-1].lower() | ||
| log.debug(f"Connected before get lid status write: {self._poller}") | ||
| _lid_status_res = await self._write_and_wait(GCODES['GET_LID_STATUS']) | ||
| log.debug(f"Connected AFTER get lid status write: {_lid_status_res}") |
There was a problem hiding this comment.
Same here? Or maybe make it a more helpful debug statement if we keep it.
Laura-Danielle
approved these changes
Mar 21, 2019
sfoster1
approved these changes
Mar 22, 2019
|
LGTM pending CI |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In addition to allowing for thermocyclers to be discovered by the hardware controller and returned
from a GET /modules call, this adds a synchronous adapter to the module class instances to allow for
communication with peripherals with out clogging the main pipes. Now all attached modules to a
hardware controller instance will be wrapped in a synchronous adapter. Calls to GET
/modules/{serial}/data should return the expected payload in the presence of a connected
thermocycler board.
Note: the firmware should shim the serial and model strings until further
development on the FW side. I will follow up with a quick PR in the FW repo that I'll link here: [], that adds the stubbed strings for now.
Closes #2958