Added HWI support #53
Conversation
- Add new Processor class to abstract RA2, QS, HWI differences - Abstracted the Parser classes to have Process specfici parsers - Support for HWI LEDs still missing Added a simple CLI for interactively exercising the library
There was a problem hiding this comment.
Thank for uploading, it's a really good direction in general to abstract some of these things to support different processors in this family.
I started reviewing and have some top level comments that you need to address first.
This is a huge rewrite of quite a lot of code, espeically some core connect and some command management code. Having it all together in one PR and even one git sha is really hard to review. IF anything breaks here it'll be impossible to bisect and revert partially. You need to split this up into logical changes that we can review independently. For instance:
- the cli tool is cute, but should be in a separate git change.
- don't bump the version.
- add the processor base as a separate change
- basically, the refactor of the existing code to move some subset into the base processor code without any of the HWI code. That way we can review the implications of splitting things up without really worrying about whether or not you broke the existing integration.
The connect code for instance looks quite a bit different, without any explanation as to why. It was some funky stuff to get right in the first place, so without a good explanation of why you rewrote it, i won't accept it.
Overall, it's a good direction to split things up to support these other processors, but lets do it in a way thats debuggable, traceable and doesn't set us back stability wise. Split things up into smaller changes. They can all be part of the same PR if you like, but I'd like to see a clean, split-up patch stack where the git changes can be reviewed individually as a logical blocks.
|
|
||
| # self._l = pylutron.Lutron(controller, user, password, pylutron.Processor.QS) | ||
| # self._db = self._l.load_xml_db("/tmp/cached_xml_db") | ||
| self._l = pylutron.Lutron(controller, user, password) |
There was a problem hiding this comment.
Let's use a few more characters on the name :) They're free.
| import time | ||
|
|
||
| # Change these to match the site | ||
| default_controller = "192.168.111.123" |
There was a problem hiding this comment.
Leave these blank since they likely will only work for you.
| button_parser = argparse.ArgumentParser() | ||
|
|
||
| class lutron(cmd2.Cmd): | ||
| def __init__(self, controller, user, password): |
There was a problem hiding this comment.
inconsistent spacing with the rest of the project. we use 2 spaces everywhere. Also, in init.py your change is all over the place. I even saw 3 space indents. Please reformat your entire CL to be consistent.
|
|
||
| class QSProcessor(Processor): | ||
| """ Processor for QS systems """ | ||
| Processor.QS = "QS" |
There was a problem hiding this comment.
This is pretty dirty, please don't do that. The proper way to solve this is to have a ProcessorType enum, defined either as a top-level object, or namespaced inside the Processor class. Having derived types add random variables into the base class is bad form.
| # static factory for creating the processors. | ||
| # example: | ||
| # processor = Processor.Factory[Processor.HWI]() | ||
| Factory = {} |
There was a problem hiding this comment.
This is a really ugly way to make a factory with just raw access to the dictionary.
I suggest you create a true factory function (e.g. Create). For registration, you can add a Register function or you can just have Create instantiate the right thing based on some input without needing an explicit registration of the subclasses (i.e. just if xx: return yyy) since the number of implementations is small and all in this same file for now. You can also separate it out into a separate factory class, ProcessorFactory, that does the same thing.
| @@ -29,6 +32,290 @@ | |||
| socket.timeout, | |||
| ) | |||
|
|
|||
| class Processor(object): | |||
| """ Encapsulates the specific communication protocols associated with a Lutron Processor. | |||
There was a problem hiding this comment.
no space after """
Add a blank line between the first line and rest of description. Applies to rest of your change.
|
|
||
| class CommandFormatter(string.Formatter): | ||
| """ Helper class to format strings for conversions. Adds to the Formatter spec: | ||
| - Added a psuedo arg_name 'all'. An arg_name such as {all} will return all the arguments |
There was a problem hiding this comment.
bad indent, please follow conventions.
| else: | ||
| raise EOFError('Telnet object already torn down') | ||
| except _EXPECTED_NETWORK_EXCEPTIONS: | ||
| _LOGGER.exception("Uncaught exception") | ||
| try: | ||
| # if it didn't connect, wait a bit to see if the transient error cleared up. |
There was a problem hiding this comment.
why do we need this sleep if there's one below?
|
Sure, smaller commits, more why for the connection code and other complex logic. You'd think by now github would have added some sugar to make that easier. To cut down the iteration, how exactly do you want to see this pushed? One PR, multiple PRs? Multiple branches? I'll break it up into logical stacked SHAs, just want to get the logistics right the first time. The only minor pushback I have is the factory. Why not take advantage of python's dynamic typing and allow a subclass to push an id into its parent? In strongly typed languages, the enums need to be listed separately for the whole thing to work. My proposal has everything about the subclass self contained, there's no need to make additional edits elsewhere. With everything in a single file as it is now it doesn't matter much, but the design principle holds. Think about how this would work if we were implementing plugins? It's your project, if you say traditional factory, i'll add the machinery. |
This PR adds HWI support. Abstracted out the processor and parser classes. I also added a small CLI, I found it useful for testing. Cleaned up a bunch from the proof of concept a while back.
I don't have a QS/RA2 system to test against. It should just work, but there was enough refactoring that I could have broken something.
I merged from master and bumped up the version number.