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

Change subscription floor to 1 second to prevent flooding #891

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

marcelveldt
Copy link
Contributor

Change subscription floor to 1 second (instead of 0) to prevent flooding during transitions.

This basically means that sending an attribute update is debounced with 1 second.
A state update (e.g. motion detected, door sensor) still comes as fast as possible but if you e.g. toggle it super fast you will only get an update every second which is good enough for all normal usecases.

Tested it with doorsensor and updates are still instant (just debounced with 1 seconds if you open/close very fast in succession). Also tested a bit with a generic switch (remote button press) and the 1 second debounce still felt natural (you just get a burst of events at once) but if this may ever lead to complaints/issues, we could consider splitting up the subscription in 2 parts, one for regular attribute updates (where we could even go for 2 seconds maybe) and one for events (where we want a 0 seconds floor).

Without this fix (so with a 0 second floor), a device sends a burst of attribute changes every 100ms (or even higher frequency!) resulting in increased traffic and even a bit of a laggy experience such as a clunky transition on the light itself (maybe because it needs to send so many updates). same applies for e.g. switch with "onWithTimedOff" where the device sends the state of the timer at an insane interval.

For lights, this makes the transitions working smoothly, at least for Thread based lights.
Does not fix transition issues on ESP32 because that is a firmware issue.

Copy link

@Apollon77 Apollon77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :-))

@agners
Copy link
Collaborator

agners commented Sep 19, 2024

I was actually planning on making this configurable. It might be useful for the ceiling as well, depending on needs, and for testing in general.

@MartinHjelmare MartinHjelmare merged commit 8328190 into main Sep 19, 2024
3 checks passed
@MartinHjelmare MartinHjelmare deleted the subscription-floor branch September 19, 2024 08:16
@marcelveldt
Copy link
Contributor Author

I was actually planning on making this configurable. It might be useful for the ceiling as well, depending on needs, and for testing in general.

Let's do that later as we need to hide those settings from users and make them dev-only.

@Apollon77
Copy link

I agree, the requirements also from matter and such should not offer "unexperienced users" to tweak them because this can have device specific strange effects

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.

4 participants