python-ivi / python-ivi

A Python implementation of the Interchangeable Virtual Instrument standard.
MIT License
214 stars 121 forks source link

Need more general way of formatting/parsing booleans for writtten commands. #20

Closed sacherjj closed 9 years ago

sacherjj commented 9 years ago

I'm working on the Rigol DP832 and finding all kinds of issues with any command that is essentially a boolean output or query.

Ex: _set_output_ovp_enabled / _get_output_ovp_enabled

The method hard codes an int(bool(value)). Rigol wants 'on' or 'off'. I'm not very familiar with the architecture yet, so I'm not sure what is the best option here.

With no changes to anything outside of rigol related files, it looks like the common mode is to copy the methods from dcpwr.py and fix what is different. However, the only difference is bool handling. (Both for read and write).

To fully support the rigol boolean methods, it looks like I need to redefine all of those in rigolBaseDCPwr.py.

It seems like there is a better way than this, but I need someone understanding the architecture better to validate the idea. This also may be valid to use in other non-power supply objects.

  1. Define a method self._get_bool_str(value) that returns the representative boolean string for an object. This is defined as the most default at the lowest object (which looks to be '0' and '1')
  2. Modify get and set boolean methods to use new method.
  3. Redefine the and redefined at manufacturer level as needed to change functionality from '0'/'1'

Some examples

# set method
# OLD
value = bool(value)
...
self._write("output %d" % int(value))
# NEW
# value = bool(value) is gone and moved into _get_bool_str
self._write("output %s" % self._get_bool_str(value))

# get method from _get_output_enabled
# OLD
self._output_enabled[index] = bool(int(self._ask("output?")))
# NEW
self._output_enabled[index] = self._ask("output?").lower() == self._get_bool_str(True)

If every other device is 0/1, then it might not make sense. Although I worry about updating the core functions without knowing that the rigol specific functions need changed as well.

alexforencich commented 9 years ago

This is a known issue. I don't have a rigol supply to debug on so I have not fixed it myself. There were some changes submitted (implementing the correct commands in rigolBaseDCPwr.py) not so long ago, but I do not think all of the commands were fixed.

Your idea is a good one, though. All of the 'standard' power supply commands are in ivi/scpi/dcpwr.py. Might as well start with modifying that one and then if a helper function proves to be useful enough, it could be moved into ivi.py.

Also, keep the value = bool(value). This is needed to make sure the cached value is also a boolean type and not something else if the caller passed in something strange.

sacherjj commented 9 years ago

You would keep value = bool(value), but move that into the method for generating the bool string. Unless the raw boolean is needed in the function.

I'll see what I can do to work on these changes.

alexforencich commented 9 years ago

Take a look at the code in ivi/scpi/dcpwr.py. This is the _set_output_enabled method:

def _set_output_enabled(self, index, value):
    index = ivi.get_index(self._output_name, index)
    value = bool(value)
    if not self._driver_operation_simulate:
        if self._output_count > 1:
            self._write("instrument:nselect %d" % (index+1))
        self._write("output %d" % int(value))
    self._output_enabled[index] = value
    for k in range(self._output_count):
        self._set_cache_valid(valid=False,index=k)
    self._set_cache_valid(index=index)

The value that is passed in is cast to boolean and then used both in the write call and saved in a class variable that acts as cache. If the get method is called, it will return the cached value from this variable instead of reading it from the instrument. (Of course, the cache can be disabled if need be)