niolabs / python-xbee

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

Break out thread implementation, add Tornado Support #53

Closed bp1222 closed 7 years ago

bp1222 commented 7 years ago

Break the threading implementation away from the core backend pieces which allows different loop support. This commit also creates the support for the TornadoIO Loop.

bp1222 commented 7 years ago

Note: there is a test that fails, TestReadFromDevice.test_is_response_parsed_as_io in test_ieee.py which is failing in master as well. I'm not sure what the correct test should be, so it's also occurring the same within the tornado implementation

jamesleesaunders commented 7 years ago

I'll clone and give this code a whirl with my XBee and Home Automation project.

I guess we need to get to the bottom of that failing test also? You say it is also failing in master branch? I can't see it failing on master, i see you have only removed a spurious semi colon in the test - that could not have done it?

I will see if/what help I can provide with testing.

I can confess to being a Python expert nor have I used Tornado so I am not really qualified to comment too much, but like @mattdodge I am a little nervous about breaking backwards compatibility on this.

The above said it is nice to see some more interest in this library! I feel this library is an unsung hero in the success of XBees!

bp1222 commented 7 years ago

fwiw, it's in test_ieee.py (both tornado/thread) https://github.com/nioinnovation/python-xbee/blob/master/xbee/tests/test_ieee.py#L607

py.test spits out:

E       AssertionError: {'fra[80 chars]er': b'\x01\x02\xaa\x00\xaa\x00\xff'} != {'fra[80 chars]er': [{'dio-1': True, 'adc-0': 255, 'dio-3': T[31 chars]ue}]}
E         {'command': b'IS',
E          'frame_id': b'D',
E          'id': 'at_response',
E       -  'parameter': b'\x01\x02\xaa\x00\xaa\x00\xff',
E       +  'parameter': [{'adc-0': 255,
E       +                 'dio-1': True,
E       +                 'dio-3': True,
E       +                 'dio-5': True,
E       +                 'dio-7': True}],
E          'status': b'\x00'}

so the param is parsed as the hex values, rather than the list-dict.

jamesleesaunders commented 7 years ago

I made a start at testing this new code today, but before I did I wanted to get to the bottom of the aforementioned failing test. After a little head banging I finally managed to figure out why it was failing (and it was something silly!). I have put out a separate PR #54 to fix this.

The issue was interesting as it only showed up using pytest where as python-xbee uses nosetest for its Tracis CI testing. Please work on getting thev Travis CI test passing as it is currently failing (not for the reason above).

Sorry I did not get very far in the end with actually testing your new code, once this test fix is merged in I will have another go. However, as an initial observation, I am a little uneasy on the way the previously quite simple code and its tests has now been scattered and in places duplicated across different folders (but like I say I am not a Python expert so I would call on @mattdodge and @hansmosh to make the final decision). I do still want to test the code for basic backwards compatibility. I will get back to you soon.

All the best, J

jamesleesaunders commented 7 years ago

@bp1222 I have had another look though your PR code. So, I fully appreciate you have probably take a considerable amount of time to put this together but in my personal opinion I would prefer this PR to be broken down into smaller changes (but like I say this comes from my limited knowledge of Perl and Tornado, perhaps the other guys will be more comfortable?). Nevertheless, I have a few random comments/observations which may or may not be of use (you probably know more than I so feel free to ignore if you see fit?!)...

  1. PyCharm is complaining to me that your new /xbee/tornado/tests/test_ieee.py file and its use of super() without any arguments is not supported y Python 2.7 - This may or may not be an issue?

    super().setUp(*args, **kwargs)
    self.xbee = XBee(None)
  2. In the base /xbee/init.py file it includes the files from xbee.thread ? Is this correct? Should it not be the files from xbee.backend?

    from xbee.thread import XBee
    from xbee.thread import ZigBee
    from xbee.thread import DigiMesh
  3. Why have you decided to move some of the files to 'backend' ? Could these not stay in the root /xbee/ folder?

  4. There appear to be a number of duplicated tests. The tests need a tidy up as it is. This makes this more complicated. Possibly move all the tests to a single tests folder??

  5. You mention that this breaks backwards compatibility? What exactly does it break? I had a quick whirl with an xbee in a simple example and it appears to work as normal.

bp1222 commented 7 years ago

@jamesleesaunders Points 1 & 2 of your comment were in the code review, and taken care of:

3) I opted to create the backend folder to distinguish it as backend code, rather than an implementation.

4) I was thinking this as well. Could probably have some general test dir that has the duplicate, but each implementation have its own "runner" to do the thread/ioloop of resolving the input.

5) Per @mattdodge , I changed the top level init to use the treaded implementation rather than nothing, so it should be BC now.

mattdodge commented 7 years ago
  1. Seems resolved
  2. Seems resolved
  3. I'm good with the backend functionality being in its own folder
  4. If the exact same tests exist in two places then something is probably not right about that. If we want to run the same test with two different runners then we should make a base class and then subclass and set the runners in the respective folders.
  5. Backwards compatibility looks good, and according to @jamesleesaunders it works on his existing setup so that's good too.

The biggest concern for me at this point is the Travis build. Tests are still failing (I have merged #54 which may resolve that if we bring that into this PR). I'm also a bit curious about how the tests didn't catch the super() bug (good catch, Jim) since they run it with 2.7. Or maybe they did catch that and since they were failing we just didn't notice?

Let's get the Travis build green and then double check that we're all square on python 2.7 then we can merge.

bp1222 commented 7 years ago

This PR builds clean now, having tweaked the travis build, and rebasing onto master to pull in the test fix.

mattdodge commented 7 years ago

@bp1222 can you please also add some docs for running in Tornado mode to the README and docs folder? Specifically, we should include something about how the threading can run in multiple modes and also how to install the tornado dependencies if needed. Thanks

mattdodge commented 7 years ago

LGTM, will merge and then run the release to bump this to 2.3.0