WIP Create superclass for insteon thermostats #105
Conversation
wz2b
commented
Sep 8, 2018
wz2b
commented
- Create a subclass for insteon thermostats and split 2441TH and 2441V devices. Currently these are identical but they won't be for long, as I don't want to introduce "universal" additions that I can't test against a device I don't have
- In the IPDB, removed the "product code" matching requirement from ipdb for the 2441V adapter. The documentation says 0x0000001F is correct but that's not what the device actually sends.
- In climate control device model, fixed references to self._update_method that should have been self._updatemethod. Long term this should be either added to the constructor or made into a proper property, but doing that means touching a lot of device/state code that I can't currently test
…low creating an interface then manually triggering an aldb refresh later
…ances, one for the 2441TH (which already existed) and one for the 2441V (currently adding).
…ances, one for the 2441TH (which already existed) and one for the 2441V (currently adding).
of the Insteon thermostat. Currently they are identical, but they won't be shortly. Fixed an apparent bug in states/thermostat where it referred to _update_method when the instance member of state is actually named _updatemethod. Note that this really should either be included in the constructor (best choice) or added as a proper property. If the convention is that things starting with _ are private to the class, then external classes shouldn't be reading or writing them.
…houdln't have been there - that was intended for the backlight configuration command that isn't debugged yet
… to spacing and line length more to satisfy pylint, and not make last-second changes just prior to commit without running tox first.
| class ClimateControl_2441th(ClimateControl_Base): | ||
| """TH2441TH thermostat model.""" | ||
|
|
||
| def __init__(self, plm, address, cat, subcat, product_key=None, |
There was a problem hiding this comment.
Don't need init if all it does is call super().init
| class ClimateControl_2441v(ClimateControl_Base): | ||
| """TH2441V thermostat adapter model.""" | ||
|
|
||
| def __init__(self, plm, address, cat, subcat, product_key=None, |
There was a problem hiding this comment.
Don't need init if all it does is call super.init()
|
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 ***@***.***> wrote:
***@***.**** 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
<#105 (comment)>
:
> @@ -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
<#105 (comment)>
:
> +
+
+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
<#105 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABc2OSO7rfAYQH7a6_Zca6_71kDnTPy3ks5uZSyEgaJpZM4Wf4mN>
.
|
|
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 ***@***.***>
wrote:
> ***@***.**** 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
> <#105 (comment)>
> :
>
> > @@ -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
> <#105 (comment)>
> :
>
> > +
> +
> +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
> <#105 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABc2OSO7rfAYQH7a6_Zca6_71kDnTPy3ks5uZSyEgaJpZM4Wf4mN>
> .
>
|
|
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. 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
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. |
teharris1
changed the title
|
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. |
|
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? |
|
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 GitHub<#105 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AIosHGZOThdXYcFs9Jy-gO0upB9FV5Hyks5uZx3EgaJpZM4Wf4mN>.
|
|
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. |
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 see. In terms of single responsibility, I think Temperature and Humidity are their own things: a subclass of State:
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.
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.
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:
which means feedback and discussion. Hope that's what you intended. |
|
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 There are only a few things I feel strongly about with the design of this module:
A fourth goal is asperational and I could be talked out of ut but: |
|
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 ***@***.***> 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
<#105 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABc2Oar9-9yP6ClCOD82Ykn5HWym0BCcks5uZ7tZgaJpZM4Wf4mN>
.
|
|
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 ***@***.***> 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
<#105 (comment)>,
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 GitHub<#105 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AIosHD74mulDeP3MYHnijwyQDxYJrZayks5uZ8o9gaJpZM4Wf4mN>.
|
