Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add 2441V thermostat device for temperature weirdness #65

Closed
wants to merge 1 commit into from

Conversation

jlambert121
Copy link

Resolves #64

This PR resolves the issue I am having where the 2441V thermostat is returning the actual temperature in cmd2 instead of needing to divide by 2.

2021-01-06 01:55:12,120 DEBUG pyinsteon.messages RX: msg_id: 0x50, address: 14f855, target: 34788c, flags: 0x0f, cmd1: 0x6e, cmd2: 0x3d
2021-01-06 01:55:12,120 INFO samples Topic: 14f855.thermostat_temperature_status.direct data: {'cmd1': 110, 'cmd2': 61, 'target': 34788c, 'user_data': None, 'hops_left': 3}
2021-01-06 01:55:12,120 INFO samples Topic: handler.14f855.thermostat_temperature_status.direct data: {'degrees': 61}
2021-01-06 01:55:12,121 INFO samples Topic: state_14f855_temperature_10 data: {'name': 'temperature', 'address': '14f855', 'value': 61.0, 'group': 10}

I added the device name to the classes I added because I wasn't sure what else to call it other than "_why_oh_why_venstar" which also didn't seem like a good name.

@jlambert121
Copy link
Author

It looks like the 2441V has a lot of differences on the command set from the 2441TH that's currently coded. I've been continuing to look at this when I get time, and ran across this this morning:

Topic: 14f855.extended_get_response.direct data: {'cmd1': 46, 'cmd2': 0, 'target': 34788c, 'user_data': 01.01.01.09.41.4b.44.02.00.00.00.00.00.ae, 'hops_left': 1}
Topic: handler.14f855.extended_get_response.direct data: {'group': 1, 'data': OrderedDict([('data3', 1), ('data4', 9), ('data5', 65), ('data6', 75), ('data7', 68), ('data8', 2), ('data9', 0), ('data10', 0), ('data11', 0), ('data12', 0), ('data13', 0), ('data14', 174)])}

which is all accurate according the 2441v docs (page 8), but that return format is completely different than the 2441TH (page 14) response for a status request. I have a feeling that the set commands are going to be the same as well.

I'm more than happy to finish up a PR to add this thermostat type, but if you could point me to another device to model this off of I'd appreciate it - this is more complex python than I'm used to so modeling after something helps a lot! Also if you have any opinions on naming - that's always a hard topic! :)

@jlambert121
Copy link
Author

I believe all the commands for this are now working. Most of the changes seem reasonable, but the device_type change seems a bit more copy pasta than is good, but I'm also not sure how to improve it without refactoring the thermostat a whole lot more (which is also probably beyond my skillset).

@jlambert121
Copy link
Author

@teharris1 trying not to harass you, but would you be able to give me some feedback on this so I can make whatever changes I need to and get this merged in?

@teharris1
Copy link
Collaborator

@jlambert121 Sorry I had drafted a response and never hit comment. Thank you very much for this PR. Thermostats are a tough device so it is easy to see why you had issues. I agree with your concern there is a lot of copy and paste in this and there is an attempt to modify existing classes to adjust to the 2441v. I would suggest you do the following:

  1. Create a ThermostatBase that includes all base methods for a thermostat class that all thermostats can derive from.
  2. If a handler is different than the current handlers, please use a separate file for them rather than putting them into the same file. I am going to be moving to a model of one file has only one class. This is currently violated in the device_types module but I am going to change that soon.

If you can make those changes, I will review it in detail and give more feedback.

@jlambert121
Copy link
Author

I started on this this morning and afraid I might make more of a mess trying to separate these apart and move stuff around than the value I'd bring - probably best if you do that if it was already on your list. Maybe the climate/thermostat stuff would be a good place to start (selfishly :) ) and then I can rework this onto that pattern?

@mstovenour
Copy link
Contributor

@teharris1 I can try to help split this out if you'd like, but I don't have the device for testing. I see four new classes in this PR:

class ClimateControl_Thermostat_2441V(Device):
class ThermostatStatusResponseHandler2441V(InboundHandlerBase):
class ThermostatTemperatureHandler2441V(InboundHandlerBase):
class GetThermostatStatus2441V:

Are you asking that all four be separated into base / derived classes? Should all thermostats require a derived class so base is only used for inheritance?

E.g.

class ClimateControl_Thermostat_base(Device):
class ClimateControl_Thermostat_Generic(ClimateControl_Thermostat_base):
class ClimateControl_Thermostat_Wireless(ClimateControl_Thermostat_base):**
class ClimateControl_Thermostat_2441V(ClimateControl_Thermostat_base):

All existing devices using ClimateControl_Thermostat would use ClimateControl_Thermostat_Generic. Then repeat this for all four type of classes as necessary?

**Note: this is a class name change from ClimateControl_WirelessThermostat to ClimateControl_Thermostat_Wireless

Anyway, mock that up and let me know what you prefer and I'll make the changes. I'm not too sure how to get it all tested though.

@teharris1 teharris1 changed the base branch from master to dev May 17, 2021 13:57
@jlambert121 jlambert121 force-pushed the 2441v_temp branch 2 times, most recently from 3e746ed to ccee02b Compare December 24, 2021 20:01
@jlambert121
Copy link
Author

@teharris1 I haven't tested this at all, but is this more in line with what you were looking to see?

@jlambert121
Copy link
Author

Replaced this thermostat with a different one.

@zunderscore
Copy link

If anyone is willing to continue the work on this, I have a 2441V I can test with. I'd love to get it working here so the changes could be integrated into Home Assistant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2441v Thermostat recording wrong temperature
4 participants