Conversation
campagnola
left a comment
campagnola
left a comment
There was a problem hiding this comment.
Overall this looks really good! I have some comments below, and one major suggestion: consider making this code thread-safe by adding a mutex that locks the serial communication for each send/receive cycle. Acq4 does a lot of multithreading under the hood, so you could end up in a situation where two threads are trying to make simultaneous calls to the device.
|
|
||
| __author__ = """arielle leon""" | ||
| __email__ = 'ariellel@alleninstitute.org' | ||
| __version__ = '0.1.0' |
There was a problem hiding this comment.
init needs to import the Thorlabs DC4100 class.
Also: I recommend omitting the author/email/version.
| self.escape = '\n\n' | ||
| self.read_buffer = [] | ||
|
|
||
| def led_on(self, channel: int): |
There was a problem hiding this comment.
This will cause a syntax error in python 2. I hope python 3 support is coming soon, but currently micromanager is also py2 only and we don't know when that will be fixed..
| "error_status": "E?" | ||
| } | ||
| class ThorlabsDC4100: | ||
| def __init__(self,port,baudrate,timeout): |
There was a problem hiding this comment.
style comments:
- PEP8 likes spaces after commas (and I think it makes the code easier to read).
- Acq4 uses camelCase instead of snake_case; consistency is preferred here. (this decision was originally inherited from Qt, although I regret it somewhat at this point..)
| self._write_to_LED(COMMANDS["led_on"].format(channel)) | ||
|
|
||
| def led_off(self,channel: int): | ||
| self._write_to_LED(COMMANDS["led_off"].format(channel)) |
There was a problem hiding this comment.
Another minor suggestion: by rewriting _write_to_LED a little, you could turn this into
self._write_to_LED('led_off', channel)
..which reduces the amount of repeated code and makes everything more readable.
| self.dev.write(f"{command} {self.escape}".encode()) | ||
|
|
||
| def _read_from_LED(self): | ||
| while self.dev.is_open: |
There was a problem hiding this comment.
If self.dev.is_open becomes False before this method has returned, then it will just return None, whereas I assume raising an exception would be more appropriate. You'd also potentially have junk left in self.read_buffer. For context: serial errors are relatively common in our setup, so making this bit robust is key. I recommend reading over (and maybe using) the code in https://github.com/acq4/acq4/blob/develop/acq4/drivers/SerialDevice.py, since it incorporates many of the lessons we learned through years of dealing with serial failures.
| if len(self.read_buffer) == 0 and output == '\r': | ||
| self.dev.flush() | ||
| continue | ||
| if output == "\n": |
There was a problem hiding this comment.
if I am not mistaken, it's possible for output to contain multiple bytes ending with \n, in which case we are stuck in an infinite loop here (and in general, there seem to be a few other ways we could end up stuck here forever, depending on how (un)reliable the serial communication is). Something like readUntil (https://github.com/acq4/acq4/blob/develop/acq4/drivers/SerialDevice.py#L156) might work here?
2 participants
I have added the ThorlabsDC4100 driver to the acq4 drivers directory. This is an LED device used for the IVSCC upgrade that can communicate via a USB serial connection. The device can connect to an LED hub which can hold up to 4 LEDs.
Documentation for hardware can be found here:
DC4100 Device: https://www.thorlabs.com/thorproduct.cfm?partnumber=DC4100
DC4100 Hub: https://www.thorlabs.com/thorproduct.cfm?partnumber=DC4100-HUB