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

Detect non ha changes | wait for transitions | performance improvements #486

Conversation

th3w1zard1
Copy link
Collaborator

@th3w1zard1 th3w1zard1 commented Mar 27, 2023

This is a rewrite designed to accomplish the following:

  1. The interval function should not do anything besides update attributes if a transition is in progress.
  2. Slightly rework detect_non_ha_changes to suit goal 1
  3. _adapt_lights has been renamed to _update_manual_control_and_maybe_adapt and all manual control checks have been moved there.

This pr slightly reworked TurnOnOffListener's last_state_change dictionary as it now stores all state changes done to our lights, The old behavior where it is cleared on every new adapt is still there.

Based on #450

Things this PR fixed that are broken in the main Adaptive Lighting releases ranging from versions 1.3.0 to 1.7.0:

Things this PR would improve:

  • Possibly significant performance increases, especially to those using zigbee/z-wave, as we aren't sending light commands unnecessarily.

@th3w1zard1
Copy link
Collaborator Author

Slow and steady wins the race. Having a local testing environment was substantial for development and cut the testing time down by 90%. Tested on my own system and works great :). @basnijholt this PR is ready for review.

@th3w1zard1
Copy link
Collaborator Author

@nathang21 this is ready!

@th3w1zard1
Copy link
Collaborator Author

th3w1zard1 commented Mar 28, 2023

I'm satisfied with this PR, everything is very efficient now. I've been forced to use an interval of 90 or else my zigbee network would go completely out of whack as the update_entity spam was just too much.

Now Adaptive Lighting will correctly wait for all transitions before doing anything, and alt_detect_method finds non ha changes without calling update_entity. The interval will now only update attributes, and check if things need to be done. I hope we can get this in a release for everyone in the near future.

cnt_significant_changes can be added back if needed, but I'd be surprised if it is still required.

@th3w1zard1 th3w1zard1 enabled auto-merge (squash) March 28, 2023 01:47
@th3w1zard1
Copy link
Collaborator Author

This could be improved by instead starting the transition timer when TurnOnOffListener gets the first state_change event from our last_service_data. This way we'd ensure that the light successfully got our light.turn_on command and has started adapting accordingly. I haven't noticed this problem on my Home Assistant setup so I have no way to test it.

Also sleep the correct time.
@th3w1zard1 th3w1zard1 force-pushed the rewrite_detect_non_ha_changes_slowattempt branch from 592c387 to 4ebfbee Compare April 1, 2023 22:18
@th3w1zard1 th3w1zard1 mentioned this pull request Apr 3, 2023
@th3w1zard1
Copy link
Collaborator Author

th3w1zard1 commented Apr 3, 2023

@basnijholt Hey, hope you're having a good weekend 😄

Do you have a development plan for the next release?
What order are these PRs merging in?
Do you have any issues with this PR in its current form? If not what would you like changed?

I'd like to get the fix 'include_config_in_attributes for change_switch_settings' pushed into a hotfix 1.7.1 so that users posting issues could easily use change_switch_settings to post their config.

@basnijholt
Copy link
Owner

Hi @th3w1zard1!

Do you have a development plan for the next release?
What order are these PRs merging in?
Do you have any issues with this PR in its current form? If not what would you like changed?

Every single time I look at this PR I get a bit overwhelmed because it changes 20-30% of the lines in switch.py. Since it contains 3 changes. Would it be possible to separate them out in to different PRs? Or am I now asking something horrible? 😅

Merge `upstream/main`
Timer should start when we first detect the state change in `TurnOnOffListener`, not when we call `light.turn_on`

TODO: warn user when light doesn't change states from a previous `light.turn_on` call.
@th3w1zard1
Copy link
Collaborator Author

Hi @th3w1zard1!

Do you have a development plan for the next release?
What order are these PRs merging in?
Do you have any issues with this PR in its current form? If not what would you like changed?

Every single time I look at this PR I get a bit overwhelmed because it changes 20-30% of the lines in switch.py. Since it contains 3 changes. Would it be possible to separate them out in to different PRs? Or am I now asking something horrible? 😅

I absolutely will do that for you, no problem at all! Thanks for the honesty, I should have done this from the start!

@th3w1zard1
Copy link
Collaborator Author

th3w1zard1 commented Apr 3, 2023

@basnijholt I created a new branch on your repo as some of these changes only can merge into each other

@th3w1zard1 th3w1zard1 marked this pull request as draft April 3, 2023 06:24
auto-merge was automatically disabled April 3, 2023 06:24

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment