Closed eoinell closed 3 years ago
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.
Yeah that's more or less it. I'm thinking to go with an ignore_echo keyword in the SerialInstrument.write.. it still returns nothing. One of those things I could spend a lot of time thinking about but this preserves functionality (and is already written and tested). Side note realised that SerialInstrument.readline() didn't time out as each self.ser.read(1) had no timeout, so added an fset for self.timeout that also sets self.ser's
From: wdeacon notifications@github.com Sent: Thursday, August 27, 2020 7:25:00 PM To: nanophotonics/nplab nplab@noreply.github.com Cc: Eoin Elliott ee306@cam.ac.uk; Author author@noreply.github.com Subject: Re: [nanophotonics/nplab] No convenient way to ignore an echo from a command that returns no other string in MessageBusInstrument (#120)
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.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/nanophotonics/nplab/issues/120#issuecomment-682115426, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKORCB6TQVZFRDSI7TIMFALSC2QHZANCNFSM4QM4FOYA.
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
This works fine, but if there’s no string waiting to be read then the command times out
From: YagoDel [mailto:notifications@github.com] Sent: Friday 4 September 2020 08:28 To: nanophotonics/nplab nplab@noreply.github.com Cc: Eoin Elliott ee306@cam.ac.uk; Author author@noreply.github.com Subject: Re: [nanophotonics/nplab] No convenient way to ignore an echo from a command that returns no other string in MessageBusInstrument (#120)
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
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/nanophotonics/nplab/issues/120#issuecomment-686968246, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKORCB7LIP3UKDQOGVD23X3SECJH7ANCNFSM4QM4FOYA.
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?
So normally query
takes a string as an argument and returns a reply from the instrument. When the instrument ‘echos’ it repeats all commands sent, and then the reply. However in some instances the instrument will echo the command, but no other reply is expected (like just changing a speed setting), and this second readline
times out.
From: YagoDel [mailto:notifications@github.com] Sent: Friday 4 September 2020 09:42 To: nanophotonics/nplab nplab@noreply.github.com Cc: Eoin Elliott ee306@cam.ac.uk; Author author@noreply.github.com Subject: Re: [nanophotonics/nplab] No convenient way to ignore an echo from a command that returns no other string in MessageBusInstrument (#120)
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?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/nanophotonics/nplab/issues/120#issuecomment-687012717, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKORCB63GD5GGVEWXXB5A73SECR67ANCNFSM4QM4FOYA.
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:
Situations when new branch won't work and the old one does:
Situations when neither work:
write
to either clear the echo or not instead of having it as a class attribute in self.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 query
s 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
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 inwrite
calledignore_echo=False
? Better solutions?