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.

Sign up for GitHub

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

Do not request brightness of light by default #982

Merged
merged 3 commits into from
Jan 4, 2025
Merged

Do not request brightness of light by default #982

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b6fe321
Do not request brightness of light by default
itavero Jan 4, 2025
3e2b9f2
Fix tests and some typos
itavero Jan 4, 2025
524bd1a
Apply some suggestions from code rabbit
itavero Jan 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,9 @@
},
"[typescript]": {
"editor.defaultFormatter": "esbenp.prettier-vscode"
}
},
"cSpell.words": [
"Elgato",
"Zigbee"
]
}
7 changes: 5 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ Since version 1.0.0, we try to follow the [Semantic Versioning](https://semver.o
### Changed

- Light sensor will now use `illuminance` property if `illuminance_lux` is not available. This should fix compatibility with the new major v2 release of Zigbee2MQTT. (see [#966](https://github.com/itavero/homebridge-z2m/issues/966))
- Brightness for a Light is no longer requested by default. This should prevent issues when the light is off.
Old behavior can be restored using the `request_brightness` option in the converter specific configuration.
(see [#882](https://github.com/itavero/homebridge-z2m/issues/882))

## [1.11.0-beta.6] - 2024-06-30

Expand Down Expand Up @@ -111,7 +114,7 @@ Based on v1.9.2, as v1.9.3 was made later as a hotfix.

### Fixed

- Added additional checks to prevent certain errors from occuring during creation of a service handler. (see [#443](https://github.com/itavero/homebridge-z2m/issues/443))
- Added additional checks to prevent certain errors from occurring during creation of a service handler. (see [#443](https://github.com/itavero/homebridge-z2m/issues/443))
- Removed some default values from `config.schema.json` to prevent certain illegal configurations from being created by accident.

## [1.9.0] - 2022-06-29
Expand Down Expand Up @@ -458,7 +461,7 @@ For `cover` devices the following changes/fixes are in this release:

### Changed

- Restore BatteryServuce and WindowConvering properly on start up.
- Restore BatteryService and WindowCovering properly on start up.
- Improve state determination for WindowCovering.

[unreleased]: https://github.com/itavero/homebridge-z2m/compare/v1.11.0-beta.6...HEAD
Expand Down
2 changes: 2 additions & 0 deletions docs/light.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# Light

If the device definition from Zigbee2MQTT contains one or more `exposes` entries of type `light` that at least have a feature named `state` (i.e. on/off), a [Lightbulb](https://developers.homebridge.io/#/service/Lightbulb) service will be created.
The table below shows how the different features within this `exposes` entry are mapped to characteristics.

Expand All @@ -16,6 +17,7 @@ The table below shows how the different features within this `exposes` entry are
Additionally you can also configure the following options for Adaptive Lighting:
- `only_when_on`: Only update the color temperature when the light is on. Defaults to `true`.
- `transition`: Transition time to send along with the color temperature change when the light is on. If not defined, `transition` will not be send.
- `request_brightness`: Set to `true` to allow the brightness to be requested (if possible). Defaults to `false`, as this can cause issues when the light is off.

```json
{
Expand Down
10 changes: 8 additions & 2 deletions src/converters/light.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ interface AdaptiveLightingConfig {

interface LightConfig {
adaptive_lighting?: boolean | AdaptiveLightingConfig;
request_brightness?: boolean;
}

const isAdaptiveLightingConfig = (x: unknown): x is AdaptiveLightingConfig =>
Expand Down Expand Up @@ -74,8 +75,12 @@ export class LightCreator implements ServiceCreator {

private createService(expose: ExposesEntryWithFeatures, accessory: BasicAccessory): void {
const converterConfig = accessory.getConverterConfiguration(LightCreator.CONFIG_TAG);
let requestBrightness = false;
let adaptiveLightingConfig: AdaptiveLightingConfig | undefined = undefined;
if (isLightConfig(converterConfig)) {
if (converterConfig.request_brightness === true) {
requestBrightness = true;
}
if (isAdaptiveLightingConfig(converterConfig.adaptive_lighting)) {
adaptiveLightingConfig = converterConfig.adaptive_lighting;
} else if (converterConfig.adaptive_lighting === true) {
Expand All @@ -84,7 +89,7 @@ export class LightCreator implements ServiceCreator {
}

try {
const handler = new LightHandler(expose, accessory, adaptiveLightingConfig);
const handler = new LightHandler(expose, accessory, requestBrightness, adaptiveLightingConfig);
accessory.registerServiceHandler(handler);
} catch (error) {
accessory.log.warn(`Failed to setup light for accessory ${accessory.displayName} from expose "${JSON.stringify(expose)}": ${error}`);
Expand Down Expand Up @@ -138,6 +143,7 @@ class LightHandler implements ServiceHandler {
constructor(
expose: ExposesEntryWithFeatures,
private readonly accessory: BasicAccessory,
private readonly requestBrightness: boolean,
private readonly adaptiveLightingConfig?: AdaptiveLightingConfig
) {
const endpoint = expose.endpoint;
Expand Down Expand Up @@ -182,7 +188,7 @@ class LightHandler implements ServiceHandler {
if (exposesCanBeGet(this.stateExpose)) {
keys.push(this.stateExpose.property);
}
if (this.brightnessExpose !== undefined && exposesCanBeGet(this.brightnessExpose)) {
if (this.brightnessExpose !== undefined && exposesCanBeGet(this.brightnessExpose) && this.requestBrightness) {
keys.push(this.brightnessExpose.property);
}
Comment on lines +189 to 191
Copy link

@coderabbitai coderabbitai bot Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider conditional brightness characteristic creation

Currently, the brightness characteristic is always created even when requestBrightness is false, but its value won't be updated. This might lead to stale or incorrect values being displayed.

Consider moving the requestBrightness check to the tryCreateBrightness method to prevent characteristic creation when not needed:

  private tryCreateBrightness(features: ExposesEntryWithProperty[], service: Service) {
    this.brightnessExpose = features.find(
      (e) => e.name === 'brightness' && exposesHasNumericRangeProperty(e) && exposesCanBeSet(e) && exposesIsPublished(e)
    ) as ExposesEntryWithNumericRangeProperty;
-   if (this.brightnessExpose !== undefined) {
+   if (this.brightnessExpose !== undefined && this.requestBrightness) {
      this.mainCharacteristics.push(
        getOrAddCharacteristic(service, hap.Characteristic.Brightness).on('set', this.handleSetBrightness.bind(this))
      );

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Owner Author

@itavero itavero Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Even if we cannot "get" brightness, it will still be published (we check that when finding the exposes information if I recall correctly). So the characteristic should be created regardless of this new setting.

Copy link

Choose a reason for hiding this comment

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

@itavero, understood. Thank you for clarifying! The brightness characteristic might still hold value even when it isn’t actively polled, given that it will be published based on the device’s exposes. Let me know if there’s anything else I can help with.

✏️ Learnings added 
Learnt from: itavero
PR: itavero/homebridge-z2m#982
File: src/converters/light.ts:189-191
Timestamp: 2025-01-04T19:37:24.409Z
Learning: Even if brightness isn't gettable (when requestBrightness is false), the brightness characteristic remains relevant since it is published by the device. This is reflected in src/converters/light.ts, where we retain the brightness characteristic creation logic.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

if (this.colorTempExpose !== undefined && exposesCanBeGet(this.colorTempExpose)) {
Expand Down
16 changes: 10 additions & 6 deletions test/light.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ describe('Light', () => {
newHarness.checkCreationExpectations();
newHarness.checkHasMainCharacteristics();

newHarness.checkExpectedGetableKeys(['state', 'brightness', 'color_temp', 'color']);
newHarness.checkExpectedGetableKeys(['state', 'color_temp', 'color']);

// Expect range of color temperature to be configured
lightbulb.checkCharacteristicPropertiesHaveBeenSet('color_temp', {
Expand Down Expand Up @@ -507,7 +507,7 @@ describe('Light', () => {
newHarness.checkCreationExpectations();
newHarness.checkHasMainCharacteristics();

newHarness.checkExpectedGetableKeys(['state', 'brightness', 'color_temp', 'color']);
newHarness.checkExpectedGetableKeys(['state', 'color_temp', 'color']);

// Expect range of color temperature to be configured
lightbulb.checkCharacteristicPropertiesHaveBeenSet('color_temp', {
Expand Down Expand Up @@ -557,7 +557,7 @@ describe('Light', () => {
});
});

describe('Hue White Single bulb B22', () => {
describe('Hue White Single bulb B22 (allow request brightness)', () => {
// Shared "state"
let deviceExposes: ExposesEntry[] = [];
let harness: ServiceHandlersTestHarness;
Expand All @@ -571,6 +571,10 @@ describe('Light', () => {

// Check service creation
const newHarness = new ServiceHandlersTestHarness();

// Enable adaptive lighting to check if it will be ignored (as this device does not have a color temperature)
newHarness.addConverterConfiguration('light', { request_brightness: true });

newHarness
.getOrAddHandler(hap.Service.Lightbulb)
.addExpectedCharacteristic('state', hap.Characteristic.On, true)
Expand Down Expand Up @@ -687,7 +691,7 @@ describe('Light', () => {
newHarness.checkCreationExpectations();
newHarness.checkHasMainCharacteristics();

newHarness.checkExpectedGetableKeys(['state', 'brightness', 'color_temp', 'color']);
newHarness.checkExpectedGetableKeys(['state', 'color_temp', 'color']);

// Expect range of color temperature to be configured
lightbulb.checkCharacteristicPropertiesHaveBeenSet('color_temp', {
Expand Down Expand Up @@ -829,7 +833,7 @@ describe('Light', () => {
newHarness.checkCreationExpectations();
newHarness.checkHasMainCharacteristics();

newHarness.checkExpectedGetableKeys(['state', 'brightness']);
newHarness.checkExpectedGetableKeys(['state']);
harness = newHarness;
}

Expand Down Expand Up @@ -885,7 +889,7 @@ describe('Light', () => {
newHarness.checkCreationExpectations();
newHarness.checkHasMainCharacteristics();

newHarness.checkExpectedGetableKeys(['state', 'brightness', 'color_temp']);
newHarness.checkExpectedGetableKeys(['state', 'color_temp']);

// Expect range of color temperature to be configured
lightbulb.checkCharacteristicPropertiesHaveBeenSet('color_temp', {
Expand Down
Loading