Adaptive Lighting tweaks #868
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #868 +/- ##
==========================================
- Coverage 66.17% 63.73% -2.45%
==========================================
Files 40 40
Lines 2903 2509 -394
Branches 792 650 -142
==========================================
- Hits 1921 1599 -322
+ Misses 883 739 -144
- Partials 99 171 +72
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
itavero
force-pushed
the
revisit-adaptive-lighting
branch
from
6d6c526 to
1da7611
Compare
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request introduces significant enhancements to the Adaptive Lighting feature across multiple files. The changes primarily focus on transforming the adaptive lighting configuration from a simple boolean toggle to a more flexible object-based approach. The new configuration allows users to enable/disable adaptive lighting, control updates only when the light is on, and set transition times. By default, adaptive lighting is now enabled for compatible lights, with additional logic added to reset the internal color temperature reference when lights are adjusted via HomeKit. Changes
Sequence DiagramsequenceDiagram
participant User
participant HomeKit
participant Light
participant AdaptiveLighting
User->>Light: Turn On/Adjust Brightness
Light->>AdaptiveLighting: Reset Color Temperature
Light->>HomeKit: Update Light State
HomeKit->>User: Confirm Changes
Possibly related PRs
Suggested labels
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/converters/light.ts (1)
81-90: Consider simplifying the configuration logicThe configuration initialization could be simplified using object spread operator with conditional properties.
Consider this alternative implementation:
-let adaptiveLightingConfig: AdaptiveLightingConfig = { ...LightCreator.DEFAULT_CONFIG }; -if (isLightConfig(converterConfig)) { - if (isAdaptiveLightingConfig(converterConfig.adaptive_lighting)) { - adaptiveLightingConfig = converterConfig.adaptive_lighting; - if (adaptiveLightingConfig.enabled === undefined) { - adaptiveLightingConfig.enabled = true; - } - } else if (converterConfig.adaptive_lighting === false) { - adaptiveLightingConfig.enabled = false; - } -} +const adaptiveLightingConfig: AdaptiveLightingConfig = { + ...LightCreator.DEFAULT_CONFIG, + ...(isLightConfig(converterConfig) && { + ...(isAdaptiveLightingConfig(converterConfig.adaptive_lighting) + ? { ...converterConfig.adaptive_lighting, enabled: converterConfig.adaptive_lighting.enabled ?? true } + : { enabled: converterConfig.adaptive_lighting !== false }) + }) +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)config.schema.json(1 hunks)docs/light.md(2 hunks)src/converters/light.ts(9 hunks)test/light.spec.ts(5 hunks)test/testHelpers.ts(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/testHelpers.ts
🧰 Additional context used
🪛 LanguageTool
docs/light.md
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...uration (light) - adaptive_lighting: Set to false to disable [Adaptive Lig...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~16-~16: A comma may be missing after the conjunctive/linking adverb ‘Additionally’.
Context: ...a Color Temperature characteristic. Additionally you can also configure the following op...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🪛 Markdownlint (0.37.0)
CHANGELOG.md
12-12: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
13-13: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (13)
docs/light.md (3)
15-15: LGTM: Clear documentation for disabling adaptive lighting
The documentation clearly explains how to disable the Adaptive Lighting feature.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...uration (light) - adaptive_lighting: Set to false to disable [Adaptive Lig...
(UNLIKELY_OPENING_PUNCTUATION)
17-19: LGTM: Well-documented configuration options
The new configuration options are clearly documented with their default values and purposes:
enabled: Controls adaptive lighting (defaults to true)only_when_on: Controls updates timing (defaults to true)transition: Controls transition time (optional)
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: Loose punctuation mark.
Context: ...on. Defaults to true. - transition: Transition time to send along with the ...
(UNLIKELY_OPENING_PUNCTUATION)
[grammar] ~19-~19: Consider using either the past participle “sent” or the present participle “sending” here.
Context: ...f not defined, transition will not be send. ```json { "converters": { "ligh...
(BEEN_PART_AGREEMENT)
26-26: LGTM: Clear configuration example
The JSON example clearly demonstrates how to configure the new options.
config.schema.json (1)
164-186: LGTM: Well-structured schema for adaptive lighting configuration
The schema properly defines the new configuration structure with appropriate titles, types, and defaults:
enabled: boolean, defaults to trueonly_when_on: boolean, defaults to truetransition: integer, defaults to 0
src/converters/light.ts (6)
33-33: LGTM: Clean interface extension
The AdaptiveLightingConfig interface is properly extended with the new optional enabled property.
44-45: LGTM: Updated type guard
The type guard for AdaptiveLightingConfig correctly handles the new enabled property while maintaining type safety.
57-60: LGTM: Well-defined defaults
The default configuration properly sets the initial values:
enabled: true: Enables adaptive lighting by defaultonly_when_on: true: Prevents unnecessary updates when light is offtransition: undefined: Allows flexible transition timing
386-393: LGTM: Proper state handling with adaptive lighting reset
The handleSetOn method correctly:
- Handles the state change
- Resets adaptive lighting temperature when turning on
416-417: LGTM: Proper brightness handling with adaptive lighting reset
The brightness handler correctly resets the adaptive lighting temperature when brightness changes.
Line range hint 544-560: LGTM: Robust adaptive lighting implementation
The adaptive lighting handler properly implements:
- Skip updates when light is off (if configured)
- Transition timing for smooth changes
- Proper temperature change threshold
- Detailed debug logging
CHANGELOG.md (1)
9-13: LGTM! Clear and well-structured changelog entry.
The changelog entry properly documents the key changes related to Adaptive Lighting, following the Keep a Changelog format.
🧰 Tools
🪛 Markdownlint (0.37.0)
12-12: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
13-13: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
test/light.spec.ts (2)
169-170: Consistent update to adaptive lighting configuration.
The configuration has been updated to use the new object-based structure for adaptive lighting settings, which aligns with the changes in the main codebase. The numberOfExpectedControllers is appropriately set based on the device capabilities.
Also applies to: 496-497
Line range hint 866-959: Well-structured test suite for Innr RB-249-T.
The new test suite thoroughly covers:
- Basic light functionality (on/off, brightness)
- Color temperature bounds handling
- Proper controller initialization
Good job on including edge cases for color temperature bounds testing.
| adaptiveLightingConfig.enabled = true; | ||
| } | ||
| } else if (converterConfig.adaptive_lighting === false) { | ||
| adaptiveLightingConfig.enabled = false; |
There was a problem hiding this comment.
This looks like a promising solution for #987, right?
There was a problem hiding this comment.
I'll rebase this branch on the current master. Would you also be able to give it a spin on your environment to see if it resolves #987?
There was a problem hiding this comment.
Just pushed the rebased branch.
There was a problem hiding this comment.
Would you also be able to give it a spin on your environment to see if it resolves #987?
Sure!
I‘m going to pull the branch and report back. 🤞
… is changed or turned on via HomeKit. Still need to fix tests
itavero
force-pushed
the
revisit-adaptive-lighting
branch
from
1da7611 to
eef7d2a
Compare
…a config option for adaptive lighting
|
@itavero Looking good so far! We should probably check, before restoring accessories from cache, if Adaptive Lighting is actually enabled, so we can promptly remove the relevant characteristics in case it's turned off. |
Good catch. Unlike services, I don't think I put in any logic to do this for the adaptive lighting controller. |
itavero
force-pushed
the
revisit-adaptive-lighting
branch
from
aa04439 to
8301a88
Compare
|
Not yet sure how to check if the controller was present. I see an Update: after going through the HAP-NodeJS sources, it seems that it's not very straightforward to remove it. For now I'll add a note in the documentation that the users may have to remove the accessory from the Homebridge accessory cache for it to get disabled. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
10-16: Consider adding more details about Adaptive Lighting changes.The changelog entry for Adaptive Lighting could benefit from additional details:
- What is the default behavior for lights that don't support it?
- What is the minimum delta for color temperature changes?
- How can users configure these settings?
Consider expanding the entry like this:
- Adaptive Lighting: - Enabled by default for lights that support it (can still be disabled). - Internal color temperature reference is reset when brightness or state is changed via HomeKit. - - Allow a minimum delta to be set for color temperature changes. + - Allow a minimum delta to be set for color temperature changes (configurable via `min_delta` option). + - Gracefully falls back to manual control for unsupported lights. + - See documentation at <link> for configuration options.🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
14-14: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
15-15: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
13-15: Fix markdown list indentation.The list items under "Adaptive Lighting" are indented with 3 spaces, but markdown style guides recommend 2 spaces for better consistency.
Apply this formatting change:
- Adaptive Lighting: - - Enabled by default for lights that support it (can still be disabled). - - Internal color temperature reference is reset when brightness or state is changed via HomeKit. - - Allow a minimum delta to be set for color temperature changes. + - Enabled by default for lights that support it (can still be disabled). + - Internal color temperature reference is reset when brightness or state is changed via HomeKit. + - Allow a minimum delta to be set for color temperature changes.🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
14-14: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
15-15: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.nvmrc(1 hunks).vscode/settings.json(1 hunks)CHANGELOG.md(1 hunks)config.schema.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .vscode/settings.json
- .nvmrc
🚧 Files skipped from review as they are similar to previous changes (1)
- config.schema.json
🧰 Additional context used
🪛 Markdownlint (0.37.0)
CHANGELOG.md
13-13: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
14-14: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
15-15: Expected: 2; Actual: 3
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (1)
CHANGELOG.md (1)
Line range hint
1-24: LGTM! The changelog follows the Keep a Changelog format.The changelog is well-structured and follows the recommended format, making it easy for users to track changes across versions.
🧰 Tools
🪛 Markdownlint (0.37.0)
13-13: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
14-14: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
15-15: Expected: 2; Actual: 3
Unordered list indentation(MD007, ul-indent)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docs/light.md (4)
16-16: Improve clarity of the adaptive lighting description.The description should be more explicit about the default state. Consider restructuring as:
-`adaptive_lighting`: Set to `false` to disable [Adaptive Lighting](https://support.apple.com/guide/iphone/control-accessories-iph0a717a8fd/ios#iph79e72e212). Apple requires a home hub for Adaptive Lighting to work. This feature is only available for lights that expose a *Color Temperature* characteristic. +`adaptive_lighting`: Controls [Adaptive Lighting](https://support.apple.com/guide/iphone/control-accessories-iph0a717a8fd/ios#iph79e72e212) support. This feature is enabled by default for compatible lights that expose a *Color Temperature* characteristic. Set to `false` to disable. Note: Apple requires a home hub (HomePod, Apple TV, or iPad) for Adaptive Lighting to work.🧰 Tools
🪛 LanguageTool
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...uration (light) -adaptive_lighting: Set tofalseto disable [Adaptive Lig...(UNLIKELY_OPENING_PUNCTUATION)
18-20: Clarify the relationship between boolean and object configurations.The documentation should be clearer about backward compatibility. Consider:
- - `enabled`: Set to `true` to enable Adaptive Lighting. Defaults to `true`. (same as setting `adaptive_lighting` to a boolean value) + - `enabled`: Controls whether Adaptive Lighting is enabled (defaults to `true`). For backward compatibility, you can also set `adaptive_lighting` directly to a boolean value.🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...previously had Adaptive Lighting enabled and you try to disable it, you may need to ...(COMMA_COMPOUND_SENTENCE)
22-22: Fix grammar in transition description.- - `transition`: Transition time to send along with the color temperature change when the light is on. If not defined, `transition` will not be send. + - `transition`: Transition time to send along with the color temperature change when the light is on. If not defined, `transition` will not be sent.🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: Loose punctuation mark.
Context: ...on. Defaults totrue. -transition: Transition time to send along with the ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~22-~22: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...f not defined,transitionwill not be send. -min_delta: Minimum difference in...(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
31-34: Enhance the JSON example with comments and alternative syntax.Consider adding comments and showing both configuration styles:
{ "converters": { "light": { + // Object syntax (recommended) "adaptive_lighting": { "enabled": true, "only_when_on": true, "transition": 1, "min_delta": 10 - }, + } + // OR boolean syntax (legacy) + // "adaptive_lighting": true, } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)docs/light.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 LanguageTool
docs/light.md
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ...uration (light) - adaptive_lighting: Set to false to disable [Adaptive Lig...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~17-~17: A comma may be missing after the conjunctive/linking adverb ‘Additionally’.
Context: ...a Color Temperature characteristic. Additionally you can also configure the following op...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[uncategorized] ~19-~19: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...previously had Adaptive Lighting enabled and you try to disable it, you may need to ...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~21-~21: Loose punctuation mark.
Context: ...ccessories* section). - only_when_on: Only update the color temperature when ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~22-~22: Loose punctuation mark.
Context: ...on. Defaults to true. - transition: Transition time to send along with the ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~22-~22: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...f not defined, transition will not be send. - min_delta: Minimum difference in...
(AI_EN_LECTOR_REPLACEMENT_VERB_FORM)
[uncategorized] ~23-~23: Loose punctuation mark.
Context: ...itionwill not be send. -min_delta`: Minimum difference in color temperature...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~24-~24: Loose punctuation mark.
Context: ...um allowed value. - request_brightness: Set to true to allow the brightness t...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (1)
docs/light.md (1)
19-20: Documentation properly addresses the cache removal issue.The note about removing accessories from the cache when disabling Adaptive Lighting is well-documented and aligns with the discussion in the PR comments. This is a good temporary solution until a proper fix can be implemented in the code.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...previously had Adaptive Lighting enabled and you try to disable it, you may need to ...(COMMA_COMPOUND_SENTENCE)
|
|
🆗 Published SonarCloud and code coverage data. |
I guess removing should be possible with the method you’ve mentioned, but how do we know if a controller service is actually attached? Maybe with @itavero Enabling Adaptive Lighting by default, for all lights, might not be such a good idea after all, if we have to remove entire accessories to get rid of it. Edit: Couldn’t we basically do the same as here, but with the homebridge-z2m/src/converters/light.ts Line 361 in a677151 |
Tweaks in preparation of new release:
Still to do:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores