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.

Sign up for GitHub

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

Jump to bottom

[WIP] Support for remote serial ports #176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[WIP] Support for remote serial ports #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c415486
Added SerialBase to allowed serial port bases.
crazy4pi314 Jul 11, 2017
66e0a8b
Added open_serial_remote method. Still not fully working with APT b/c…
crazy4pi314 Jul 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions instruments/abstract_instruments/comm/serial_communicator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,13 @@ class SerialCommunicator(io.IOBase, AbstractCommunicator):
def __init__(self, conn):
super(SerialCommunicator, self).__init__(self)

if isinstance(conn, serial.Serial):
allowed_bases = (serial.Serial, )
try:
allowed_bases += (serial.serialutil.SerialBase, )
except AttributeError:
pass

if isinstance(conn, allowed_bases):
self._conn = conn
self._terminator = "\n"
self._debug = False
Expand Down Expand Up @@ -172,8 +178,10 @@ def _sendcmd(self, msg):

:param str msg: The command message to send to the instrument
"""
msg += self._terminator
self.write(msg)

message = msg.decode(encoding='UTF-8')
message += self._terminator
self.write(message)

def _query(self, msg, size=-1):
"""
Expand Down
25 changes: 23 additions & 2 deletions instruments/abstract_instruments/instrument.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
import os
import collections
import socket
import serial

from builtins import map
from serial import SerialException
from serial import SerialException
from serial.tools.list_ports import comports

from future.standard_library import install_aliases
Expand All @@ -41,7 +42,7 @@ class WindowsError(OSError):
from instruments.abstract_instruments.comm import (
SocketCommunicator, USBCommunicator, VisaCommunicator, FileCommunicator,
LoopbackCommunicator, GPIBCommunicator, AbstractCommunicator,
USBTMCCommunicator, VXI11Communicator, serial_manager
USBTMCCommunicator, VXI11Communicator, serial_manager, SerialCommunicator
)
from instruments.errors import AcknowledgementError, PromptError

Expand Down Expand Up @@ -524,6 +525,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,
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

All excellent points

xonxoff = True, timeout=3):
"""
This method follows updates in the API for PySerial so that IK can use the new
serial.serial_for_url constuctor for serial devices.
TODO: Finish me
"""
# baudrate=9600, bytesize=8, parity='N', stopbits=1, timeout=None, xonxoff=False, rtscts=False, dsrdtr=False
#Should put in parsing to check for valid URL?
ser = serial.serial_for_url(
url,
baudrate=baud,
timeout=timeout,
xonxoff=xonxoff,
rtscts=rtscts,
dsrdtr=dsrdtr
)
return cls(SerialCommunicator(ser))

@classmethod
def open_gpibusb(cls, port, gpib_address, timeout=3, write_timeout=3):
"""
Expand Down
3 changes: 2 additions & 1 deletion instruments/thorlabs/thorlabsapt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

revert unrelated changes

Copy link
Contributor

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.

},
# TODO: add other tables here.
}
Expand Down