parameter.cache() will change an invalid cache to valid, even for parameters with get_cmd=None

[dweigand]

Steps to reproduce

p = Parameter("name", set_cmd=None)
assert not p.cache.valid
p.cache()
assert not p.cache.valid

p = Parameter("name", set_cmd=None)
p.set(5)
assert p.cache.valid
p.cache.invalidate()
assert not p.cache.valid
p.cache()
assert not p.cache.valid

the last assert will fail in both cases

Expected behaviour

As we did not implement a get_cmd, parameter.cache() always returns the last cached value,even though it is not valid (this is fine).
Querying the cache should leave it marked as invalid, unless we get information about the parameter from another place, i.e. unless a get_cmd is implemented.

Actual behaviour

parameter.cache() will always update the cache, switching an invalid cache to valid. See the red path in the flow diagram:

Comment

It is possible that using both cache.invalidate() and get_cmd=None together is not intended and considered user error. In this case, this should be documented.

System

windows
qcodes 0.33.0

[jenshnielsen]

Hmn, this is tricky. In my opinion get_cmd=None means that that the parameter is fully defined by its cache so I would probably lean towards that the error is that it is possible to ever get a parameter with get_cmd=None into a state where the cache is invalid. Do you have a specific usecase for using get_cmd=None.

I would probably suggest:

1. get_cmd=None can only be used together with set_cmd=None -> ManualParameter. Otherwise warn and eventually an error
2. get_cmd=None + set_cmd=None the cache is always valid and invalidate raises

[dweigand]

We stumbled upon this because we had an instrument driver where one parameter cannot be queried from the instrument. The person that wrote the parameter therefore only specified the set_cmd (an thus used the default get_cmd=None). This then led to errors downstream where the cache was valid, but no longer in sync with the actual instrument state. We will use get_cmd=False from now on, that is more explicit anyway.

1. good idea. I propose that the docstring should also be updated so that developers do not repeat our mistake
2. I tend to agree, but this might be tricky. cache.invalidate() and the cache itself know nothing about get_cmd and set_cmd.
3. It might be good to change the default to get_cmd=False, in line with set_cmd. That would prevent mistakes made by using the default settings.