Pullreq nrao

[gitj]

I've added a few improvements; in particular many try, except, finally clauses to make sure the serial port connection gets closed if something goes wrong. Also, ability to set the serial port timeout easily. Finally, added a few convenience functions to simplify common tasks. I haven't thoroughly tested it all, but the changes are straightforward.

[patbrandt]

It would be better to apply some consistency in error handling.

• In ValonSynth.get_frequency, Serial.write can throw either a ValueError (if the port is closed) or a SerialTimeoutException, but it is not guarded by the try/except. The documentation is a little vague but Serial.__init__ can throw an exception if you specify a port (because it immediately tries to open it) so I'm guessing Serial.open should also be included in the try block.
• If error handling is added there should be an overall philosophy to it. In some cases the errors are silently suppressed (get_frequency) and may allow invalid/incorrect results to be returned to the caller (or even generate other exceptions when empty bytes are sent to struct.unpack), while others actually rethrow the exception after making sure the port is properly closed (set_frequency).

In regards to the latter, the current/previous philosophy was "throw hands in air and give up", which I agree is a bad way to go about it. But hiding exceptions from the caller seems almost as bad since it hides that something went wrong. Personally I vote for cleaning up and rethrowing.

In regards to the convenience functions, my initial thought it that they can easily be accomplished via a subclass (e.g. MoreConvenientSynthesizer), rather than cluttering the base class with umpteen methods. But I don't really maintain the code anymore, I just have strong opinions. ;)

[gitj]

Hi Patrick,
I think you misunderstood the intended function of the try/finally clauses I added: The only point is to be sure that the conn.close() method gets called if some error comes up. This simplifies error handling higher in the code, because then you don't have to worry about resetting the state of conn to closed before trying another command.

For example, if I call get_frequency and someone has sent some spurious characters to the synthesizer, it's likely that the method will encounter an error. As it is now, an exception will be thrown, but conn will be left open, which means that if I then catch the exception in my code and want to retry the command; I have to first check that conn is closed. Otherwise, when I call get_frequency again, it will choke on the conn.open() line, because the connection is still open.

In particular, I use this code a lot from the ipython prompt, so if an error comes up, I have to manually call vs.conn.close(), which gets tiring.

I wrote the try/finally clauses such that they will still throw the same exact errors as before (note, in many cases there is no except block, so the exception still gets raised, and when I used the except block, I re-raise the exception).

Regarding the convenience functions; I definitely want these in the module, so I don't have to install yet another dependency (both valon_synth and my_valon_synth). Creating a subclass is fine I guess, but seems more cumbersome. This class is used for exactly one piece of hardware: the Valon synthesizer. So it's not like it's going to grow an infinite number of convenience methods. I added the ones that I use most often; again often for the sake of making interactive use more pleasant.

Glenn

[patbrandt]

I guess I can see your point regarding exceptions, but still to be complete I think the try blocks should include any/all of the serial port operations (at least write and read, if not also open) as they can all throw. Again using get_frequency as an example, the first call to Serial.write could throw and the connection would still be left open since it is not in the try block.

Also the try/except block in set_frequency, set_rf_level, and set_options could be changed to try/finally for consistency.

And if we're adding convenience methods, may as well do it for all the synthesizer dependent methods (*_label[_a, _b, s], *_phase_lock[_a, _b, s], *_vco_range[_a, _b, s], etc) again for consistency.

[gitj]

You are right: I missed a .write. I'll fix that. The intent was to include all .write and .read calls. I don't think .open should be included because if .open fails, the port should still be closed, so it shouldn't be needed. But I suppose it shouldn't hurt (provided closing an already closed port doesn't error; I don't think it does).

Exhaustively adding convenience methods is fine, but I'll save that for another day; I included the ones I use all the time.