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.

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

bug: decadac getting and setting voltages fails unpredictably #546

Closed
nataliejpg opened this issue Mar 28, 2017 · 7 comments · Fixed by #552
Closed

bug: decadac getting and setting voltages fails unpredictably #546

nataliejpg opened this issue Mar 28, 2017 · 7 comments · Fixed by #552

Comments

@nataliejpg
Copy link
Contributor

nataliejpg commented Mar 28, 2017

Steps to reproduce

  1. start Harvard Decadac and import a qcodes instrument instrument:
from qcodes.instrument_drivers.Harvard.Decadac import Decadac
sl0 = Decadac('dec_slot_0', port=5, slot=0)
  1. run

sl0.ch1_voltage(0)

  1. fail as below, seems like its something to do with the mode (which is set in the init but somehow that isn't reliable), It is also not super reliable to fix by setting the mode to 1 but seems to work if I set it to 0 and back to 1 a few times...

Expected behaviour

should set the voltage

Actual behaviour

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-18-e543db036233> in <module>()
----> 1 sl0.ch1_voltage(0)

a:\qcodes\qcodes\instrument\parameter.py in __call__(self, *args)
    133         else:
    134             if self.has_set:
--> 135                 self.set(*args)
    136             else:
    137                 raise NotImplementedError('no set cmd found in' +

a:\qcodes\qcodes\instrument\parameter.py in _validate_and_sweep(self, value)
    945             e.args = e.args + (
    946                 'setting {} to {}'.format(self.full_name, repr(value)),)
--> 947             raise e
    948 
    949     def set_step(self, step, max_val_age=None):

a:\qcodes\qcodes\instrument\parameter.py in _validate_and_sweep(self, value)
    929             step_clock = time.perf_counter()
    930 
--> 931             for step_val in self._sweep_steps(value):
    932                 self._set(step_val)
    933                 self._save_val(step_val)

a:\qcodes\qcodes\instrument\parameter.py in _sweep_steps(self, value)
    891         state = self._latest()
    892         if state['ts'] is None or state['ts'] < oldest_ok_val:
--> 893             start_value = self.get()
    894         else:
    895             start_value = state['value']

a:\qcodes\qcodes\instrument\parameter.py in get(self)
    826         except Exception as e:
    827             e.args = e.args + ('getting {}'.format(self.full_name),)
--> 828             raise e
    829 
    830     def _valmapping_get_parser(self, val):

a:\qcodes\qcodes\instrument\parameter.py in get(self)
    821     def get(self):
    822         try:
--> 823             value = self._get()
    824             self._save_val(value)
    825             return value

a:\qcodes\qcodes\utils\command.py in __call__(self, *args)
    175             raise TypeError(
    176                 'command takes exactly {} args'.format(self.arg_count))
--> 177         return self.exec_function(*args)

a:\qcodes\qcodes\instrument_drivers\Harvard\Decadac.py in _getvoltage(self, channel)
    129         response = temp[::-1]
    130 
--> 131         rawvoltage = self._code2voltage(response, channel)
    132         actualvoltage = rawvoltage - self._offsets[channel]
    133 

a:\qcodes\qcodes\instrument_drivers\Harvard\Decadac.py in _code2voltage(self, code, channel)
    232         """
    233 
--> 234         code = float(code)
    235         translationdict = {1: lambda x: (x+1)*20/2**16-10,
    236                            2: lambda x: (x+1)*10/2**16,

ValueError: ("could not convert string to float: '0! M 0'", 'getting dec_slot_0_ch1_voltage', 'setting dec_slot_0_ch1_voltage to 0')

@WilliamHPNielsen @giulioungaretti

@MerlinSmiles
Copy link
Contributor

@nataliejpg that string you get looks like a response from the decadac upon setting the mode.
Looking into the driver, no response is read when setting the mode.

You could try setting the mode, and then calling
dac.visa_handle.read()
maybe a few times...

@nataliejpg
Copy link
Contributor Author

thanks @MerlinSmiles , I'll put that in the my initialisation wrapper and see if it helps, perhaps a fix in the driver where it does that after setting the mode (as part of set_cmd) would help. Also I found another bug which is that if you initialise the instrument and then ask what the voltages are they do not correspond to what you last set them to which is maybe because the get isn't actually querying the instrument (I don't know) but at least a warning on this would be good, do you think this is the case @WilliamHPNielsen? I'm right now in the middle of measuring so am disinclined to disconnect and check what they are actually outputting!

@nataliejpg
Copy link
Contributor Author

ok tried that and answer is to run visa_handle.read() twice (but no more because it times out if you try more times than it has messages)

@nataliejpg
Copy link
Contributor Author

actually the above is only true sometimes. Also I now cannot get the instrument to output anything even with turning mode to 1 a lot of times. @MerlinSmiles are you (or anyone else?) actually using this instrument and driver reliably?

@MerlinSmiles
Copy link
Contributor

MerlinSmiles commented Mar 29, 2017

@nataliejpg yes it will time out, thats because visa will read until it finds a line termination character.
If that reading is helping you, it should be included in that set_mode command, but it would be strange if you'd have to call it more than once, but at every time that mode is set...

For your other bug, the get is asking the instrument, just in a funny way of handling the response (with things like inverting strings, and searching from the back 😮) , which seems to be a double response.
when I compare that to our matlab driver, we select the channel, but that also reads the instrument response. Basically every command also generates a response, which one needs to read, otherwise it will mess up your next reading.

For your last question, No, I dont have that instrument installed, but spent quite some time on our 21bit resolution matlab driver a few years ago. Which worked just fine.
https://github.com/qdev-dk/matlab-qd/tree/master/%2Bqd/%2Bins
look especially in the DecaDACChannel.m and HRDecaDACChannel.m

@MerlinSmiles
Copy link
Contributor

@WilliamHPNielsen this driver is putting the decadac in mode 1, which is a high resolution mode, and effectively disables output on channels 3,4,7,8,... aaaand it is not handling that extra resolution?
There is a manual on the qdev wiki, and those matlab drivers for copying functionality...

WilliamHPNielsen added a commit to WilliamHPNielsen/Qcodes that referenced this issue Mar 30, 2017
Unfortunately not tested.

Should fix microsoft#546
@WilliamHPNielsen
Copy link
Contributor

Sorry for that shitshow. Unfortunately, I don't have a Decadac at hand, but from @MerlinSmiles' helpful comments, I hope that #552 does the trick.

spauka added a commit to spauka/Qcodes that referenced this issue Apr 11, 2017
Commits a channelized version of the Harvard DecaDAC driver
using the new channelization implementation. Note that this driver
at the present time also fixes a number of open bugs in the previous
driver, including occasional crashes (microsoft#546), incorrectly reading
DAC voltages (microsoft#563), reset (microsoft#478).

This commit also adds a number of new features to the driver
including:
 - Feature and version detection
 - Access to calibrated high-resolution mode of DAC
 - Parameter read from memory
 - Validation of writes

Closes: microsoft#478, microsoft#543, microsoft#563
spauka added a commit to spauka/Qcodes that referenced this issue Apr 11, 2017
Commits a channelized version of the Harvard DecaDAC driver
using the new channelization implementation. Note that this driver
at the present time also fixes a number of open bugs in the previous
driver, including occasional crashes (microsoft#546), incorrectly reading
DAC voltages (microsoft#563), reset (microsoft#478).

This commit also adds a number of new features to the driver
including:
- Feature and version detection
- Access to calibrated high-resolution mode of DAC
- Parameter read from memory
- Validation of writes
giulioungaretti pushed a commit that referenced this issue May 8, 2017
* fix: Respond to issue #546

Unfortunately not tested.

Should fix #546

* fix: Remove fine mode

Remove the high res mode
jenshnielsen added a commit that referenced this issue Jun 16, 2017
* Driver/Keysight DMM (#556)

* feat: Add a Keysight_34465A driver

Add a first file for a driver for the Keysight 34465A DMM and a
benchmarking notebook

* Add trigger parameters to driver

Add four trigger parameters to the driver.

* refactor: Remove 'READ?' as volt get_cmd

Make a more general get_cmd for the volt parameter. Using simply 'READ?' is wrong in many cases.

* feat: Add sampling parameters

Add four parameters for sampling.

* fix: Fix wrong volt parameter get_cmd

* WIP: Add Array parameter

* feat: Add functioning data buffer parameter

* Move notebook

* Respond to review

* feat: Add channelization

Initial commit of two new classes to allow for channelization of
instruments. This includes ChannelList, a container for channels
in an instrument, and InstrumentChannel which implements a base
class for an instance of a channel.

* feat: Add channelized Lakeshore Model336 Driver

Add a driver for the Lakeshore Model 336 temperature controller
using new channelization implementation.

* feat: Add a channelized driver for the Harvard DAC

Commits a channelized version of the Harvard DecaDAC driver
using the new channelization implementation. Note that this driver
at the present time also fixes a number of open bugs in the previous
driver, including occasional crashes (#546), incorrectly reading
DAC voltages (#563), reset (#478).

This commit also adds a number of new features to the driver
including:
- Feature and version detection
- Access to calibrated high-resolution mode of DAC
- Parameter read from memory
- Validation of writes

* fix: bug in slicing and adding code

Fix bug in slicing/adding code, not correctly passing along
channel list name.

* style: fix formatting and make codacy fixes

* style: fix more code style issues

- Removed useless methods
- Whitespace

* fix: flush is now handled correctly in instrument base class

* feat: Allow channellists to be excluded from snapshots

Allows certain channel lists to be excluded from snapshots. This is
helpful when, for example, there are multiple ways to access the same
channel.

* fix: Allow snapshots of ChannelLists

This feature is implemented by creating the idea of a submodule in the
instrument class. This is just a class which implements the Instrument
class, and may be a channel list or may just implement a logical
collection of parameters within a parent instrument.

For example, looking at the Lakeshore Model_336 driver, each channel can
either be accessed as a member of a channel list, or by using the name
of the channel:
```python3
therm = Model_336("therm", "...")
# These two accesses are equivalent
therm.A.temperature()
therm.channels[0].temperature()
```

* style: remove force arg in snapshot_base

Should think about the best way to implement this change.

* style: remove unessecary import

* fix: error in type check.

* fix: submodules should be Metadatable

Submodule objects must be Metadatables at minimum, instrument is too
restrictive for channels.

* Start adding tests for channels

* Extend tests

* Fix: ensure consistent dataset names regardless of chan[] or chanA reading

* More work on channels testing

* Make sure that instument is closed in tests

* Depend on hypothesis for test

* fix improve testing of channels

* test more:

* fix: channels pep8

* fix: ensure that channels can be extended from generator

* More tests of adding channels

* Add missing functions and parameters to channellist

* fix: function testS

* assert to unittest style

* Revert "fix: function testS"

This reverts commit ff8a316.

* Revert "Add missing functions and parameters to channellist"

This reverts commit 347c0b2.

* Remove docs of non existing attributes

* Docs: decadac quote *IDN to remove warning

* Add some channel notebooks

* fix: test_channels small tweeks

* Add support to array parameters in channels

And error clearly if you try to measure multiple multiparameters

* Mock parameters add instruments to fix names in tests

* refactor tests to reduce code duplication

* Revert "Add some channel notebooks"

This reverts commit 2dbacc0.

* feat: Add channelised QDac driver

Add a QDac driver using the new channelisation feature

* feat: Add a set method to MultiChannelInstrumentParameter

* docs: Add a driver example for the channelised Qdac

* feat: Make ChannelList take default paramclass

Make the paramclass returned by __getattr__ be customisable.

* feat: Add a channelised QDac driver

Add a channelised driver for the QDac. This requires changing the base code
for channels a bit to allow for custom MultiChannelInstrumentParameter
subclasses.

* docs: Add notebook for QDac w. channels

* style: Remove unnecessary comment

* Fix syntax/spelling errors in DAC driver

* Fix: qdac_channels remove magic number

* Doc: clarify doc string

* Fix: rename paramclass to be more specific

* Lakeshore tab to space

* pep8 lakeshore driver

* Pep8 decadac driver

* Fix warnings in qdac driver

* Annotate parameters that linter has problems with
Dominik-Vogel pushed a commit to Dominik-Vogel/Qcodes that referenced this issue Aug 9, 2017
* Driver/Keysight DMM (microsoft#556)

* feat: Add a Keysight_34465A driver

Add a first file for a driver for the Keysight 34465A DMM and a
benchmarking notebook

* Add trigger parameters to driver

Add four trigger parameters to the driver.

* refactor: Remove 'READ?' as volt get_cmd

Make a more general get_cmd for the volt parameter. Using simply 'READ?' is wrong in many cases.

* feat: Add sampling parameters

Add four parameters for sampling.

* fix: Fix wrong volt parameter get_cmd

* WIP: Add Array parameter

* feat: Add functioning data buffer parameter

* Move notebook

* Respond to review

* feat: Add channelization

Initial commit of two new classes to allow for channelization of
instruments. This includes ChannelList, a container for channels
in an instrument, and InstrumentChannel which implements a base
class for an instance of a channel.

* feat: Add channelized Lakeshore Model336 Driver

Add a driver for the Lakeshore Model 336 temperature controller
using new channelization implementation.

* feat: Add a channelized driver for the Harvard DAC

Commits a channelized version of the Harvard DecaDAC driver
using the new channelization implementation. Note that this driver
at the present time also fixes a number of open bugs in the previous
driver, including occasional crashes (microsoft#546), incorrectly reading
DAC voltages (microsoft#563), reset (microsoft#478).

This commit also adds a number of new features to the driver
including:
- Feature and version detection
- Access to calibrated high-resolution mode of DAC
- Parameter read from memory
- Validation of writes

* fix: bug in slicing and adding code

Fix bug in slicing/adding code, not correctly passing along
channel list name.

* style: fix formatting and make codacy fixes

* style: fix more code style issues

- Removed useless methods
- Whitespace

* fix: flush is now handled correctly in instrument base class

* feat: Allow channellists to be excluded from snapshots

Allows certain channel lists to be excluded from snapshots. This is
helpful when, for example, there are multiple ways to access the same
channel.

* fix: Allow snapshots of ChannelLists

This feature is implemented by creating the idea of a submodule in the
instrument class. This is just a class which implements the Instrument
class, and may be a channel list or may just implement a logical
collection of parameters within a parent instrument.

For example, looking at the Lakeshore Model_336 driver, each channel can
either be accessed as a member of a channel list, or by using the name
of the channel:
```python3
therm = Model_336("therm", "...")
# These two accesses are equivalent
therm.A.temperature()
therm.channels[0].temperature()
```

* style: remove force arg in snapshot_base

Should think about the best way to implement this change.

* style: remove unessecary import

* fix: error in type check.

* fix: submodules should be Metadatable

Submodule objects must be Metadatables at minimum, instrument is too
restrictive for channels.

* Start adding tests for channels

* Extend tests

* Fix: ensure consistent dataset names regardless of chan[] or chanA reading

* More work on channels testing

* Make sure that instument is closed in tests

* Depend on hypothesis for test

* fix improve testing of channels

* test more:

* fix: channels pep8

* fix: ensure that channels can be extended from generator

* More tests of adding channels

* Add missing functions and parameters to channellist

* fix: function testS

* assert to unittest style

* Revert "fix: function testS"

This reverts commit ff8a316.

* Revert "Add missing functions and parameters to channellist"

This reverts commit 347c0b2.

* Remove docs of non existing attributes

* Docs: decadac quote *IDN to remove warning

* Add some channel notebooks

* fix: test_channels small tweeks

* Add support to array parameters in channels

And error clearly if you try to measure multiple multiparameters

* Mock parameters add instruments to fix names in tests

* refactor tests to reduce code duplication

* Revert "Add some channel notebooks"

This reverts commit 2dbacc0.

* feat: Add channelised QDac driver

Add a QDac driver using the new channelisation feature

* feat: Add a set method to MultiChannelInstrumentParameter

* docs: Add a driver example for the channelised Qdac

* feat: Make ChannelList take default paramclass

Make the paramclass returned by __getattr__ be customisable.

* feat: Add a channelised QDac driver

Add a channelised driver for the QDac. This requires changing the base code
for channels a bit to allow for custom MultiChannelInstrumentParameter
subclasses.

* docs: Add notebook for QDac w. channels

* style: Remove unnecessary comment

* Fix syntax/spelling errors in DAC driver

* Fix: qdac_channels remove magic number

* Doc: clarify doc string

* Fix: rename paramclass to be more specific

* Lakeshore tab to space

* pep8 lakeshore driver

* Pep8 decadac driver

* Fix warnings in qdac driver

* Annotate parameters that linter has problems with
peendebak pushed a commit to VandersypenQutech/Qcodes that referenced this issue Aug 11, 2017
* Driver/Keysight DMM (microsoft#556)

* feat: Add a Keysight_34465A driver

Add a first file for a driver for the Keysight 34465A DMM and a
benchmarking notebook

* Add trigger parameters to driver

Add four trigger parameters to the driver.

* refactor: Remove 'READ?' as volt get_cmd

Make a more general get_cmd for the volt parameter. Using simply 'READ?' is wrong in many cases.

* feat: Add sampling parameters

Add four parameters for sampling.

* fix: Fix wrong volt parameter get_cmd

* WIP: Add Array parameter

* feat: Add functioning data buffer parameter

* Move notebook

* Respond to review

* feat: Add channelization

Initial commit of two new classes to allow for channelization of
instruments. This includes ChannelList, a container for channels
in an instrument, and InstrumentChannel which implements a base
class for an instance of a channel.

* feat: Add channelized Lakeshore Model336 Driver

Add a driver for the Lakeshore Model 336 temperature controller
using new channelization implementation.

* feat: Add a channelized driver for the Harvard DAC

Commits a channelized version of the Harvard DecaDAC driver
using the new channelization implementation. Note that this driver
at the present time also fixes a number of open bugs in the previous
driver, including occasional crashes (microsoft#546), incorrectly reading
DAC voltages (microsoft#563), reset (microsoft#478).

This commit also adds a number of new features to the driver
including:
- Feature and version detection
- Access to calibrated high-resolution mode of DAC
- Parameter read from memory
- Validation of writes

* fix: bug in slicing and adding code

Fix bug in slicing/adding code, not correctly passing along
channel list name.

* style: fix formatting and make codacy fixes

* style: fix more code style issues

- Removed useless methods
- Whitespace

* fix: flush is now handled correctly in instrument base class

* feat: Allow channellists to be excluded from snapshots

Allows certain channel lists to be excluded from snapshots. This is
helpful when, for example, there are multiple ways to access the same
channel.

* fix: Allow snapshots of ChannelLists

This feature is implemented by creating the idea of a submodule in the
instrument class. This is just a class which implements the Instrument
class, and may be a channel list or may just implement a logical
collection of parameters within a parent instrument.

For example, looking at the Lakeshore Model_336 driver, each channel can
either be accessed as a member of a channel list, or by using the name
of the channel:
```python3
therm = Model_336("therm", "...")
# These two accesses are equivalent
therm.A.temperature()
therm.channels[0].temperature()
```

* style: remove force arg in snapshot_base

Should think about the best way to implement this change.

* style: remove unessecary import

* fix: error in type check.

* fix: submodules should be Metadatable

Submodule objects must be Metadatables at minimum, instrument is too
restrictive for channels.

* Start adding tests for channels

* Extend tests

* Fix: ensure consistent dataset names regardless of chan[] or chanA reading

* More work on channels testing

* Make sure that instument is closed in tests

* Depend on hypothesis for test

* fix improve testing of channels

* test more:

* fix: channels pep8

* fix: ensure that channels can be extended from generator

* More tests of adding channels

* Add missing functions and parameters to channellist

* fix: function testS

* assert to unittest style

* Revert "fix: function testS"

This reverts commit ff8a316.

* Revert "Add missing functions and parameters to channellist"

This reverts commit 347c0b2.

* Remove docs of non existing attributes

* Docs: decadac quote *IDN to remove warning

* Add some channel notebooks

* fix: test_channels small tweeks

* Add support to array parameters in channels

And error clearly if you try to measure multiple multiparameters

* Mock parameters add instruments to fix names in tests

* refactor tests to reduce code duplication

* Revert "Add some channel notebooks"

This reverts commit 2dbacc0.

* feat: Add channelised QDac driver

Add a QDac driver using the new channelisation feature

* feat: Add a set method to MultiChannelInstrumentParameter

* docs: Add a driver example for the channelised Qdac

* feat: Make ChannelList take default paramclass

Make the paramclass returned by __getattr__ be customisable.

* feat: Add a channelised QDac driver

Add a channelised driver for the QDac. This requires changing the base code
for channels a bit to allow for custom MultiChannelInstrumentParameter
subclasses.

* docs: Add notebook for QDac w. channels

* style: Remove unnecessary comment

* Fix syntax/spelling errors in DAC driver

* Fix: qdac_channels remove magic number

* Doc: clarify doc string

* Fix: rename paramclass to be more specific

* Lakeshore tab to space

* pep8 lakeshore driver

* Pep8 decadac driver

* Fix warnings in qdac driver

* Annotate parameters that linter has problems with
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants