Thermostat Controls.

[lanrongwen]

While I can't start right away, I am looking at adding support for the 2441TH Thermostat. From what I can tell, it looks like it 1) Uses both standard and extended messages 2) uses different Groups to control the heating, cooling, general info, etc. So this could be fun. Open to any help, suggestions, etc along the way...

[lanrongwen]

Hey guys, sorry I've been quite for some time. But I've been thinking it through and was thinking I would break out Thermostat's into it's own file to try and make it cleaner. The though is when a message comes in, check against the ipdb to see if it's from a thermostat, if so, send it to the thermostat class to be handled. Something along those lines. The idea is to localize it and make it easier to tell what's going on and how the message is being handled. As I think there are some overlapping commands with things like switches that probably don't make sense in a thermostat. (oh, and the protocol.py file is getting pretty long to scroll through... ) Thoughts? Suggestions?

[teharris1]

I have spent a good amount of time with the current code recently and I believe the concern you raise around protocol.py getting pretty long is meaningful, I suggest this should be broken up into a few different concerns. The length is one concern and the readability is another. It feels to me like multiple concepts are all bundled into one class. I suggest considering separate message handling class combined with a set of message classes. This would allow the protocol class to focus on orchestration and communication. Additionally, a command class or set of command classes that can be responsible for the actions.

[teharris1]

Thinking about your idea more and looking at the zwave code, I think your approach is right and should be used across the board. PLM class should not be the class that the insteon_plm platform modules calls in HA. In other words, we create an entity base case that all insteon device types derive from. This way "turn_on" is available to every device but "set_temperature" would only be known by the thermostat (or climate) device entities. The PLM class should be the class that sends and receives messages on the wire. It should delegate handling of messages to a command manager. Command manager would then know how to map commands to entity classes like base or thermostat. Entity classes would call PLM to send insteon messages that they create. What do you think?

[TD22057]

FYI - I didn't get anywhere trying to add smoke bridge support so I decided to write my own package for doing Insteon<->MQTT based more on the insteon-terminal software. it implements a lot of the design ideas you're describing (device classes, message classes, etc). It's still under development but you may want to look at it for ideas. Sorry - not trying to hijack this thread or anything but it wasn't clear any new development was happening here.

[lanrongwen]

So I think part of the question is do we do a refactor or look at a new code base. I'd hate to loose some of the progress this project has made, but I do agree that a refactoring of the code is needed to make it more readable/easier to support. I am also curious on your MQTT package and how that's coming along. I'll have to check it out.

[teharris1]

My opinion is to refactor. There is not a fundamental problem with the current code base and there is not enough code to make refactoring too difficult. I think once we start separating concerns the code becomes much more clear and refactoring will be straight forward. I had a took at your Insteon-MQTT package and it looks well defined and structured. I think there is a lot we could like to steal (um, borrow) from your concepts. My feeling is to have a standalone Insteon package that is abstracted from the HA framework so if you want to write an MQTT wrapper around it for portability it can be done.

[teharris1]

Here is my current development where I built out the message classes. It is still a work in progress but you should be able to see where it is going. I made Message a factory class and MessageBase is the base class for all messages. This way PLM can pass a message to the factory and receive a formatted message back. Would love feedback.

[lanrongwen]

Cool. I was thinking something along these lines (it’s pretty rough so bear with me):

1. On Start a. Register the devices (through all-link or other means) in a master handler class.
2. On Message in a. Receive a message in it's entirety. i. If the message is for us, look at the source device type (from the registration above). ii. Send it to the registered handler for that type (I’m thinking a class per type, all referenced off a generic base type) iii. That type can look at the message and decide what to do with it.
3. Probably a giant case statement of handled command codes that send it to the appropriate method.
4. Return the appropriate message/object to the main class.
5. On Message send a. Call the appropriate method in the handler for the device type. i. It would generate the command. ii. It would then return the command to the master handler class b. Master Handler class would then send the message to the modem/messaging class to send it out on the wire.

Examples:

1. On Start a. On load we see we have 2 switches. b. Master handler loads the appropriate classes to handle the switches.
2. On Message In a. Message from switch 1 comes in addressed in a way that we should look at it. b. Master Handler sends the message to the message in method on the switch 1 handler and then starts listening for the next message. c. It looks at the command code and determines it’s a state update message, sends it to the state update method of the switch class. d. State update method gets new state. e. Current state is updated with the new state.
3. On Message Send a. We need switch 2 to turn on. b. Master handler calls the set state (or turn on) method in the object for switch 2 c. Set state returns the message to be sent on the wire d. Master handler calls the modem class to send it out.

The overall thought was it could help make traceability and debugging easier and give an easy path for others to learn the code and add new device type handlers. :)

[lanrongwen]

Doing a quick once over of @teharris 's code, and I think he's going down something along the same lines as what I was thinking. Curious what his thoughts where on different device classes?

[teharris1]

Generally, yes. My thinking is PLM class handles sending and receiving messages. A received message gets handed to a message factory class to create the actual message. Here I debate if the message class should know what command to execute or if the PLM class should hand the message off to a command class to handle the message. I lean towards option 2. Here are two examples for option 2:

Example 1: PLM class receives an All-Link Record message and sends it to the message factory. The message factory sends back a message object. PLM then sends this message to a Command factory class. Command factory class creates an All-Link command class. The All-Link command class knows how to handle All-Link messages.

Example 2: A thermostat device in HASS tells an insteon thermostat command class to set the temp. The thermostat command class sends a SET TEMP message to PLM to set the temp. The thermostat device in HASS knows the insteon thermostat command class rather than the PLM class.

[teharris1]

@lanrongwen I think we agree to utilize some form of "Command" pattern, correct? I am thinking of an implementation similar to this: Function Objects. Thoughts?

[teharris1]

To put a little more detail, the "Invoker" would be the PLM class and the Device classes. In other words, the "Turn_On" command could be invoked from a message received by the PLM, or it could be received from a user action in HASS. Either object (PLM or device) would invoke a command. Make sense?

By the way, I realized I did not answer your prior question well about the different device classes. The key question I would want to get agreement on is if we are building a HA module or if we are building an Insteon implementation. If we are building a HA module, I would use HA devices as the device classes. If we are building an Insteon implementation I would use Insteon device categories. My vote would be for an Insteon implementation that is consumed by HA but could go either way. If we focus on an Insteon implementation it can be leveraged outside of HA (such as the MQTT implementation that @TD22057 is working on). If we develop a HA module, it may be easier but less portable to other uses.

[TD22057]

FYI - I know you decided to fork and refactor this code but I thought I'd let you know that the first version of my Insteon-MQTT system is up and working with HASS. I still need to add a more code comments and more test cases but it's working well for my configuration and seems pretty reliable so far.

[larizzo]

@teharris1 You thought about opening up the issues on your github project? We could discuss on there. I'd like to test your code let me know when it's getting closer to usable code.

[teharris1]

@larizzo I opened comments on my GitHub fork but I am reluctant to have a separate thread for design decisions. It feels a bit clunky to direct people here and then redirect people there for refactoring discussions. Unless the wider audience here disagrees, I would rather keep the refactoring thread inside this core project. To that end I will start a new topic for refactoring that we can contribute to.

[teharris1]

@lanrongwen if you are still up for it, I would be happy to guide you through the creation of the Thermostat device. I suggest that I create a shell device and states set for you to work off of. From there you code the message handling. Let me know.

[lanrongwen]

@teharris1 Hey sorry it's been a crazy couple of months. Let me go through the code (so much has changed) and I'll reach back out. Looks like it should be a lot easier to add the device.

[teharris1]

Closing this issue as the insteonplm module is no longer maintained. Please use the pyinsteon module instead.