search

nugget / python-insteonplm

Python 3 asyncio module for interfacing with Insteon Powerline modems
MIT License
33 stars 19 forks source link

WIP Create superclass for insteon thermostats #105

Open wz2b opened 6 years ago

wz2b commented 6 years ago
wz2b commented 6 years ago

I'm doing this in two steps. Step 1 was to create a base class so that we can support the concept of different insteon thermostats that are mostly the same. Step 2 is coming (I'm working on it now). There definitely are some differences, especially the read status message 0x2E. For example:

On the TH2441ZTH: message 1 returns data3=0, data4=localTempLowByte, data5=Humidity, data6=TempOffset, data7=HumiOffset, data8=SystemMode, data9=FanMode ....

On the 2441V message 1 returns: data3=mode, data4=humidity, data5=temperature, data6=cooling setpoint, data7=heating setpoint, data9=fan mode ...

The current insteom climate module doesn't make use of these 0x2E status messages, but it could (and I'd argue should). Especially when you are actually changing setpoints. In this situation it makes sense to me to be paranoid about polling the device from time to time to ensure it's set 1) to what you want it to be 2) to something sane so it won't burn down your house.

I'm open to suggestions or guidance on any of this, though. I'm gathering basedon the fact that no device was declared in ipdb for cat=0x05,subcat=0x03 that I'm the only one (so far) using the 2441V. Part of the purpose of my pull request is to carve out a place I could refine it while minimizing the chances of breaking somebody already successfully using a 2441TH.

(Note I made a rookie python mistake when I submitted this PR - I used super() but passed self - I fixed that but now it's telling me useless super() delegation so I need a few more minutes on this).

On Sun, Sep 9, 2018 at 10:58 AM Tom Harris notifications@github.com wrote:

@teharris1 commented on this pull request.

This indicates there is no difference between the 2441th and the 2441v. Is that correct?

In insteonplm/devices/climateControl.py https://github.com/nugget/python-insteonplm/pull/105#discussion_r216159876 :

@@ -100,3 +100,23 @@ def async_refresh_state(self):

pylint: disable=unused-argument

def _mode_changed(self, addr, group, val): self.async_refresh_state() + + +class ClimateControl_2441th(ClimateControl_Base):

  • """TH2441TH thermostat model."""
  • def init(self, plm, address, cat, subcat, product_key=None,

Don't need init if all it does is call super().init

In insteonplm/devices/climateControl.py https://github.com/nugget/python-insteonplm/pull/105#discussion_r216159885 :

+ + +class ClimateControl_2441th(ClimateControl_Base):

  • """TH2441TH thermostat model."""
  • def init(self, plm, address, cat, subcat, product_key=None,
  • description=None, model=None):
  • """Constructor, delegates most work to the base thermostat class."""
  • super().init(self, plm, address, cat, subcat, product_key,
  • description, model)

  • +class ClimateControl_2441v(ClimateControl_Base):

  • """TH2441V thermostat adapter model."""
  • def init(self, plm, address, cat, subcat, product_key=None,

Don't need init if all it does is call super.init()

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nugget/python-insteonplm/pull/105#pullrequestreview-153583855, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc2OSO7rfAYQH7a6_Zca6_71kDnTPy3ks5uZSyEgaJpZM4Wf4mN .

wz2b commented 6 years ago

I'll let you know when I have fixed the status message handling for the 2441V.

On Sun, Sep 9, 2018 at 12:50 PM Christopher Piggott cpiggott@gmail.com wrote:

I'm doing this in two steps. Step 1 was to create a base class so that we can support the concept of different insteon thermostats that are mostly the same. Step 2 is coming (I'm working on it now). There definitely are some differences, especially the read status message 0x2E. For example:

On the TH2441ZTH: message 1 returns data3=0, data4=localTempLowByte, data5=Humidity, data6=TempOffset, data7=HumiOffset, data8=SystemMode, data9=FanMode ....

On the 2441V message 1 returns: data3=mode, data4=humidity, data5=temperature, data6=cooling setpoint, data7=heating setpoint, data9=fan mode ...

The current insteom climate module doesn't make use of these 0x2E status messages, but it could (and I'd argue should). Especially when you are actually changing setpoints. In this situation it makes sense to me to be paranoid about polling the device from time to time to ensure it's set 1) to what you want it to be 2) to something sane so it won't burn down your house.

I'm open to suggestions or guidance on any of this, though. I'm gathering basedon the fact that no device was declared in ipdb for cat=0x05,subcat=0x03 that I'm the only one (so far) using the 2441V. Part of the purpose of my pull request is to carve out a place I could refine it while minimizing the chances of breaking somebody already successfully using a 2441TH.

(Note I made a rookie python mistake when I submitted this PR - I used super() but passed self - I fixed that but now it's telling me useless super() delegation so I need a few more minutes on this).

On Sun, Sep 9, 2018 at 10:58 AM Tom Harris notifications@github.com wrote:

@teharris1 commented on this pull request.

This indicates there is no difference between the 2441th and the 2441v. Is that correct?

In insteonplm/devices/climateControl.py https://github.com/nugget/python-insteonplm/pull/105#discussion_r216159876 :

@@ -100,3 +100,23 @@ def async_refresh_state(self):

pylint: disable=unused-argument

def _mode_changed(self, addr, group, val): self.async_refresh_state() + + +class ClimateControl_2441th(ClimateControl_Base):

  • """TH2441TH thermostat model."""
  • def init(self, plm, address, cat, subcat, product_key=None,

Don't need init if all it does is call super().init

In insteonplm/devices/climateControl.py https://github.com/nugget/python-insteonplm/pull/105#discussion_r216159885 :

+ + +class ClimateControl_2441th(ClimateControl_Base):

  • """TH2441TH thermostat model."""
  • def init(self, plm, address, cat, subcat, product_key=None,
  • description=None, model=None):
  • """Constructor, delegates most work to the base thermostat class."""
  • super().init(self, plm, address, cat, subcat, product_key,
  • description, model)

  • +class ClimateControl_2441v(ClimateControl_Base):

  • """TH2441V thermostat adapter model."""
  • def init(self, plm, address, cat, subcat, product_key=None,

Don't need init if all it does is call super.init()

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nugget/python-insteonplm/pull/105#pullrequestreview-153583855, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc2OSO7rfAYQH7a6_Zca6_71kDnTPy3ks5uZSyEgaJpZM4Wf4mN .

wz2b commented 6 years ago

The states themselves (in states/thermostat.py) have the message parsing in them, which I think is a problem because the extended status message that comes back from a 2441TH is different than a 2441V.

Even for the TH, I'm having problems really lining up what I see in the code vs. the developer's guide. 

        self._message_callbacks.add(temp_msg, self._temp_received)
        ext_status_recd = ExtendedReceive.template(
            commandtuple=COMMAND_EXTENDED_GET_SET_0X2E_0X00,
            cmd2=0x02,
            userdata=Userdata.template({"d1": 0x01}))
        self._message_callbacks.add(ext_status_recd,
                                    self._ext_status_received)

On a ZTH, when you get back a cmd1=0x2E, cmd2=0x02, Data1=0x01 you get back this:

The actual extended status message I think we want is cmd1=0x2e, cmd2=0x00, d2=0x01 which returns

Note again that's still different than what a 2441V returns.

I can go ahead and try to fix all of this, but I think the thing that has to happen is that the specific message parsing has to be removed from the states. Either that or the states need to call back into the device-specific model (e.g. ClimateControl_2441TH) and ask how to parse the messages.

I can't really proceed without some guidance.

2441THdev-062012-en.pdf

wz2b commented 6 years ago

After working through several scenarios I decided the best way to do this is to take all the message-parsing out of the states and let them live in the main module. This way you can have the same set of "states" shared among different types of thermostats and adapters.

To accomplish this, the states - for example Temperature - will have a set() method that takes a temperature value, in degrees. Then it's up to the individual Climate_XXX module to figure out how to receive the message (for example extended status message), parse it according to the device Developer Notes manual for that specific adapter, and call the more generic update_value method for that state. The ACK handlers will do the same, meaning the semantics of "Launch a set() request but don't update my state until I receive confirmation the device got the message" still holds.

Feedback on this is entirely welcome. If anyone has a 2441TH who is willing to help me test that I don't break that please advise. I did find one on ebay and made a low bid, just to have it for development.

wz2b commented 6 years ago

Why does State need group, linkdata1, linkdata2, linkdata3, is_controller, and is_responder? Why are these part of "State" ? Is there a good reason? The accessors don't seem to be called anywhere. I don't think they belong there. What am I missing?

teharris1 commented 6 years ago

Future plan for link management. Ultimately _state will be renamed _group. Just haven’t gotten around to it. For any device that has multiple states/groups, the _states list will be used to create the default links needed between the device and the modem (i.e. PLM or Hub). The properties you list are all needed for proper default link management.

From: Christopher Piggott notifications@github.com Sent: Monday, September 10, 2018 10:19:48 PM To: nugget/python-insteonplm Cc: Tom Harris; Comment Subject: Re: [nugget/python-insteonplm] WIP Create superclass for insteon thermostats (#105)

Why does State need group, linkdata1, linkdata2, linkdata3, is_controller, and is_responder? Why are these part of "State" ? Is there a good reason? The accessors don't seem to be called anywhere. I don't think they belong there. What am I missing?

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/nugget/python-insteonplm/pull/105#issuecomment-420123880, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIosHGZOThdXYcFs9Jy-gO0upB9FV5Hyks5uZx3EgaJpZM4Wf4mN.

teharris1 commented 6 years ago

The reason the message handling is inside State is because this makes it clear what messages effect what state of the device. Multiple states can listen/subscribe to a single message. The alternative is one method in the device needs to hand changes to each state that is effected or the state needs to listent/subscribe to a change made by the device. The problem with the device handing changes to the state is that the state is no longer controlling itself. This breaks encapsulation. Notice that today all state changes are managed by the state itself either through contained methods (i.e on(), off(), set_level(), etc.) or via a message received. I had really considered having the device be the master of all message intake but decided on the current method as a way to encapsulate the effect any message has on a single state. This also makes states more reusable across devices and as base states for more complex states.

wz2b commented 6 years ago

For any device that has multiple states/groups, the _states list will be used to create the default links needed between the device and the modem (i.e. PLM or Hub).

I feel like this breaks The Single Responsibility Principle of S.O.L.I.D. The fact that needed to be explained kind of shows that - when I saw it, I thought "Why is this here?". If you haven't had time to finish, might you reconsider this? At least use the wiki to explain the OOP responsibilities of a State vs. a Device, then take a step back and see where you're going still makes sense.

I had really considered having the device be the master of all message intake but decided on the current method as a way to encapsulate the effect any message has on a single state.

I see. In terms of single responsibility, I think Temperature and Humidity are their own things: a subclass of State:

An object is an entity that has state, behavior, and identity. The structure and behavior of similar objects are defined in their common class. The terms instance and object are interchangeable. Booch

and being common I'd like to reuse it between devices. The problem is with these thermostat status messages is that they conflict between the TH, ZTH, and V.

The reason the message handling is inside State is because this makes it clear what messages effect what state of the device.

True, but look at it the other way: you spread the "protocol" out across multiple files. In the case of the status message, you actually spread the handling for that message across multiple classes. I found it more confusing because I think they are really two different concepts:

I'm not saying that the state objects necessarily have to be dumb osbservable beans (though I don't see the problem with that) but I feel like these are different responsibilities.

For any device that has multiple states/groups, the _states list will be used to create the default links needed between the device and the modem

I have until now been managing links externally. I wouldn't mind seeing things combined in some way but you haven't convinced me that it belongs in State. You're giving State yet a third job to do. I am separately thinking about what happens when you persist the state vs. persist the links. And I think the links are a facet of the protocol, not of "Humidity - an object that represents the humidity in a particular room."

Is this good discussion? I'm operating along the spirit of you having said:

I would love to see how someone else can work with my code and create one of the more complex devices.

which means feedback and discussion. Hope that's what you intended.

teharris1 commented 6 years ago

Yes, this is exactly what I was hoping for. The only thing is perhaps we can take it out of this PR and make it an issue or part of a discussion thread somewhere, given that we are getting more into discussions that should stay persistent with the project. Let me think about where that should go.

First let me admit that the default link data sitting in the State class was not meant to hit the master branch. It got there by accident because I figured out while codeing the thermostat that default links with the termostat are critical to the overall success of that device. So that code was just playing around. In then decided to take a step back and fix a bunch of persistent lint issue and accidently used the branch that I had been playing with default links. (Hense the value of code reviews to find silly mistakes like that.)

So you make a lot of really good points and I see what you mean regarding State being too extensive in it's purpose. I am not convinced however, that it is best to bring it up to the Device level. I am not sure how that actually solves your concern but only moves the concern up to the device level. What then is the purpose of the Device object?

I have looked at a lot of Insteon implementations and the one that I modeled this most closely to is openHAB. Most other implemetations put everthing on the device. This can be very clean if you just want to turn a light on or off. But when you look at the more complex devices it becomes a lot more difficult to have a consistant implementation and the Device class winds up being just a collection of methods. In the openHAB model, a device is a collection of features. A feature a message handler and command generator. So in many respects the State class is similar to the openHAB feature. I do agree with you that I probably put too much on the State class at this time. But I also think I have put too much on the Device class too.

At the end of the day, I am not emotionally tied to any of the architecture of this module. I have known for a while that I need to rearchitect this to make it easier for future developers but have not sat down to actually do it. And give that it is now supporting Hubs, it is no longer just a PLM module. I have recently reserved the pyinsteon name in GitHub and am looking to post a dummy module to pypi to reserve the name there. That will become the 1.0.0 version of this module.

There are only a few things I feel strongly about with the design of this module: 1) The module is build for consistent and reliable handling of Insteon devices. I am choosing reliability over speed. This is a really important point. There are a lot of things that could be done to make this library faster and the advantage of that would be to allow this module to handle your Christmas light display. With the current design, that would not be possible. It is too slow. But it is consistent and will run your house 24x7x365 without issue. Perhaps in the future speed can be a programmer setting but for now consistency and reliability is the goal. 2) Message intake has to be truly asyncronous. Most, if not all, implementations I have looked at assume a send/respond pattern that is not valid. (i.e. I send an 'on' message, get a message ACK so it must be good. Or I send an 'on' message, get a message ACK and the next message must be a device ACK.). At the end of the day, the underlying protocol is truly asyncronous (albeit in a serial model), and messages can appear without warning when the device is itneracted with by the human. 3) Devices must be discoverable and not require a config file to define them. While I gave in to this with battery opperated devices, that was just due to assuming that the user was coming from another piece of software and the link was set up already. If you do link management correctly the device is always known. Additionally, even battery operated devices can be discovered if you know how to talk to them (i.e wait for them to talk first, then ask the question.

A fourth goal is asperational and I could be talked out of ut but: 4) Devices should be implemented in a consistent pattern regardless of the device complexity. This will allow programmers (who are the real users of this module) to discover the device's capabilities rather than have to know them to implement them. This one is more aspirational right now but hoping to get there.

wz2b commented 6 years ago

I am trying to figure this out if we could use this but it sounds like you'd have to create an 'organization' first

https://blog.github.com/2017-11-20-introducing-team-discussions/

On Tue, Sep 11, 2018 at 9:32 AM Tom Harris notifications@github.com wrote:

Yes, this is exactly what I was hoping for. The only thing is perhaps we can take it out of this PR and make it an issue or part of a discussion thread somewhere, given that we are getting more into discussions that should stay persistent with the project. Let me think about where that should go.

First let me admit that the default link data sitting in the State class was not meant to hit the master branch. It got there by accident because I figured out while codeing the thermostat that default links with the termostat are critical to the overall success of that device. So that code was just playing around. In then decided to take a step back and fix a bunch of persistent lint issue and accidently used the branch that I had been playing with default links. (Hense the value of code reviews to find silly mistakes like that.)

So you make a lot of really good points and I see what you mean regarding State being too extensive in it's purpose. I am not convinced however, that it is best to bring it up to the Device level. I am not sure how that actually solves your concern but only moves the concern up to the device level. What then is the purpose of the Device object?

I have looked at a lot of Insteon implementations and the one that I modeled this most closely to is openHAB. Most other implemetations put everthing on the device. This can be very clean if you just want to turn a light on or off. But when you look at the more complex devices it becomes a lot more difficult to have a consistant implementation and the Device class winds up being just a collection of methods. In the openHAB model, a device is a collection of features. A feature a message handler and command generator. So in many respects the State class is similar to the openHAB feature. I do agree with you that I probably put too much on the State class at this time. But I also think I have put too much on the Device class too.

At the end of the day, I am not emotionally tied to any of the architecture of this module. I have known for a while that I need to rearchitect this to make it easier for future developers but have not sat down to actually do it. And give that it is now supporting Hubs, it is no longer just a PLM module. I have recently reserved the pyinsteon name in GitHub and am looking to post a dummy module to pypi to reserve the name there. That will become the 1.0.0 version of this module.

There are only a few things I feel strongly about with the design of this module:

  1. The module is build for consistent and reliable handling of Insteon devices. I am choosing reliability over speed. This is a really important point. There are a lot of things that could be done to make this library faster and the advantage of that would be to allow this module to handle your Christmas light display. With the current design, that would not be possible. It is too slow. But it is consistent and will run your house 24x7x365 without issue. Perhaps in the future speed can be a programmer setting but for now consistency and reliability is the goal.
  2. Message intake has to be truly asyncronous. Most, if not all, implementations I have looked at assume a send/respond pattern that is not valid. (i.e. I send an 'on' message, get a message ACK so it must be good. Or I send an 'on' message, get a message ACK and the next message must be a device ACK.). At the end of the day, the underlying protocol is truly asyncronous (albeit in a serial model), and messages can appear without warning when the device is itneracted with by the human.
  3. Devices must be discoverable and not require a config file to define them. While I gave in to this with battery opperated devices, that was just due to assuming that the user was coming from another piece of software and the link was set up already. If you do link management correctly the device is always known. Additionally, even battery operated devices can be discovered if you know how to talk to them (i.e wait for them to talk first, then ask the question.

A fourth goal is asperational and I could be talked out of ut but: 4) Devices should be implemented in a consistent pattern regardless of the device complexity. This will allow programmers (who are the real users of this module) to discover the device's capabilities rather than have to know them to implement them. This one is more aspirational right now but hoping to get there.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nugget/python-insteonplm/pull/105#issuecomment-420275662, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc2Oar9-9yP6ClCOD82Ykn5HWym0BCcks5uZ7tZgaJpZM4Wf4mN .

teharris1 commented 6 years ago

https://github.com/orgs/pyinsteon/teams/pyinsteon

pyinsteon is an org.

From: Christopher Piggott notifications@github.com Sent: Tuesday, September 11, 2018 10:35:41 AM To: nugget/python-insteonplm Cc: Tom Harris; Comment Subject: Re: [nugget/python-insteonplm] WIP Create superclass for insteon thermostats (#105)

I am trying to figure this out if we could use this but it sounds like you'd have to create an 'organization' first

https://blog.github.com/2017-11-20-introducing-team-discussions/

On Tue, Sep 11, 2018 at 9:32 AM Tom Harris notifications@github.com wrote:

Yes, this is exactly what I was hoping for. The only thing is perhaps we can take it out of this PR and make it an issue or part of a discussion thread somewhere, given that we are getting more into discussions that should stay persistent with the project. Let me think about where that should go.

First let me admit that the default link data sitting in the State class was not meant to hit the master branch. It got there by accident because I figured out while codeing the thermostat that default links with the termostat are critical to the overall success of that device. So that code was just playing around. In then decided to take a step back and fix a bunch of persistent lint issue and accidently used the branch that I had been playing with default links. (Hense the value of code reviews to find silly mistakes like that.)

So you make a lot of really good points and I see what you mean regarding State being too extensive in it's purpose. I am not convinced however, that it is best to bring it up to the Device level. I am not sure how that actually solves your concern but only moves the concern up to the device level. What then is the purpose of the Device object?

I have looked at a lot of Insteon implementations and the one that I modeled this most closely to is openHAB. Most other implemetations put everthing on the device. This can be very clean if you just want to turn a light on or off. But when you look at the more complex devices it becomes a lot more difficult to have a consistant implementation and the Device class winds up being just a collection of methods. In the openHAB model, a device is a collection of features. A feature a message handler and command generator. So in many respects the State class is similar to the openHAB feature. I do agree with you that I probably put too much on the State class at this time. But I also think I have put too much on the Device class too.

At the end of the day, I am not emotionally tied to any of the architecture of this module. I have known for a while that I need to rearchitect this to make it easier for future developers but have not sat down to actually do it. And give that it is now supporting Hubs, it is no longer just a PLM module. I have recently reserved the pyinsteon name in GitHub and am looking to post a dummy module to pypi to reserve the name there. That will become the 1.0.0 version of this module.

There are only a few things I feel strongly about with the design of this module:

  1. The module is build for consistent and reliable handling of Insteon devices. I am choosing reliability over speed. This is a really important point. There are a lot of things that could be done to make this library faster and the advantage of that would be to allow this module to handle your Christmas light display. With the current design, that would not be possible. It is too slow. But it is consistent and will run your house 24x7x365 without issue. Perhaps in the future speed can be a programmer setting but for now consistency and reliability is the goal.
  2. Message intake has to be truly asyncronous. Most, if not all, implementations I have looked at assume a send/respond pattern that is not valid. (i.e. I send an 'on' message, get a message ACK so it must be good. Or I send an 'on' message, get a message ACK and the next message must be a device ACK.). At the end of the day, the underlying protocol is truly asyncronous (albeit in a serial model), and messages can appear without warning when the device is itneracted with by the human.
  3. Devices must be discoverable and not require a config file to define them. While I gave in to this with battery opperated devices, that was just due to assuming that the user was coming from another piece of software and the link was set up already. If you do link management correctly the device is always known. Additionally, even battery operated devices can be discovered if you know how to talk to them (i.e wait for them to talk first, then ask the question.

A fourth goal is asperational and I could be talked out of ut but: 4) Devices should be implemented in a consistent pattern regardless of the device complexity. This will allow programmers (who are the real users of this module) to discover the device's capabilities rather than have to know them to implement them. This one is more aspirational right now but hoping to get there.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/nugget/python-insteonplm/pull/105#issuecomment-420275662, or mute the thread https://github.com/notifications/unsubscribe-auth/ABc2Oar9-9yP6ClCOD82Ykn5HWym0BCcks5uZ7tZgaJpZM4Wf4mN .

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/nugget/python-insteonplm/pull/105#issuecomment-420296499, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AIosHD74mulDeP3MYHnijwyQDxYJrZayks5uZ8o9gaJpZM4Wf4mN.