Channel of Instrument does not use overwritten Instrument methods

[dkriegner]

I am implementing another device driver based on pymeasure's Instrument and run into an issue.
The device uses messages based on binary code for communication. I have overwritten Instrument.read/write/values to work with binary messages. This all works great. For a set of properties, it seemed useful to use the Channel framework.

Using Channels, however, seems to not use all the overwritten methods. In particular my custom values method is not used.
I have prepared a minimal working example where the test framework nicely illustrates the problem.

The Instrument code and test file can be found at
https://gist.github.com/dkriegner/7af08dc78fdd1461125e079670675bcf

The test_prop succeeds as expected, while testing the same property via a channel fails because CommonBase.values seems to be used (the string conversion causes the problem).

    def test_prop_channel():
        """Verify the processing the load capacity property via Channel"""
        with expected_protocol(
            DKTest,
            [(b'A', b'\x01'), ],
        ) as inst:
>           assert inst.ch_1.prop == 1
E           assert "b'\\x01'" == 1

Is it wrong to expect that with Channels I can still overwrite the values method?

My WIP code for the real device can be found in my fork, but its not ready and lots of other things issue the problem there.

[BenediktBurger]

Channels and Instrument both use values of their commen superclass CommonBase.

Normally all communication problems can be handled in read/write, such that it is not necessary to overload values.

Why did you change values at all? What do you try to achieve?

[dkriegner]

What I am trying to achieve is that values does not perform string conversion and split. The string conversion could likely be undone by using encode where needed, but I do not see an option to avoid the split.

My instrument is sending numbers (integers) binary encoded. So for certain values the split-function in CommonBase will cause troubles because I can not select a separator which will avoid producing an unwanted split in all cases.

For example communication messages see the test file

I do not see how this works without overloading values?

read/write I also need to overwrite to deal with acknowledgement messages and header/checksum issues.

[BenediktBurger]

Thanks for providing the example communication (the tests are great to have).

I have to look at the code and think how to solve it best. I don't know, whether you can deactivate the split method.
Maybe giving the first char (which seems to be always the same one) as the split char and then use maxsplit=1 (not yet implemented). 🤔

[BenediktBurger]

I just looked at the instrument code and it seems very reasonable.
You can do a lot more in the read and write methods.
Maybe the hcp ovens (tc38 and TC38D) can give you some ideas.

I agree that values is not written for bytes communication.

[msmttchr]

You could also add the following method to your PresetChannel definition:

def values(self, command, cast=int, preprocess_reply=None):
        return self.parent.values(command, cast, preprocess_reply)

This is possibly an indication that values method should be redirected to parent implementation in Channel like other methods. Indeed, it is expected that values is overridden in Instrument as this implementation demonstrate.

[BenediktBurger]

If values redirects to the instrument one, we bypass the channel write method.

You can "import" values from instrument as you @msmttchr suggest, but you cannot call it directly.

[msmttchr]

Yes, you are right write would be bypassed. I think it should be possible to split values behaviour in a command part and a formatting part.

In this specific case, you would have:

In class CXN (I omitted the docstring for brevity)

def values(self, command, cast=int, preprocess_reply=None):
    """ .... """
    results = self.ask(command)
    return self._format_reply(results, cast, preprocess_reply)

def _format_reply(self, results, cast=int, preprocess_reply=None)
    if callable(preprocess_reply):
        results = preprocess_reply(results)
    for i, result in enumerate(results):
        try:
            if cast == bool:
                # Need to cast to float first since results are usually
                # strings and bool of a non-empty string is always True
                results[i] = bool(float(result))
            else:
                results[i] = cast(result)
        except Exception:
            pass  # Keep as bytes
    return results

In PresetChannel, you would have:

def values(self, command, cast=int, preprocess_reply=None):
    results = self.ask(command)
    return self.parent._format_reply(results, cast, preprocess_reply)

If this works, a similar split can be implemented in the default values implementation with the rationale that only the formatting part requires customization.

[BenediktBurger]

I like that idea.

[dkriegner]

Thanks for all the feedback.

I tested all suggestions. The first one of @msmttchr indeed would bypass Instrument.write and is therefore not helping.
The second one seems to be a good solution for my problem. I can also implement it in my instrument locally if you do not want to put it into the core. But it seems to me that overwriting this formatting logic in values should be possible and useful for more than one device (although the current implementation leads to a problems only if the device also uses channels).

Regarding TC38D: thanks for this hint: I looked into it, but it seems to me I can not do this easily. My reply messages do not identify themself to which write command they correspond. So inside read I can not know which type of message and what value I would want to extract. For this pattern to work for me keyword arguments from the measurement/control properties would need to make it until read so that I can specify what to extract. But this seems to make all the preprocess_reply / get_process functions duplicated/replaced.

I think the solution suggested above is what I was looking for. I updated the new device branch on my fork and the minimal example with a working version.
In both these codes the values method could be removed if the splitting suggested above is put into CommonBase.

For me all tests of the package are passing if the split method is put into CommonBase.

[BenediktBurger]

@dkriegner could you try the following, please:

def values(self, parameters):
    # here is all the values stuff: ask and the content of _format_values

class SomeChannel(Channel):
    values = values

class CXN(Instrument):
    values = values

I think, that you could have a single function for both objects. It is important, that you assign them at class level (as here) and that the function contains self as a parameter

@bilderbuchi, could you please provide your opinion:
The question is, whether to split values into the ask part (individual one for Channel and Instrument) and an extract_values / interpret_values part, which can be shared among Channel and Instrument. The reason is. This bytes communication is best implemented with some changes to values.

For an example see #800

I don't like it, as values will be just two calls: ask and interpret_values, however I don't see a better way, except the idea sketched above.

[dkriegner]

@dkriegner could you try the following, please:

def values(self, parameters):
    # here is all the values stuff: ask and the content of _format_values

class SomeChannel(Channel):
    values = values

class CXN(Instrument):
    values = values

This pattern also works. I tested in in the minimal example as well as in the code of the device in #800 (not commited yet). I guess this last suggestion is a bit cleaner if you look for a solution for the device in #800 only, but it should be somewhere documented how values has to be overwritten in connection with Channels.

[BenediktBurger]

Thanks for testing. @dkriegner

@bilderbuchi another idea: As ask returns (as default) a str, we could get rid of the str cast in str(self.ask(command)).strip() in values, which would aid the change to a binary capable values method.
We can also move that strip to the pre_process_reply. It should not be necessary in most instruments anyway:

    def values(self, command, separator=',', cast=float, preprocess_reply=lambda s: s.strip(), maxsplit=-1):
        """Write a command to the instrument and return a list of formatted
        values from the result.

        :param command: SCPI command to be sent to the instrument
        :param separator: A separator character to split the string into a list
        :param cast: A type to cast the result
        :param preprocess_reply: optional callable used to preprocess values
            received from the instrument. The callable returns the processed
            string. Set to `None` for deactivation.
        :param maxsplit: At most `maxsplit` splits are done. -1 (default) indicates no limit.
        :returns: A list of the desired type, or strings where the casting fails
        """
        results = self.ask(command)
        if callable(preprocess_reply):
            results = preprocess_reply(results)
        results = results.split(separator, maxsplit=maxsplit)
        for i, result in enumerate(results):
            try:
                if cast == bool:
                    # Need to cast to float first since results are usually
                    # strings and bool of a non-empty string is always True
                    results[i] = bool(float(result))
                else:
                    results[i] = cast(result)
            except Exception:
                pass  # Keep as string
        return results

At least the first change (getting rid of str in str(self.ask) is feasable. Strip can work at bytes as well, so it should not raise any Exception.

[BenediktBurger]

I opened a pull request, getting rid of the str conversion (#803 ).
Together with the recently added option to limit the splitting to zero splits (maxsplit=0), your issue should be addressed, @dkriegner, as you do not have to reimplement values anymore, is it correct?

(N.B: you have to set the separator to some bytes object or to None)

[dkriegner]

I like the suggestion of improving values. But I think #803 does not fix my issue. If I have the equivalent of a whitespace (e.g. b'\x20') in my binary message then the split will remove it.

>>> b"\x13 test \x0d\x20".strip()
b'\x13 test'

I guess strip would need to be at least moved to the preprocess_reply as you suggest above. Then one can also get rid of it in an Instrument where this does not help. (But since overloading values is not recommended and no default preprocess_reply can be anymore define at the Adapter this would mean every single property (measurement/control) needs to define an noop preprocess_reply. This seems not elegant, but is no big issue for #800 were preprocess_reply is defined for almost all properties anyways)

[BenediktBurger]

I like the suggestion of improving values. But I think #803 does not fix my issue. If I have the equivalent of a whitespace (e.g. b'\x20') in my binary message then the split will remove it.

strip will only remove whitespaces at the end. I don't whether that happens in your case. If you talk about split, that is not a problem: if you have maxsplit=0, it won't do any split and won't remove any whitespace.

Moving strip (and possibly split) into preprocess_reply is a larger change, as some instruments might require the strip and use their own preprocess_reply.

This seems not elegant, but is no big issue for #800 were preprocess_reply is defined for almost all

You could make a values method, which has different default settings and calls afterwards the super.values:

def values(self, preprocess_reply=lambda v: v*5, ...):
    return super().values(preprocess_reply=preprocess_reply, ...)

For #800 I suggest, you use one of the previously mentioned methods (splitting values into two or having an "external" values method) and we continue the discussion in this issue until we find a good solution (I'd like to get feedback from other maintainers as well).

[dkriegner]

strip will only remove whitespaces at the end. I don't whether that happens in your case. If you talk about split, that is not a problem: if you have maxsplit=0, it won't do any split and won't remove any whitespace.

Moving strip (and possibly split) into preprocess_reply is a larger change, as some instruments might require the strip and use their own preprocess_reply.

Sorry I mistyped. Of course I meant strip will remove any whitespace character. In binary communication, one can certainly not exclude that the equivalent of a space (b"\x20") will be received since e.g. an integer value of 32 (which almost all parameters of the instrument in #800 can assume would trigger the problem). I understand it's a bigger change to move the strip, since its implications are difficult to find out. But I would guess it should not be so bad since the default cast (float) intrinsically performs a strip operation anyways.

Overall I think we could close this issue since the solutions suggested above provide good workarounds for the problems with overloading values in connection with Channels (which I guess is not so common in the codebase anyways)!

[BenediktBurger]

Thanks for your thoughts.

I looked at the code of the example and the tc038 and noticed, that we have the conversion to Str and back to numbers.
I guess byte wise communication needs often to reimplement values.

This instrument is a good candidate for #779, a list of solutions to different problems.

[bilderbuchi]

Thanks for the ping, I'll try to weigh in here over the Christmas break.

[bilderbuchi]

The way that values does both ask and formatting, while convenient (and thus frequently used), now turns out to having been unfortunate, as we notice a bad separation of concerns along the lines that @msmttchr has suggested (messaging and message processing, respectively).

Splitting out the formatting into a separate method that then becomes overridable/replaceable as needed seems to me a solid idea that would allow flexible use. I agree, though, that the now quite slim values method, and that it forwards most of its arguments to that formatting method, feels weird.

The approach with values=values in both channel and instrument of #800 has very limited "blast radius"/potential impact, so it's great that we have identified a workable workaround!

Moving the strip to the kwarg default will need quite some care in migrating, as this will potentially change behaviour for any instruments (ours or elsewhere) that have a preprocess_reply defined and turn out to need that strip (maybe with a custom cast arg?). Can we be sure that this won't cause problems?
This looks like the least attractive solution to me.

Yet another potential approach to improve the separation would be to add an argument to values to disable the "messaging" part, but I did not find an attractive way to do this, so the refactoring approach above seems preferable.

[bilderbuchi]

@bmoneke Concerning the potential sensitivity to whitespace-stripping of binary protocols, should we add a test that fails if the strip() in values were removed/disabled?

What about this:

• Document the problem and solution from add T&C Power Conversion RF power supply #800 in Add collection of instrument solutions to documentation #779, so that others can easily find this should it occur in another instrument.
• Open an enhancement issue regarding the refactoring of values into messaging and processing parts, including the what, why, context. This way we won't lose track of this in case it turns out that we want to do this in the future.
• If this problem re-occurs with some frequency, we can always do that refactoring.

[BenediktBurger]

Thanks @bilderbuchi.

I don't understand, what you try to achieve with that test.
I agree with your proposed course of action.

Regarding separation (to log an idea): we could rename values to "extract_values" (or similar) and remove the messaging part (a deprecated "values" will remain). Finally "Control" will call ask and that formatting method.

[bilderbuchi]

I don't understand, what you try to achieve with that test.

I was thinking that we make sure we cannot inadvertently introduce a change that would change the behaviour when parsing binary messages because a whitespace character like \x20 is located at the end of the message. Maybe it was not a useful idea.

Regarding separation (to log an idea): we could rename values to "extract_values" (or similar) and remove the messaging part (a deprecated "values" will remain). Finally "Control" will call ask and that formatting method.

I considered that, too. However, values is convenient because you give it a command string and get back ready-made processed values. It's used 30+ times in our codebase. If we deprecate values, I think it would probably just be replaced by self.values(self.ask('command_string')), which is not actually nicer. So, we should probably should retain values. We could still change the usage within control, of course (we could also do this with the proposed refactoring approach.

[BenediktBurger]

As we talk in another issue about the reworking, I close this issue.