No convenient way to ignore an echo from a command that returns no other string in MessageBusInstrument
Closed this issue · 17 comments
It would be nice to still know if it echoed rather than just discarding it with self.write(cmd). Could make a kwarg for this in write called ignore_echo=False? Better solutions?
Just to make sure I understand, the aim is to ignore an echo without calling a query? i.e. have a write command ignore the echo?
I think if you want to do that by changing the write command you would have to do it in SerialInstrument or visa etc as the write command is over written when MessageBus is subclassed. I guess you could get the desired functionality by decorating the write command but to be honest there's not usually any harm in calling a query command that isn't expecting a return apart from the increase in run time time due to the timeout. You could off course add another function to MessageBus that calls the write command and ignores the echo, but thats not quite what you want.
Ah good to know!
Conceptually a good plan, just need to be careful not to break query (which of course calls write) when it attempts its own echo check.
Not sure if I completely understand, but MessageBusIntrument.query uses the ignore_echo attribute to check if it echoed and it sends a warning to the logger if it didn't. So you kinda already know if it echoed or not
Maybe I don't understand what we mean by echoing, but if there's no string to be read, then the device hasn't echoed, and it should throw out a timeout error, shouldn't it?
I swear that's the expected behaviour for a query, isn't it?
But now I think get the problem a bit more (let me know if I'm completely off). For echoing instruments sometimes you want the write command to also clear the read buffer so that you don't read the echo the next time you readline
So implement the same kind of echoing as the MessageBusInstrument.query but in the write (which reading back is what you said in your comment 👍 )
In terms of implementation, I would make a check_echo method in MessageBusInstrument containing:
if self.ignore_echo:
echo_line = self.readline(timeout).strip()
if echo_line != queryString:
self._logger.warn('Command did not echo: %s' % queryString)
And can then call that from the subclasses
Just FYI, I've made a first pass at a solution of this (see branch issue120)
It doesn't seem to break any functionality (all my instruments still work, and I think I'm using a Serial, a Visa, and an APT). But I don't think any of mine echo, so needs testing on a gio_rotator or a Thorlabs SC10
Situations when new branch should work:
- when an instrument either always or never echoes when writing.
Situations when new branch won't work and the old one does:
- if an instrument only echoes with specific commands (which coincide with the ones that you query, for whatever reason).
Situations when neither work:
- if some commands echo and some don't. In that case, the instrument would require an option in the
writeto either clear the echo or not instead of having it as a class attribute inself.ignore_echo
Either way, if there's no longer interest in this issue we should close it 😅
Sorry, I missed this at the time. I don't want to change the reimplemented method from write to _write to avoid confusion (and a lot of rewrites). I think (based on limited experience) that the case I initially mentioned is quite rare. If a VisaInstrument wants to do something similar it's simple enough to extend the write method to optionally ignore an echo as above.
I agree the initially mentioned case is rare, but rewriting the VisaInstrument.write only solves it for a VisaInstrument, not for a MessageBusInstrument (which was the original problem?)
I get the point of refactoring being potentially confusing, however, the refactoring only happens in the classes that immediately inherit from MesssageBusInstrument, not from any of the ones inside nplab.instrument folders, so this change happens in the background and none of the users need to change the way they write instrument classes.
In that branch, I've already done the relevant refactorings, except perhaps the APT_VCP
That's true, I forgot that write/query gets implemented in Serial/VisaInstrument not each subclass. Happy for you to go ahead and merge the above! My only worry is whether some currently in-use instruments don't echo write commands but to echo querys and would timeout indefinitely with the above. Should the timeout kwarg not be set to the timeout attribute if none is passed to the write method?
Well, given that it rewrites the base layer, I wouldn't merge it before testing the branch (which now is a fair bit behind, so might need a pull from the current master, which I'm happy to do tomorrow at work)
About the instruments that echo on the query but not the write, it would mean we'd need two flags to keep track of echos, right?
Yeah, I'm not sure they exist though..
Ah, nvm then. Because I would've thought that'd be a really silly instrument that would deserve not being thought about for a general base class. Then I'll do a quick update and test of the issue120 branch tomorrow, and then you can do the same and we can merge it