XBee ZB response definitions are incomplete

[GoogleCodeExporter]

I attached a new ZB device to my network and all of my software started 
crashing.  The documentation for wait_read_frame says it will wait until a 
valid frame appears.  There is no mention of any exceptions being raised.  If a 
frame arrives with an unexpected frame ID, KeyError is raised.

_wait_for_frame looks like it's catching a ValueError and retrying, but 
_split_response can raise KeyError.

I'm kind of new to Mercurial, but I believe I have two fixes for you to look at:

http://code.google.com/r/davidmnesting-python-xbee/source/detail?r=5624b2ba2a746
3148f3cbf811f4bffad06b35cc4
This fixes the immediate problem and allows wait_read_frame to avoid raising an 
exception, silently discarding frame types it doesn't understand.
http://code.google.com/r/davidmnesting-python-xbee/source/detail?r=c53e8626dafe8
d650889d5771854510d4efed018
This adds all of the missing frame types I could identify from the latest ZB 
user's guide.

Original issue reported on code.google.com by david@fastolfe.net on 21 Jun 2011 at 4:16

[GoogleCodeExporter]

Hi David,

Thanks for your feedback.

Perhaps the documentation isn't clear enough. The intent was that 'valid frame' 
and 'unrecognized response' were two separate concepts. A 'valid frame' is a 
binary API frame structure that passes check-summing, with no concern for the 
data contained inside it.

An 'unrecognized response' occurs at a higher level. The API frame structure 
was already declared valid, but its contents did not match any defined 
response. Thus, the internal structure of the frame's contents could not be 
parsed.

Since invalid frames could never be parsed correctly, they are silently 
discarded in _wait_for_frame (xbee/base.py, near line 132). However, receiving 
a valid, yet unrecognizable frame was deemed an error since it implies that 
python-xbee was not working properly. In all probability, it should have been 
able to parse the given response, but was unable to do so. The alternative of 
silently ignoring expected, legal responses would prove very frustrating to 
those who aren't aware of the behavior.

My apologies for the confusion; I hope this clarifies a few things. I would be 
happy to accept your definitions of the missing response types; I'll try to 
merge in your change later this evening. 

Original comment by pmalms...@gmail.com on 21 Jun 2011 at 5:15

• Changed title: XBee ZB response definitions are incomplete
• Changed state: Accepted
• Added labels: Component-Logic, Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Thanks for the clarification.

As things are now, code written according to the documented API is not 
forwards-compatible and will crash when new response codes are introduced on 
the network.  Not all "response" frames are actually responses; some will be 
unsolicited, like the routing frames, so it's entirely likely that a client 
application will exist with no expectation that it's going to be receiving 
these new frame types.  Some alternatives to silently discarding unknown frames:

1. Log them with logging.error (and then discard them so that the application 
won't crash)
2. Raise an exception only if the user requests so (e.g. a "be_tolerant" flag 
in the initialization of the object)
3. Clearly document the exception as part of the API, so that users know to 
catch it.

I suspect the general case for #3 will be that users will ignore it, which is 
essentially what I optimized for in my suggested patch, but I understand that 
some people might like to know that the module saw a frame that it didn't 
understand.

I can try and put together a documentation fix if you think that's the best 
path forward.

Original comment by david@fastolfe.net on 21 Jun 2011 at 5:48

[GoogleCodeExporter]

Note that this exception can also occur in the background/callback thread, 
which causes the background thread to crash with no indication to the caller 
and no way to restart it.  The user code will just fail to ever see any new 
events.

Original comment by david@fastolfe.net on 22 Jun 2011 at 5:01

[GoogleCodeExporter]

Those are good points; this definitely needs to be fixed. 

I like all three of your suggestions. I'm not sure what the convention is for 
using the logging module within library code; do you know?

My first thought would be to add two optional arguments to the XBeeBase 
constructor: a logger object, and the 'raises exceptions' flag you suggested. 
If a logger is provided, we use that, otherwise we create/use a default one. 
The exceptions flag would be false by default.

For the background thread, I think we can just log and squash any/all 
exceptions raised.

What do you think?

Original comment by pmalms...@gmail.com on 22 Jun 2011 at 5:59

[GoogleCodeExporter]

It would be great if at least David's ignore patch were integrated.  The 
current behavior is worse than silently ignoring unknown packet ids. It would 
be nice to be able to use this otherwise fine library without having to patch 
it first. 

Original comment by ergo...@gmail.com on 22 Jan 2013 at 7:48