niolabs / python-xbee

Python tools for working with XBee radios
MIT License
101 stars 45 forks source link

Do not break on error as this is not thread safe #24

Closed jamesleesaunders closed 8 years ago

jamesleesaunders commented 8 years ago

Following the reversion of PR18 which contained a handful of different fixes I have now broken it down into in smaller individual PR's to allow each fix to be discussed and committed individually.

This PR has been raised to trigger discussion as to a more appropriate action and (for now) simply comments out the 'break' statement in base.py on _error_callback. I have done this as it is not thread safe and meant that threads would hang (break) and the main code is not aware, this in turn appears like the thread has hung (but no error messages are presented to the user).

It may be that there is a more appropriate solution? I am happy to alter/add to this PR further if people feel necessary.

Further information on this issue can be seen here: http://axotron.se/blog/problems-with-python-xbee-2-2-3-package/

jamesleesaunders commented 8 years ago

@hansmosh This is the proposed fix for the break on error issue. I don't know if you had a more appropriate solution in mind?

hansmosh commented 8 years ago

The addition of the error_callback was a small addition that I made when I first took over this project. Originally, exceptions besides ThreadQuitException were uncaught which would un-gracefully exit the thread. My intention was to keep the same behavior of exiting the thread but allow the user to react to that with the error_callback.

https://github.com/nioinnovation/python-xbee/commit/73dffd17a8cdc0c13d502f3bfc7f0bd3703c5a78

For my use case, that made sense as it meant I needed to re-initialize my XBee. What behavior are you seeing? Are you seeing an exception that you are want to be caught but still let the thread continue to run?

jamesleesaunders commented 8 years ago

Hi Chris,

Thanks for getting back to me. The particular issue for me was when the xbee received a packet type it did not recognise (in my case it was the xA1 route-record-indicator). This caused the code to barf the exception and then break. Also see URL link example.

I would have expected more normal behaviour upon receiving a unrecognised packet would be to throw error message but continue running. Otherwise you run the risk of the code dying each time some weird/new packet type comes about.

In my case the xA1 packet issue was fixed by adding it to the Zigbee dictionary file (so for my specific needs it is OK), but I still think the issue still remains.

Thanks again for this great library!

Jim

On 7 Sep 2016, at 01:00, Chris Brackert notifications@github.com wrote:

The addition of the error_callback was a small addition that I made when I first took over this project. Originally, exceptions besides ThreadQuitException were uncaught which would un-gracefully exit the thread. My intention was to keep the same behavior of exiting the thread but allow the user to react to that with the error_callback.

73dffd1

For my use case, that made sense as it meant I needed to re-initialize my XBee. What behavior are you seeing? Are you seeing an exception that you are want to be caught but still let the thread continue to run?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

hansmosh commented 8 years ago

Got it. Why don't you re-push this commit with the break deleted instead of commented out. Go ahead and include a link to that blog post in the commit message if you want a reference to it somewhere.

jamesleesaunders commented 8 years ago

Thanks Chris, I removed the commented out #break line and I see you have now merged it in :-) Thank you.