Instrument driver adidtions

[MerlinSmiles]

I added a bunch of drivers for some instruments I use:

• Oxford Triton fridge
• Oxford Magnet
• Keithley 2600
• Keitley 2400 (this one is absolutely not finished)
• Agilent 34400A

Also I added some notebooks where I use them.

@alexcjohnson
I had to change the IPinstrument to be able to talk to the Triton computer.
The loops crashed for some reason when background = false

@alexcjohnson, in general, when I have to fix something in the base stuff, would I add them to a pull request like this?

How do I get information from my instrument drivers like self.axes = ['X', 'Y', 'Z'] out to the remote instrument thingy?

How do I add a combining parameter?
I the magnet I have params for x_fld,y_fld,z_fld I also want a fld param that sets and retrurns [x,y,z] how would I do this in a smart way?

I think the drivers I have written should be carefully reviewed for coding style, especially all those partials and string matching / formatting things, also I have some eval things in the magnet driver.

[alexcjohnson]

Cool! I'll look at this tomorrow, but to answer your general question:

when I have to fix something in the base stuff, would I add them to a pull request like this?

Yes, that's appropriate, you're not adding a new feature in the core, just fixing an existing one to work with the new feature you're adding? (Which makes sense, because I hadn't gotten to testing IPInstrument at all yet... curious to see how the persistent switch works - cc @damazter ) But make sure you run the test suite after your changes and it still passes. Ideally you also create a test that would have failed prior to the fix and passes with it. IPInstrument has no tests written for it yet so don't worry about that, but Loop does so we should see if we can build on something there to test your change.

[alexcjohnson]

How do I get information from my instrument drivers like self.axes = ['X', 'Y', 'Z'] out to the remote instrument thingy?

is magnet.getattr('axes') what you're looking for? I suppose we could also construct a @property for each such attribute... but I haven't done that yet.

[MerlinSmiles]

@alexcjohnson Thanks for the comments, I know nothing about testing, which I probably should change soon, right now I have to focus on other stuff though.

I have another question, I think we will have several situations where an Instrument can be an IPinstrument or a VisaInstrument or whatever, what Is the proper way to change the base class of the driver according to the arguments given upon initialization?
I dont feel like using PyVisa for IP instruments, even though it is possible, I'd prefer the direct route.

class MercuryiPS(VisaInstrument):
    def __init__(self, name, axes=None, port=None, **kwargs):
         if port is not None:
             # Make me an IP instrument :)

[MerlinSmiles]

is magnet.getattr('axes') what you're looking for? I suppose we could also construct a @Property for each such attribute... but I haven't done that yet.

Great, yes :)

[alexcjohnson]

what Is the proper way to change the base class of the driver according to the arguments given upon initialization?

Ah, interesting... I wondered if this question would come up. I see some suggestions here though they seem a bit limiting to me. I might do something more explicit and flexible like:

class InstGeneric:
    def __init__(...):
        # add parameters, any init code that doesn't depend on the interface
    def some_method(...):
        # methods that are common to both interfaces

class InstVisa(VisaInstrument, InstGeneric):
    def __init__(...):
        super().__init__(...)  # or maybe separately call the two supers
        # then visa-specific init code, if any
    def other_method(...):
        # visa flavor

class InstIP(IPInstrument, InstGeneric):
    def __init__(...):
        super().__init__(...)
        # then ip-specific init code, if any
    def other_method(...):
        # ip flavor

def InstFinal(...):  # named to look like a class even though it's a function, since it
    if args say it is visa:
        return InstVisa(...)
    elif args say it is ip:
        return InstIP(...)

I'm curious how other people feel about this though - visa is certainly a nice abstraction layer, especially for instruments that have multiple interfaces like GPIB and IP... but it still means you're effectively tied to proprietary software (NI-VISA) where that's really not needed for anything but GPIB, so in as far as the GPIB era is winding down is it time for us to make an effort to provide non-visa options?

[alexcjohnson]

How do I add a combining parameter?
I the magnet I have params for x_fld,y_fld,z_fld I also want a fld param that sets and retrurns [x,y,z] how would I do this in a smart way?

hmm, that's an interesting case - we may want to actually make a Parameter subclass to support this because it's going to come up frequently and there are a bunch of things you should ideally do:

• make the get and set methods call out to the component parameters
• combine the component name, label, and units attributes into composite names, labels, and units
• _latest should also delegate to the components, and _save_val should be omitted entirely... otherwise the snapshot/get_latest for the combined would get out of sync from the components.

[alexcjohnson]

I know nothing about testing

As important as it is, I don't want to make testing a requirement for contributing, because that would leave out too many people! Just please run the test suite to make sure you didn't break it, and I'll try and take care of testing changes to the core (and including that in the PR before it's merged).

Eventually we'd like to have tests for all our instrument drivers, and that will be harder to centralize since no central person even has all the instruments... but I'm OK with leaving that part mostly until later.

[MerlinSmiles]

Yes I ran the test and it returns an OK as in the contributing description.
I also tried to add a test, but I culdnt figure out that assertion stuff.
Basically, for the loops your run_temp() assumes only one combination of arguments, what breaks the loop is a .run(location='something',background=False)

[MerlinSmiles]

This is actually also in #60

[alexcjohnson]

a couple of things:

• make this a kwarg default?
• this isn't a buffer, it's the buffer size, so should be named accordingly.
• I'm guessing you increased it from 512 because that wasn't big enough... presumably visa does something smarter here, which I'm guessing (since I don't see anything in the interface to ask whether there's any data waiting in the socket?) is repeatedly calling recv until it gets read_termination character(s). We should probably build this into IPInstrument as an option (ie if you supply read_termination). I can do that... but it should happen soon so new drivers can start using it. As it is we're going to be hitting weird intermittent bugs...

[MerlinSmiles]

Yes, I had an issue with asking for a general data, then the reply would contain all sub-datasets, which was a lot of data.
Isnt the buffer-size irrelevant for systems we use? does it slow anything down for real? i was considering making it ten times this amount to not run into this again.
And yes, I was also about to rename it to buffer_size, as that is what it is, but wouldnt want to mess too much

[alexcjohnson]

@dbwz8 do you want to weigh in? I don't know that it would slow things down to increase the size... but I don't think it would help either. see eg http://stackoverflow.com/questions/2613734/maximum-packet-size-for-a-tcp-connection

When we get a very big response, it will get broken into multiple packets. I don't actually know what goes on behind the scenes in a socket. Unless the protocol itself has some definition of a "message" that puts these back together for us, it's quite likely that at least some of the time recv will get only the first one so if we don't wait for some concrete termination we'll miss part of the response and leave it for the next read.

_buffer is new as of this PR, so now is the time to rename it.

[MerlinSmiles]

Ups, I renamed it

[dbwz8]

The stackoverflow response is basically correct. You have almost no real control over the packet size at the lower levels (which is sort of the point of TCP... it's a virtual stream and it's supposed to "do what's best"). You should always be thinking in terms of streams and not packets (unless you want to talk to IP directly). All the chopping up, re-transmitting and re-ordering is invisible at the TCP level (by design).

[dbwz8]

There is no guarantee that a single recv will fill the buffer with everything that was sent. This is why I’ve always used a header that tells me how many bytes are coming, so I know if I’ve gotten the whole (user level) message.

From: alexcjohnson [mailto:notifications@github.com]
Sent: Tuesday, April 19, 2016 10:08 PM
To: qdev-dk/Qcodes Qcodes@noreply.github.com
Cc: Dave Wecker dbwz8@hotmail.com
Subject: Re: [qdev-dk/Qcodes] Instrument driver adidtions (#74)

In qcodes/instrument/ip.py https://github.com/qdev-dk/Qcodes/pull/74#discussion_r60218430 :

@@ -31,6 +31,7 @@ def init(self, name, address=None, port=None, timeout=5,
self._confirmation = write_confirmation

     self._ensure_connection = EnsureConnection(self)

•    self._buffer = 1024
  

@dbwz8 https://github.com/dbwz8 thanks - just to follow up on this though, should I think of it as a stream of bytes or a stream of messages (ala websockets, for example). It's helpful that data arrives in the order it was sent, but is there any guarantee that what the sender puts into the socket together will come out together, ie that if we make a single recv call into a big enough buffer it will always get the whole message?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub https://github.com/qdev-dk/Qcodes/pull/74/files/835801f957648eaf27c0f84c3e6c4b1f14f507eb#r60218430 https://github.com/notifications/beacon/AKEAqWQO0HDP9dRLFFxrzgAXx_iOqm3xks5p5MWagaJpZM4IJQCO.gif

[dbwz8]

Yes.

From: MerlinSmiles [mailto:notifications@github.com]
Sent: Tuesday, April 19, 2016 9:54 PM
To: qdev-dk/Qcodes Qcodes@noreply.github.com
Cc: Dave Wecker dbwz8@hotmail.com
Subject: Re: [qdev-dk/Qcodes] Instrument driver adidtions (#74)

In qcodes/instrument/ip.py https://github.com/qdev-dk/Qcodes/pull/74#discussion_r60217008 :

@@ -31,6 +31,7 @@ def init(self, name, address=None, port=None, timeout=5,
self._confirmation = write_confirmation

     self._ensure_connection = EnsureConnection(self)

•    self._buffer = 1024
  

@dbwz8 https://github.com/dbwz8 So we should put it to 1460 (or 1400 to be safer) to make sure to get as most data within a single call?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub https://github.com/qdev-dk/Qcodes/pull/74/files/835801f957648eaf27c0f84c3e6c4b1f14f507eb#r60217008 https://github.com/notifications/beacon/AKEAqaJyERqrriwzHRYBjmvouLYffnFpks5p5MJxgaJpZM4IJQCO.gif

[alexcjohnson]

There is no guarantee that a single recv will fill the buffer with everything that was sent. This is why I’ve always used a header that tells me how many bytes are coming, so I know if I’ve gotten the whole (user level) message.

Ah, that's what I was afraid of... unfortunately we don't control these senders and they don't appear to use headers like that, so it seems the best we can hope for is that they define a termination character/string and we can read until we receive that. And we hope nothing comes after it, or we have to store that in our own buffer to be prepended to the next read... though I can't see why that would happen, instruments should basically only be sending data that we ask for

Anyway, it's obviously bit of a mess, that presumably the visa driver already has worked out; shouldn't be too hard to do ourselves too but it won't be made robust just by increasing the buffer size.

[damazter]

I have implemented a server for TCP IP communication for the triton fridges that doesn't check for this. It just uses .recv(####) without looking for termination characters.
I have never seen any error with this.
This has also been stress tested with loads of connections, still no error. So although there is no guarantee, I think problems will not arise

[dbwz8]

As long as your messages are fairly short, they shouldn't be split. If you're on a local network it should really never happen. It's just not guaranteed according to the spec. Most TCP drivers are implemented to send complete messages if possible (more efficient to never manipulate the data).

Sent from my Windows Phone

---

From: damaztermailto:notifications@github.com
Sent: ‎4/‎22/‎2016 7:45 AM
To: qdev-dk/Qcodesmailto:Qcodes@noreply.github.com
Cc: Dave Weckermailto:dbwz8@hotmail.com; Mentionmailto:mention@noreply.github.com
Subject: Re: [qdev-dk/Qcodes] Instrument driver adidtions (#74)

@@ -31,6 +31,7 @@ def init(self, name, address=None, port=None, timeout=5,
self._confirmation = write_confirmation

     self._ensure_connection = EnsureConnection(self)

•    self._buffer = 1024
  

I have implemented a server for TCP IP communication for the triton fridges that doesn't check for this. It just uses .recv(####) without looking for termination characters.
I have never seen any error with this.
This has also been stress tested with loads of connections, still not error. So although there is no guarantee, I think problems will not arise

---

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/qdev-dk/Qcodes/pull/74/files/835801f957648eaf27c0f84c3e6c4b1f14f507eb#r60660376

[alexcjohnson]

Does this mean that if you change resolution you also change NPLC? If so (and certainly for resolution vs integration_time) then snapshot will be out of date for one after you set the other. I don't know a clean solution to this... maybe overriding the separate parameter._save_val methods after instantiation, but I'll think a bit about it. It's going to come up again so it's worth thinking of a solid solution.

[MerlinSmiles]

Yes exactly, this part is a total mess in the instrument!
resolution NPLC and integration time, they all might change with eachother, but they dont have to, increasing resolution increases integration time, but the other way around it might not be the same.
I cannot convert back and forth between those values in the driver, that would be a huge mess. What about giving the parameter an snapshot_update=True ? do you create the snapshots often so that it would mess up stuff when asking the instrument for that info?

[alexcjohnson]

Ah, I see... yes, that's annoying.

the intention is for snapshots to not ever call out to the hardware unless the user explicitly asks for it, but for the monitor to query the hardware periodically, thereby updating each of the last saved parameter values from time to time.

resolution and integration_time map to the same hardware command, so they of course have a 1:1 mapping. But it sounds like NPLC doesn't have such a mapping. In this case anyway, this parameter is never going to be part of a hot loop, right? You're going to change it once in a long while so you won't care much about how long the call takes? In that case, I think I'd like to follow each set on one of them with a get on the other two, then we never need to mess with the mechanics of how they save their values (or assume a relationship between them which may not hold!)

I'm thinking the nicest way to do this will be an event system, so each parameter emits a set and a get event, and you can do something like:

def update_res_and_time(self):
    self.resolution.get()
    self.integration_time.get()
    self.NPLC.get()

self.on(('resolution_set', 'integration_time_set', 'NPLC_set'), self.update_res_and_time)

But I haven't used any event systems in Python... anyone? I see pyee, gevent-eventemitter, eventemitter...

[MerlinSmiles]

Yes, you're right, this is nothing that will change often
That event-stuff would make it easy :)

[alexcjohnson]

I pushed this to #92 so we can discuss more before implementing this. So lets ignore this problem as far as this PR is concerned and let that issue take over.

[eendebakpt]

How different is are the Keithley 2400 and 2600 from the 2700? Maybe it would make sense to compare the 2700 driver (#69) to the other two and make the interfaces similar.

[MerlinSmiles]

@eendebakpt the 2600 has very different commands for interacting with it, it does not look like SCPI anymore, furthermore it has 2 channels. For the 2400 it might be closer, but I don't have the unit here anymore to test, that is why it has no functionality in it.
I agree on the similar interfaces and was about to open a general issue on that, which I will do in a bit.

[alexcjohnson]

closing this to make a new PR from this branch into master (note that this one is asking to merge into inst-process even though that was merged long ago) so we can see what's really here