feat: add lux sensor brightness mode#1413
Conversation
bcb3ca7 to
d188cc0
Compare
60770ee to
ad847c0
Compare
|
Thanks @an0nfunc! Will check later. Just curious, did you use Opus 4.5 or another model? |
Cool :) This is done by Opus 4.5 and manually reviewed 😇 Edit: I see there are still some tests failing, will have a look 👀. |
e82bfa7 to
de2193e
Compare
|
Hi @basnijholt, did you have a chance to look at this yet? |
|
Hi @basnijholt, can we get this added please? |
|
I have tested this custom build and while it reliably adjusts the brightness based on the value reported by the specified illumination sensor, I have some trouble with controlling the color temperature. Also, I have a general question: Does it really make sense to adapt the lightning entirely based on an illumination sensor? Even if the sensor is outdoor, it will drop to a very low lux value shortly after the sun has set, resulting in minimal brightness on the controlled lights. This could of course be mitigated by setting a higher I think a better approach would be to stick with a sun-position based method and only use an illumination sensor as an dynamic offset, so that e.g. weather conditions are taken into account. WDYT @an0nfunc & @basnijholt ? |
florianhorner
left a comment
There was a problem hiding this comment.
Review Summary
Great feature — lux-based brightness is the #1 most-requested feature (issue #1177, 19 👍). The core algorithm is sound but there are a few issues to address before this is merge-ready.
Critical Issues
1. No debounce on lux sensor changes
Consumer lux sensors (e.g. Aqara, Hue) report every 1–10 seconds. With the default 90s adaptation interval, subscribing to every lux state change causes a 6–90× increase in adaptation frequency. This will flood slower lights (Zigbee, Z-Wave) with commands.
Suggestion: Add a minimum change threshold (e.g. 5 lux or 2% of range) or a debounce timer that batches rapid changes.
2. time.time() should be time.monotonic()
The smoothing buffer uses wall-clock time for expiry. Wall clock is NTP-sensitive — a time sync can cause the entire buffer to expire or retain stale samples. time.monotonic() is the correct choice for elapsed-time measurement.
3. pytest-asyncio==1.3.0 in [project] dependencies
This is a test dependency in the runtime deps section. It belongs in [dependency-groups]. Also, the pinned version (1.3.0) may not exist — the versioning scheme for pytest-asyncio uses 0.x.
Quality Issues
4. Stringly-typed None check
lux_sensor != "None" — the codebase already has replace_none_str() in hass_utils.py for exactly this pattern. Use it for consistency.
5. Parameter sprawl
lux_value: float | None = None is added to 3 method signatures (brightness_pct, brightness_and_color, get_settings). Only get_settings needs it — let it pass lux_value down internally.
6. Misleading "race condition" comment
The comment on list(self._lux_samples) says it prevents a race condition, but this is asyncio (single-threaded cooperative multitasking). There is no race. Remove the misleading comment.
7. Redundant clamp()
In _brightness_pct_lux, after the boundary checks that already return min/max values for out-of-range lux, the final clamp() can never trigger. It's dead code.
8. Fragile split conditional for lux fallback
The brightness_mode == "lux" path is split across two separate if branches. Consolidate into one explicit block:
if self.brightness_mode == "lux":
return self._brightness_pct_lux(lux_value) if lux_value is not None else self._brightness_pct_default(dt)Efficiency Issues
9. Deque copy on every call
_get_smoothed_lux copies the entire deque with list(self._lux_samples) on every invocation. Instead, trim expired samples in _add_lux_sample (on write) so the read path is zero-copy.
10. No minimum lux change threshold
Sub-lux changes (e.g. 150.1 → 150.3) trigger full adaptation cycles with negligible brightness difference. A small dead-band (e.g. 5 lux) would eliminate unnecessary work.
Overall this is a well-tested feature that many users want. The debounce and time.monotonic issues are the most important to fix. Happy to re-review once addressed! 👍
Depends on the usecase I guess. For me its mostly for lights that are in rooms which are not illuminated or only indirectly illuminated by outside light sources. And of course for those rooms which are, I'm fine with a low brightness start and not getting much brighter, but like you said this can be regulated by min brightness. What you mean is for the lux sensor to nudge the sun-based brightness up/down, right? Could also be an interesting mode, but may be out of scope for this PR. Just out of curiosity for that use case, cant you turn on the lights earlier based on your preferred light level? |
|
Tbh this is quite a low quality review. Did you actually take the time to verify what your model of choice put out there? Some of these points are completely hallucinated. Also please use the github review function, a comment review is very hard to respond to point by point.
Thats why the smoothing and transition timers exist. Could still trigger some unnecessary calls, but certainly not critical.
We have a 300s smoothing interval, ntp is not really relevant here, not to speak of home-assistants own time.time usage.
Hallucinated?
Doesnt really replace the functionally in that line, would still need the empty check. Dont really have a pref but I find the current form easier to read.
Pretty standard delegation, not worth abstracting imho.
Valid point, should reword that.
Valid.
Hard to read and a style pref.
Its a 5 element list, what are we optimizing here?
Thats the same issue as 1.
|
1125119 to
702592e
Compare
Add support for controlling brightness based on an outdoor lux sensor instead of sun position. Users can select "lux" as the brightness_mode and configure a lux sensor entity. New configuration options: - lux_sensor: Entity ID of outdoor illuminance sensor - lux_min: Lux value for minimum brightness (default: 0) - lux_max: Lux value for maximum brightness (default: 10000) - lux_smoothing_samples: Number of samples to average (default: 5) - lux_smoothing_window: Time window in seconds (default: 300) Features: - Linear brightness mapping matching circadian behavior (dark = dim) - Smoothing buffer to prevent rapid fluctuations - Automatic fallback to sun-based calculation when sensor unavailable - Switch attributes show current_lux and lux_samples_count
702592e to
348108f
Compare
Yes my idea basically is reducing brightness based on weather conditions, e.g. when its cloudy and thus darker outside, account for this when the brightness is calculated. This could be achieved using a weather sensor or an illumination sensor which would be a bit more accurate. But generally my suggestion is to stick to the suns position and - regardless of the mode - only slightly alter it instead of basing the calculation entirely on the illuminance. The experience I made with illuminance sensors is that they have a quite poor resolution during dask/dawn and are essentially becoming binary sensors then + there is no indication whether it is 5 minutes after dusk or 5 hours after dusk since both is equally dark. But I completely understand your use-case and did not mean to challenge it based on my personal preferences. I guess both are valid use-cases and I will probably prepare a PR on my own and then the maintainers can decide what is better suitable for the project 😃 . |
Summary
Adds support for controlling brightness based on an outdoor lux sensor instead of sun position. This allows adaptive lighting to respond to actual ambient light conditions (e.g., overcast days) rather than calculated sun position.
luxoption forbrightness_modeConfiguration
lux_sensorlux_minlux_maxlux_smoothing_sampleslux_smoothing_windowTest plan
Closes #1177