I'm new to Python, and after running through some introductory exercises I wrote a class definition for working with some hardware, specifically a Signal Recovery model 7230 lockin amplifier. In the interests of learning good habits from the beginning, I've tried to write "good code" that is properly documented, follows good practices and so forth.

""" Module for  interfacing with Signal Recovery 7230 lock-in amplifier.

A substantial ammount is currently not implmented. It will not work with
a lockin which is configured for double demodulation. If you get a lot
of timeouts, that might be why.

It is also currently missing any way to configure the lockin. Use the
web interface. It does read and expose the status bytes, but at the moment
there is no checking of their content.

@author: Jack Barraclough

import pyvisa

class WrongInstrumentError(Exception):
    """The wrong instrument is connected

    A connection was successfuly established, and the instrument responded
    to a request to identify itself, but the ID recieved was wrong.
    Probably the instrument at the given VISA identifier is not the one
    you wanted.    

class Instrument:
    """Represents a Signal Recovery Model 7230 lock-in amplifier

    Note that several instances can be created representing the same
    instrument. Be careful.
    Instance variables:
      pyvisa -- a pyvisa object for the connection, avoid using.
      status_byte -- status byte after the last read operation.
      overload_byte -- overload byte after the last read operation.
      read, write -- Read and write strings to/from the instrument.
      get_X, get_Y, get_R, get_T -- get measurements from the instrument.
      get_noise -- get the estimated measurement noise

    def __init__(self,visaName):
        """ Connects to the lockin amplifier

        Connects to the lockin, and checks that the lockin is present and
        correct. An error will be raised if the lockin is not there, or
        if it fails to identify itself as a model 7230
          visaName -- A Visa Resource ID, like 'TCPIP0::'
        rm = pyvisa.ResourceManager()
        self.pyvisa = rm.open_resource(visaName)
        self.write('ID') # uses this not '*IDN?'
        resp = self.read()
        if resp != '7230':
            raise WrongInstrumentError(
                'Wrote "ID" Expected "7230" got "{}"'.format(resp))

    def write(self,string):
        """Write string to the instrument."""

    def read(self):
        """Read string from the instrument. Also sets status bytes"""
        # replies seem to be: response LF NUL then two status bytes
        resp = self.pyvisa.read().strip()
        status = self.pyvisa.visalib.read(self.pyvisa.session,2)[0]
        self.status_byte = status[0]
        self.overload_byte = status[1]
        return resp

    def get_X(self):
        """Get X value from the instrument, returns a float"""
        return float(self.read())

    def get_Y(self):
        """Get Y value from the instrument, returns a float"""
        return float(self.read())

    def get_R(self):
        """Get R value from the instrument, returns a float"""
        return float(self.read())

    def get_T(self):
        """Get theta value from the instrument, returns a float"""
        return float(self.read())

    def get_noise(self):
        """Get Y noise from the instrument, in v/sqrtHz, returns a float"""
        return float(self.read())

I'm interested in general feedback but also have some specific questions:

  1. Am I using exceptions correctly?
  2. The pyvisa resource manager has a rm.close() method. By not calling it, am I leaking something? Will the garbage collector deal with it? Is it bad form to leave it to the garbage collector? Should I clear it up with a destructor somehow?

Your code so far looks pretty good - well structured, useful docstrings, well done! You mention exceptions; the one occasion you use it seems fine to me.

Per the style guide, e.g. visaName should be visa_name. Also, rather than commenting pyvisa -- a pyvisa object for the connection, avoid using., it is conventional to prepend the names of attributes that shouldn't be used externally with a single underscore: _pyvisa.

Rather than including @author in your docstring, you could use one of Python's special module attributes:

__author__ = 'Jack Barraclough'

get methods aren't very Pythonic. Instead, I would suggest you use properties, for example:

def R(self):
    """Get R value from the instrument, returns a float"""
    return float(self.read())

Now, rather than instrument.get_R(), you can type simply instrument.R.

Going further, there is a lot of duplication - every single one calls self.write then returns the value from self.read. You could factor this out entirely:

class Instrument:

    PROPERTIES = {'R': 'MAG.', ...}

    def __getattr__(self, attr):
        if attr in self.PROPERTIES:
            return self._get(self.PROPERTIES[attr])
        msg = "{!r} object has no attribute {!r}"
        raise AttributeError(msg.format(self.__class__.__name__, attr))

    def __setattr__(self, attr, val):
        if attr in self.PROPERTIES:
            raise AttributeError("can't set attribute")
        return super().__setattr__(attr)

    def _get(self, value):
        return float(self.read())

I admit I don't know much about pyvisa, but one option to ensure the resource is released would be to make your class a context manager type. Then a user could do e.g.

with Instrument(whatever) as inst:

and be certain that everything was dealt with on __exit__.

  • \$\begingroup\$ Helpful, thanks. I don't fully understand the __setattr__ and __getattr__ bit, but I expect I'll get there after reading the appropriate docs. I'll come back and upvote once I've done that. \$\endgroup\$ – Jack B Mar 16 '15 at 10:58
  • \$\begingroup\$ @JackB you can vote up from 15 rep - nearly there! \$\endgroup\$ – jonrsharpe Mar 18 '15 at 11:05
  • \$\begingroup\$ @Evert ...the first one! \$\endgroup\$ – jonrsharpe Aug 4 '17 at 8:34

I don't know about pyvisa either, but I have pretty similar scripts to interface instruments with VXI. In that case, the talking is done through a tcp socket and not closing it may lead to problems on the next run of the script. However, it is possible the pyvisa takes care of these things for you. In my case I used a context manager for it.

Depending on the instrument you may need to make the __setattr__ more complicated, since you may be not only reading but also changing some settings in the instrument. Probably you would need a few lists like ReadOnly_PROPERTIES and ReadWrite_PROPERTIES.

@jonrsharpe I had not seen before that way of defining a bunch of properties in a single go. Awesome idea! (unfortunately I don't have karma to upvote).

  • \$\begingroup\$ Please don't reply to another answers via answers. Hang around a bit and you'll soon earn the rep to comment. \$\endgroup\$ – RubberDuck Mar 16 '15 at 19:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.