Added feature: Climate React Auto Setup plus minor Bugfix

[seidnerj]

See README changes for more details.

[seidnerj]

@benwebbbenwebb ? @nitaybz ? Any takers?

[benwebbbenwebb]

I’ll take a look over the weekend @seidnerj

I do however suspect that there may be some conflicts with the v2.4 alpha branch which we’ll need to work through (it has code to disable AUTO)

[seidnerj]

Great, much appreciated!

[seidnerj]

There's a few fixes I'm adding to this pull request so no need to review it for now. I'll update this thread when everything's ready.

Thanks in advance!

[benwebbbenwebb]

Hi @seidnerj

I know you're still working on this but I took a look and noticed that there is likely going to be merge conflicts when I push my development branch (my fault for not merging it towards main/master much, much sooner... got lazy as I was the only one active in the repo) that some users are alpha testing.

There is also feature cross-over with your AUTO mode suppression, I went whole hog and setup an array so users could exclude any and all modes (see readme example in the link above). I was thinking of deprecating the use of disableX-mode in the future.

That all said, I haven't done anything similar to your ClimateReact changes. So I'm wondering if I should do a sort of cherry-pick (it may need to be manual...) of those in to (my) development branch, and then probably push to main/master (without a release just yet), to get us in sync?

Keen for your thoughts on the best way to proceed!

[seidnerj]

Hey @benwebbbenwebb! Sure - depending on the amount of changes in your branch, it might be easier for me (or you?) to merge those changes onto my fork and then push it to main/master.

On a related note, I only have one small issue that I'm dealing with but I can't seem to track it. The issue is that when the ClimateReact Switch changes state, this has no effect, the code reaches "StateManager.set.ClimateReactEnabledSwitch" but StateHandler.set never follows.

Would love for you to take a look and give me a clue as to what is wrong. I've compared it to the Light Switch accessory which kinda has identical functionality (and does work), and I didn't notice any difference.

Thanks in advance!

[benwebbbenwebb]

Hi @seidnerj

Sure - depending on the amount of changes in your branch, it might be easier for me (or you?) to merge those changes onto my fork and then push it to main/master.

My branch compared to master, I made some linting changes which doesn't help with the comparison unfortunately, but there is quite a bit there. Your fork (master) compared to this repos master

I could do a merge/PR of my branch to master and then we check where that leaves this PR?

As for your question:

On a related note, I only have one small issue that I'm dealing with but I can't seem to track it. The issue is that when the ClimateReact Switch changes state, this has no effect, the code reaches "StateManager.set.ClimateReactEnabledSwitch" but StateHandler.set never follows.

I didn't pull your full changes to check, but I did note that you have the exact same service name for the ClimateReactService in both the AirConditioner and the ClimateReactSwitch files, wondering if there is a clash. You might need to clear out all cached Accessories manually from Homebridge as a starting point to be sure.

I also noted that both reference the same ClimateReactEnabledSwitch get/set functions in StateManager.js... probably should be fine (but I've also not used the ClimateReact, LightSwitch or SyncButton services to know if there is a common issue when the same setter is referenced from multiple Accessories).

[benwebbbenwebb]

Oh, and I think maybe you also missed adding changes to updateHomeKit within AirConditioner.js to push any change back to the Home app UI (similar to what is here: https://github.com/seidnerj/homebridge-sensibo-ac/blob/2a35793d6718be02c571e1083cbb6d57e0132f78/homekit/ClimateReactSwitch.js#L67)

[seidnerj]

Hey @benwebbbenwebb, Yes - if you could do a merge/PR of your branch to master that will be a good step towards merging this PR as well. Thanks!

Re you comments regarding having the same exact service name in two separate file / using same StateManager functions. I I followed the example set by the "LightSwitch" accessory implementation, which also uses the same name/functions. I haven't seen any issues that seem to be related to this, so I believe it is okay.

Re the "updateHomeKit" function, really the only state that's reflected in the Home app is whether Climate React is enabled or not, which the current code already takes care off. I can't think of what else should be there.

Hi @seidnerj

Sure - depending on the amount of changes in your branch, it might be easier for me (or you?) to merge those changes onto my fork and then push it to main/master.

My branch compared to master, I made some linting changes which doesn't help with the comparison unfortunately, but there is quite a bit there. Your fork (master) compared to this repos master

I could do a merge/PR of my branch to master and then we check where that leaves this PR?

As for your question:

On a related note, I only have one small issue that I'm dealing with but I can't seem to track it. The issue is that when the ClimateReact Switch changes state, this has no effect, the code reaches "StateManager.set.ClimateReactEnabledSwitch" but StateHandler.set never follows.

I didn't pull your full changes to check, but I did note that you have the exact same service name for the ClimateReactService in both the AirConditioner and the ClimateReactSwitch files, wondering if there is a clash. You might need to clear out all cached Accessories manually from Homebridge as a starting point to be sure.

I also noted that both reference the same ClimateReactEnabledSwitch get/set functions in StateManager.js... probably should be fine (but I've also not used the ClimateReact, LightSwitch or SyncButton services to know if there is a common issue when the same setter is referenced from multiple Accessories).

[benwebbbenwebb]

Hi @seidnerj

I've done the merge to master #112 and (as expected) there is unfortunately some merge conflicts :(

Let me know if I can be of any help resolving them

[seidnerj]

No worries, I will take a look and let you know if I need any help 🙏

[benwebbbenwebb]

Re you comments regarding having the same exact service name in two separate file / using same StateManager functions. I I followed the example set by the "LightSwitch" accessory implementation, which also uses the same name/functions. I haven't seen any issues that seem to be related to this, so I believe it is okay.

I think you're probably correct on that...

Re the "updateHomeKit" function, really the only state that's reflected in the Home app is whether Climate React is enabled or not, which the current code already takes care off. I can't think of what else should be there.

I still recommend you try adding the updateValue... at the moment it sounds like the .on('set'... isn't being fired (is that true from what you can see via debug logs?) and if so that's probably because the value isn't changing anywhere for StateHander to see it?

I'm also conscious that you are updating values one level deeper than all the other calls... e.g. device.state.smartMode.enabled rather than device.state.pureBoost. Not sure if that is impactful, the state handling was all written by nitaybz long before I started tinkering, so maybe that's another place to check (and eliminate)?

[seidnerj]

I've done the merge with master manually but haven't tested it yet. I'm sure there's going to be issues. I will also remove the disable auto feature as this is now duplicate functionality. Will probably take me a few days to get this sorted. I'll update the thread when I have news. Thx.

[seidnerj]

Hello @seidnerj

I know you're still working on it but I've done a first pass and added some suggestions and comments.

Please take a look when you have a chance.

Thanks!

I've fixed all pertinent the issues you raised in this latest commit.

[benwebbbenwebb]

I've fixed all pertinent the issues you raised in this latest commit.

Thanks! Sorry for the hassle. 🙇

Before I do another (maybe final?) review, please run a lint check. You may have already done this, if so, let me know.

npm run lint (or npm run lint:fix if you're happy for eslint to 'fix' it on the fly).

Also, did you figure out your issue mentioned above and successfully test your changes?

[seidnerj]

Re the linting - will do. I haven't yet tested the post-merge version and checked that everything works.

Once I've done that, I will resolve the issue of the climate react switch not firing.

[seidnerj]

I did another pass and committed the changes.

Merging this PR to the current master branch should be relatively easy now. Merging your code with mine to create this PR was a much bigger issue. So, re your question, I think you can go ahead and merge this PR to the master branch and we can continue testing there then after completion we could do a release.

Once you've done the merge, I would appreciate any help testing this as this has become a much bigger deal than I expected (I wasn't expecting this mega merge) and I'm not sure when I'll be able to complete this.

[seidnerj]

@benwebbbenwebb, any progress on merging this to master? thanks!

[benwebbbenwebb]

Hi @seidnerj

Apologies for the delay, misunderstood previous comments and was waiting for any additional changes on ClimateReact.

Merged now.

[seidnerj]

Awesome, thanks! I'll get crackin'

[seidnerj]

Okay, so - I tested the current master. The good news is that everything pretty much seems to work. The bad news is that the climate react enable/disable switch still doesn't work.

Here's a comparison of the behavior from the logs:

This is what happens when the AC is turned on:

[05/11/2023, 14:08:01] [homebridge-sensibo-ac] Living Room AC (SET) - AC Active State: true
[05/11/2023, 14:08:01] [homebridge-sensibo-ac] StateHandler SET active true for ...

This is what happens when climate react is turned on:

[05/11/2023, 14:07:32] [homebridge-sensibo-ac] Living Room AC (SET) - Climate React Enabled Switch: true

The line where it should say "StateHandler SET ...." is missing. It seems that it just doesn't get to StateHandler.set for some reason. I compared the existing code to the previous version and I couldn't find any change that would cause this behavior. I'm starting to wonder whether or not this worked before I got to work on this PR.

For the love of me, I can't figure out what's the problem there. Any help would be appreciated! 🙏

[seidnerj]

@benwebbbenwebb any chance you could take a peek re the climate react switch issue?

[benwebbbenwebb]

Hey @seidnerj Will do, might take a day or so but I’ll let you know (will also compare with v2.3.4)

[benwebbbenwebb]

@benwebbbenwebb any chance you could take a peek re the climate react switch issue?

Hi @seidnerj

Can confirm that on v2.4.0-alpha.5 (what my main Homebridge is running currently) ClimateReact works in Home app and I can see the status update (e.g. it turns ClimateReact on/off) in the Sensibo app.

Will check on your implementation of the switch as a Service attached to the AC when I can.

[seidnerj]

Great, many thanks!

[benwebbbenwebb]

Hi @seidnerj

Please see 0536481, I'm not really sure why (it was implemented before I took over), but looks like state change detection doesn't work 2 levels deep (smartMode.enabled). Keeping it at a single level (smartModeEnabled) is firing the set in StateHandler as expected...

Not sure what that means for the rest of your implementation, but it should allow you to progress at least.

Note, in my commit I didn't make changes to things like prop === 'smartMode' (line 79 of StateHandler), but when I tested it locally (by changing it to smartModeEnabled) the API call was triggered and Sensibo app updated.

Will leave it with you for now!

[seidnerj]

I think I got why! thanks a lot, I'll take over - much appreciated 🙏

[seidnerj]

Finally fixed this issue. I also documented in a comment what was the issue. I'l open a separate PR for you to review.