-
Notifications
You must be signed in to change notification settings - Fork 70
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
[WIP] Support for remote serial ports #176
base: main
Are you sure you want to change the base?
Conversation
@@ -474,7 +474,8 @@ class MotorChannel(ThorLabsAPT.APTChannel): | |||
'DRV113': (pq.Quantity(20480, 'ct/mm'),) * 3, | |||
'DRV114': (pq.Quantity(20480, 'ct/mm'),) * 3, | |||
'FW103': (pq.Quantity(25600 / 360, 'ct/deg'),) * 3, | |||
'NR360': (pq.Quantity(25600 / 5.4546, 'ct/deg'),) * 3 | |||
'NR360': (pq.Quantity(25600 / 5.4546, 'ct/deg'),) * 3, | |||
'PRM1-Z8': (pq.Quantity(1919.64 / 360 ,'ct/deg'),) * 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert unrelated changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks, we had that included in our local testing and committed by accident. That really belongs in #167, probably.
@@ -525,6 +526,26 @@ def open_serial(cls, port=None, baud=9600, vid=None, pid=None, | |||
return cls(ser) | |||
|
|||
@classmethod | |||
def open_serial_remote(cls, url=None, baud=115200, dsrdtr=False, rtscts=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I wonder if the better approach is to include this functionality in open_serial
and just add more named parameters to that function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somewhat agree, but I'm also of mixed minds. The API that PySerial offers for local and remote serial ports is subtly different enough that it might make open_serial
confusing to call if it does both. Would something like remote=True
be enough, do you think? If so, we could do both by having the old open_serial
and new open_serial_remote
both basically be private methods called by a new combined open_serial
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In open_serial
, you already offer the option to define the device either with port
or with the pid
/vid
/serial_number
combination. I think, if the logic complexity does not explode, it would be better to add another url
parameter instead of having to maintain a separate class method just for the for_url
approach.
Also, even if you don't do that, one thing that smells odd to me (without having dug too deep into the api) is that the array of configuration parameters is different between open_serial
and open_serial_remote
, (baud/timeout/write_timeout vs baud/dsrdtr/rtscts/xonxoff/timeout).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All excellent points
Since the original development of IK, PySerial has added additional support for specifying remote serial ports. Here a new method is added for using this new specification (documented in PySerial here). I needed this because Thorlabs comm ports are not exposed on Windows, and so I can forward with this PySerial example to a Linux machine to actually write to the device. There is also maybe one or two changes that I needed to make for the byte/string python versioning issue, which can be removed once #167 is done.