Conversation
Kenadia
left a comment
Kenadia
left a comment
There was a problem hiding this comment.
Added some minor comments and a question about error handling. Thanks!
| Allows for writing out to a serial port. | ||
| """ | ||
|
|
||
| from contextlib import contextmanager |
There was a problem hiding this comment.
style: prefer import contextlib
| def save_data(self, dest): | ||
| """Save serial data to the named destination while in context. | ||
|
|
||
| Spawns a thread that will open the configured serial port, continously |
| collection_thread.daemon = True | ||
| try: | ||
| self.logger.debug( | ||
| 'Starting serial data collection on %s.' % self._serial.port) |
There was a problem hiding this comment.
nit - ...collection on port %s.
| yield | ||
| except self.SERIAL_EXCEPTIONS: | ||
| self.logger.error('Serial port error. Stopping data collection.', | ||
| exc_info=True) |
There was a problem hiding this comment.
I'm suspicious of this except clause. It will catch certain errors, including any ValueErrors, that occur in the code managed by this context. But it won't catch errors that occur in the thread. Is that the intention? If so, could you give an example of how this plug is used, and what kind of ValueErrors you're trying to catch? Catching all ValueErrors in the managed block doesn't seem right
There was a problem hiding this comment.
Thanks for your input! Upon further inspection we decided to retool save_data to start_collection() and stop_collection(), which doesn't use the context manager anymore.
|
|
||
| from contextlib import contextmanager | ||
| import threading | ||
| import serial |
There was a problem hiding this comment.
Consider adding something like
try:
import serial
except ImportError:
logging.error('Failed to import pyserial. Please install the `serial_collection_plug` extra, '
'e.g. via `pip install openhtf[serial_collection_plug]`.')
raiseI notice we do this for the usb plugs.
Kenadia
left a comment
Kenadia
left a comment
There was a problem hiding this comment.
Overall LGTM, just a couple more small questions
| stop_collection() is called. | ||
| """ | ||
| SERIAL_EXCEPTIONS = (serial.SerialException, | ||
| serial.SerialTimeoutException, |
There was a problem hiding this comment.
serial.SerialTimeoutException is a subclass of serial.SerialException so it's not needed here
| """ | ||
| SERIAL_EXCEPTIONS = (serial.SerialException, | ||
| serial.SerialTimeoutException, | ||
| ValueError) |
There was a problem hiding this comment.
Could use a comment here explaining what this is intended to catch
| not self._collection_thread.is_alive()): | ||
| self.logger.warning( | ||
| 'Data collection was not running, cannot be stopped.') | ||
| return |
There was a problem hiding this comment.
Just some minor observations here. I wonder what an example usage of this API looks like. It looks like the code using this plug can't tell if the thread is still running, so it can't know whether it's appropriate to call stop_collection() or not. Because of this I imagine usage looks like:
try:
serial_plug.start_collection()
finally:
serial_plug.stop_collection()This makes me think we should log an error only if start_collection() was never called, but not if the thread was started and errored out. (The warning would be sort of redundant with the error.) It also makes me wonder why a context manager is no longer used.
There was a problem hiding this comment.
Good point, we made the state of collecting available to the test author (is_collecting property)
- Upstreaming serial collection plug
- Addressed a couple more nits on documentation and exposing thread status.
3 participants
This change is