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

Climate React Auto Setup bug fix #122

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Climate React Auto Setup bug fix #122

merged 2 commits into from
Dec 16, 2023

Conversation

seidnerj
Copy link
Contributor

AC state setting change should not re-enable Climate React if Auto Setup is disabled.

AC state setting change should not re-enable Climate React if Auto Setup is disabled.
@seidnerj
Copy link
Contributor Author

Still haven't got around to doing this. Hopefully in the next few days.

Essentially, this commit simply removes code that forcefully enabled Climate React on every AC state change.

This feature will now be limited in scope to adjusting the Climate React _settings_ (but NOT enabling/disabling Climate React mode, this should be done by the user directly).
@seidnerj
Copy link
Contributor Author

I think that did it. It was easier than I thought once I took a better look at the code.

@benwebbbenwebb
Copy link
Collaborator

Great, I’ll take a look over the weekend @seidnerj

@benwebbbenwebb benwebbbenwebb merged commit b962009 into nitaybz:master Dec 16, 2023
1 check passed
@benwebbbenwebb
Copy link
Collaborator

benwebbbenwebb commented Dec 16, 2023

Hello @seidnerj

I've done the merge.

I do have a concern though... I know the current SmartMode (ClimateReact) state will get pulled from Sensibo API every refresh (90ish seconds).

However, what happens if during the 90 second window the user changes their AC state via app? I believe the local copy of state.smartMode (e.g. acState in unified.js) will then get overwritten by updateClimateReact() in StateManager.js... called from within the set functions in StateManager.js (e.g. ACActive). That would update the local state.smartMode to new values, right?

And then doesn't that state.smartMode change get pushed back to Sensibo via prop === 'smartMode' in StateHandler.js?

I know it won't turn SmartMode (ClimateReact) on or off without explicit user action via ClimateReactEnabledSwitch, but I think it will still change the SmartMode settings which could be unexpected from the user point of view.

I've also noted that after this merge in the master (main) branch enableClimateReactAutoSetup doesn't appear to be used anywhere...

benwebbbenwebb pushed a commit that referenced this pull request Dec 16, 2023
* Climate React Auto Setup bug fixes

AC state setting change should not re-enable Climate React if Auto Setup is disabled.
Removes code that forcefully enabled Climate React on every AC state change.
This feature will now be limited in scope to adjusting the Climate React _settings_ (but NOT enabling/disabling Climate React mode, this should be done by the user directly).
@seidnerj
Copy link
Contributor Author

seidnerj commented Dec 16, 2023

You're right about enableClimateReactAutoSetup not being used, it was probably lost in one of the merges. I opened a new PR with the appropriate fix. Regarding the other issue you mentioned - I will take a look and update this thread (as well as the new PR I already opened).

@seidnerj
Copy link
Contributor Author

I thought about what you wrote re the refresh, I'm not sure I completely followed but assuming I did, I don't see a problem here. Assuming climate react auto setup is enabled, there are two scenarios here:

  1. AC state is updated via the plugin --> updateClimateReact() --> state.smartMode gets pushed back to Sensibo --> state.SmartMode's local copy and remote copy are in sync.

  2. AC state is updated via the app --> (after ~90s max) refresh happens --> state.SmartMode's local copy and remote copy are in sync.

A state changed that's done via the app will not affect the plugin's local copy of the state.SmartMode until ~90 seconds (max) have passed. When that eventually happens, updateClimateReact() will not be executed because it is not triggered by refreshState() since refreshState() does not update the state via the setters in StateManager.js but rather simply overwrites the local copy with a new object. So, in essence, a state changed triggered via the app does not trigger updateClimateReact() and so state.SmartMode local's copy will not be overwritten.

I think if anything, updateClimateReact() should in fact probably get called after every refresh so that any Climate React's settings would get updated to the new AC state that got pulled from Sensibo's servers in case a user opted to make changes to the AC state via the app vs. the plugin.

Does the above make sense?

@benwebbbenwebb
Copy link
Collaborator

benwebbbenwebb commented Dec 17, 2023

Hi @seidnerj

I think you and I are on the same page but I was talking about the reverse situation (local state change being sent to Sensibo ClimateReact unexpectedly), which you should now have resolved via #123

Thanks!

I have a few package version bumps etc in dev-v2.5 branch that I’ll finish soon and then make an alpha release.

benwebbbenwebb pushed a commit that referenced this pull request Jan 10, 2024
* Climate React Auto Setup bug fixes

AC state setting change should not re-enable Climate React if Auto Setup is disabled.
Removes code that forcefully enabled Climate React on every AC state change.
This feature will now be limited in scope to adjusting the Climate React _settings_ (but NOT enabling/disabling Climate React mode, this should be done by the user directly).
benwebbbenwebb added a commit that referenced this pull request Jan 20, 2024
### Bug fixes and improvements
- #105 fan setting not remembered, round HKToFanLevel to match fanLeveltoHK
- StateHandler.js - Better handle async (WIP) API calls, especially for smartMode
- StateHandler.js - Correctly manage smartMode state changes
- api.js - Better error handling
- api.js - updated fixResponse to prevent unexpectedly adding smartMode
- README.md changes
  - Climate React feature descriptions
  - Sensor descriptions
  - added deprecations
  - general clean up
- config and config.schema updates
- move StateHandler.js
- move HKToFanLevel, toFahrenheit, sensiboFormattedACState & sensiboFormattedClimateReactState to (new) StateHandler.js
- add Utils.js and move to using Utils.updateValue()
- Additional logging and TODOs
- Only add Humidity to AC when enabled

### Security
- Bump axios from 0.21.4 to 1.6.5
- Bump follow-redirects to 1.15.4

### Chores and maintenance
- update package.json dependency and engine versions
- Axios major version upgrade
- remove qs (functionality now provided by Axios directly)
- move to eslint stylistic (from eslint) for linting
- ESLint flat file config (v9 ready)
- add/update GitHub workflow to check linting on PRs
- update .gitignore and .npmignore
- move nodemon to own config
- TODOs, comments and debug log changes
- Tidied up / unified accessory definition files


---------

Co-authored-by: seidnerj (PRs #119 & #122, #123 back merged)
@benwebbbenwebb
Copy link
Collaborator

Hi @seidnerj

I've made the alpha release with your ClimateReact changes, please take a look when you have some time.

@seidnerj
Copy link
Contributor Author

Sure, anything specific you need want me to look at?

@benwebbbenwebb
Copy link
Collaborator

Sure, anything specific you need want me to look at?

Mostly that your new features are working as expected before I promote to beta / release.

Thanks @seidnerj

@seidnerj
Copy link
Contributor Author

@benwebbbenwebb I'm running v2.5.0.alpha.2, so far no issues. I will keep you posted.

@seidnerj
Copy link
Contributor Author

I haven't noticed any issue so far though I don't use the AC much during the winter time. Have you been using it? Noticed anything "off"?

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.

2 participants