Initial air purifier support #1012
Conversation
WalkthroughThis change introduces air purifier support to the home automation system. A new file defines classes and interfaces for managing air purifier service creation and state management. The functionality includes filtering exposes data to create service handlers and updating states with defined properties. Additionally, the new air purifier creator is integrated into the service creator manager for proper instantiation and error handling within the system. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Manager as BasicServiceCreatorManager
participant Creator as AirPurifierCreator
participant Handler as AirPurifierHandler
participant Logger as Logger
Client->>Manager: Provide exposes entries
Manager->>Creator: Iterate over exposes entries
Creator->>Handler: Create/init air purifier service
note right of Handler: Initialize properties\n(Current, Target, Rotation Speed, Lock Controls)
Creator-->>Logger: Log warnings/errors if setup fails
sequenceDiagram
participant Data as Data Provider
participant Handler as AirPurifierHandler
participant Property as AirPurifierProperty
Data->>Handler: Send sensor update
Handler->>Property: Update state for each property
Property-->>Handler: Return updated state
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 (
|
Please check the You can use I was thinking about creating a dev container for this repository, but never got around to doing so. |
I'll try that.
That would be incredibly helpful. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/converters/air_purifier.ts(1 hunks)src/converters/creators.ts(2 hunks)
🔇 Additional comments (4)
src/converters/creators.ts (1)
13-13: Ensure test coverage for the newAirPurifierCreator.The import statement and array inclusion look correct. However, please verify that the newly introduced
AirPurifierCreatoris fully tested to confirm that it works as expected in conjunction with other service creators.Also applies to: 35-35
src/converters/air_purifier.ts (3)
1-13: Overall implementation looks solid.• The import statements and basic class/type definitions (lines 1–13, 14–31, 33–42, 45–48, 49–53, and 55–83) align well with Homebridge patterns and existing code conventions.
• Construction and registration of theAirPurifierCreatorwithin the accessory appear consistent with the system's plugin architecture.Also applies to: 14-31, 33-42, 45-48, 49-53, 55-83
158-159: Verify the speed scaling factor.Multiplying by 11.11 and dividing by 11.11 might cause off-by-one or rounding issues. Confirm that a 0–9 fan speed range is intended, and consider using a variable or fraction (e.g.,
100 / 9) for clearer maintenance.Also applies to: 163-165
201-263: Suggest additional tests forAirPurifierHandler.The handler logic looks well-structured, but it's untested. Consider unit tests to confirm each property type is detected correctly, especially for string vs. numeric exposures.
|
I've made the build and all the tests run properly. But practically testing it out is still difficult, I don't have a working development environment with all three pieces available. Is there a way to install a development branch/build next to existing HomeBridge installation properly? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/converters/air_purifier.ts (1)
55-83: Solid abstract base class for air purifier properties.
ThePassthroughAirPurifierPropertysets up a consistent pattern for your derived classes. UsingcopyExposesRangeToCharacteristicensures that the characteristic’s valid range aligns with Zigbee2MQTT definitions.Consider clarifying the property type of
this.stateby introducing an enum if the possible states are known enumerations rather than free-form numeric values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
src/converters/air_purifier.ts(1 hunks)
🔇 Additional comments (8)
src/converters/air_purifier.ts (8)
1-13: Imports look good and follow established patterns.
No issues appear with these imports; the references to./interfaces,../z2mModels,../hap,../helpers, andhomebridgeappear consistent with the rest of the codebase.
14-31: Good use of grouped endpoints to create Air Purifier services.
ThecreateServicesFromExposesmethod effectively organizes the relevant exposes by endpoint. This grouping ensures modular handling of each endpoint and avoids redundant service creation.
33-43: Proper error handling for service creation.
Catching errors increateServiceand logging a warning is a good practice to prevent entire service registration from failing silently.
45-48: Clear extension forcanUseExposesEntry.
DefiningWithExposesValidatorclarifies the contract that implementers must define acanUseExposesEntrymethod. This is a succinct approach for your property factories.
49-53: Interface design is straightforward.
DeclaringAirPurifierPropertywithupdateStateensures each property class has a uniform interface to update state based on raw data.
85-113: Consistent string-to-enumeration mapping forfan_state.
The logic correctly maps'ON'toPURIFYING_AIRand'OFF'toIDLE. Fallback toINACTIVEis sensible. ThehandleSetOnmethod properly queues'fan_state': 'ON'/'OFF'.
174-199:child_locklogic is consistent.
MappingLOCKandUNLOCKto a HomeKit characteristic for physical controls is fine. The fallback toCONTROL_LOCK_DISABLEDif not defined is a practical approach.
201-263: Well-structuredAirPurifierHandler.
The property factory approach centralizes all possible property classes inpropertyFactories, making it easy to add or remove features as needed. The error thrown when no suitable exposes are found is helpful for debugging. Overall, this design fits smoothly with the existing framework.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/converters/air_purifier.ts (4)
14-31: Perform additional validation of exposed entries.
This logic correctly filters byAirPurifierHandler.propertyFactories; however, consider logging additional details if no valid exposes are found for debugging.
33-43: Optionally log the error stack increateService.
Thetry/catchappropriately warns, but loggingerror.stackcan help with debugging.- accessory.log.warn( - 'Failed to setup Air Purifier service ' + `for accessory ${accessory.displayName} for endpoint ${endpoint}: ${error}` - ); + accessory.log.warn( + `Failed to setup Air Purifier service for accessory ${accessory.displayName} on endpoint ${endpoint}: `, + error + );
49-53: Consider renamingstateto avoid confusion.
The propertystateis numeric, which may conflict with other states in the code. A more explicit name could reduce confusion.
174-199: Clarify logic for lock states.
Currently, any non-null value forsensorValueis mapped toCONTROL_LOCK_ENABLED. This might conflict if future device states are introduced. A direct check for'LOCK'or'UNLOCK'might be safer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
src/converters/air_purifier.ts(1 hunks)
🔇 Additional comments (4)
src/converters/air_purifier.ts (4)
1-13: All imports and utilities look good.
No immediate issues spotted. The code references the necessary modules, and the helpers appear relevant.
45-47: Type declaration is straightforward.
No issues found.
55-83: Abstract base class shows good structure.
No critical issues identified. The approach foron('set', this.handleSetOn.bind(this))is acceptable given the design, though typically "current" states are read-only.
201-262: Good integration flow inAirPurifierHandler.
Keep an eye on potential mismatch between read-only vs. settable characteristics in HomeKit. Overall, the approach is sound.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/converters/air_purifier.ts (2)
136-136: Consider using numeric values instead of boolean castingThe HomeKit TargetAirPurifierState characteristic uses numeric values (0 for MANUAL, 1 for AUTO). The current implementation casts the value to boolean which could lead to unexpected behavior.
- data['fan_mode'] = (value as boolean) ? 'auto' : 'off'; + data['fan_mode'] = (value as number) === 1 ? 'auto' : 'manual';
164-168: Consider renaming the device property to avoid conflictTo avoid conflict with the TargetAirPurifierState property, consider using a different property name for speed settings rather than both modifying 'fan_mode'. This depends on how the device API is designed.
If the device supports it, consider using a dedicated property for speed:
- if (speed > 0) { - data['fan_mode'] = speed; - } else { - data['fan_mode'] = 'off'; - } + if (speed > 0) { + data['fan_speed'] = speed; + data['fan_mode'] = 'manual'; // Set to manual mode when speed is adjusted + } else { + data['fan_speed'] = 0; + data['fan_mode'] = 'off'; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
src/converters/air_purifier.ts(1 hunks)
🔇 Additional comments (7)
src/converters/air_purifier.ts (7)
85-113: Review Current Air Purifier State property implementationThe HomeKit CurrentAirPurifierState characteristic is typically designed to be read-only as it reflects the actual state of the device rather than a target state. Providing a setter method in
handleSetOnmight be unintuitive and could conflict with HomeKit's design patterns.
115-140: Inconsistent mapping between HomeKit and device statesThere's a misalignment between
convertToAirPurifier()andhandleSetOn(). The converter maps anything that's not "auto" toMANUAL(value 0), whilehandleSetOnmaps falsy values to "off". This means there's no clear path to a "manual" mode with a specific speed setting when toggling through HomeKit.
142-172: Potential state conflict with RotationSpeed and TargetAirPurifierStateBoth
RotationSpeedProperty.handleSetOn()andTargetAirPurifierStateProperty.handleSetOn()write to the same device property'fan_mode', but with different value types:
- RotationSpeed sets it to numeric values or 'off'
- TargetAirPurifierState sets it to 'auto' or 'off'
This can lead to inconsistent states depending on which control the user manipulates last.
1-43: LGTM: AirPurifierCreator implementation follows service patternThe creator class properly filters exposes entries, groups them by endpoint, and registers service handlers. The error handling in
createServiceis well-implemented.
55-83: Base implementation for property classes looks goodThe
PassthroughAirPurifierPropertyabstract class provides a solid foundation for the specific property implementations. The updateState method correctly handles state updates and propagates them to the HomeKit characteristics.
233-235: Good error handling for empty propertiesThe check for empty properties array with a meaningful error message is a good practice that helps with debugging.
201-262:✅ Verification successful
Code structure follows service handler pattern well
The AirPurifierHandler implementation follows the established pattern for service handlers in the project. It properly initializes properties based on exposes entries and provides appropriate error handling when no suitable entries are found.
🏁 Script executed:
#!/bin/bash # Verify the integration of AirPurifierCreator in the service creator manager echo "Checking for integration in creators.ts..." grep -A 5 "AirPurifierCreator" src/converters/creators.ts || echo "AirPurifierCreator might not be integrated in creators.ts" # Check for tests echo "Checking for tests..." find test -name "*air*purifier*" -o -name "*AirPurifier*"Length of output: 573
AirPurifierHandler Implementation Verified and Approved
The code follows the established service handler pattern. Integration of the AirPurifierCreator in the ServiceCreatorManager (as seen in
src/converters/creators.ts) is confirmed, and the error handling for missing exposes entries is appropriately implemented. No structural changes are needed. One minor note: while the integration check is positive, please ensure that dedicated tests for the air purifier functionality are added if they are not already part of your test suite.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/converters/air_purifier.ts (2)
127-132:⚠️ Potential issueIncorrect value type handling in
TargetAirPurifierStatesetter.HomeKit's
TargetAirPurifierStateuses numeric constants (0 for MANUAL, 1 for AUTO), but you're treating the incoming value as boolean. This can lead to unexpected behavior.handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; - data['fan_mode'] = (value as boolean) ? 'auto' : 'off'; + data['fan_mode'] = (value as number) === hap.Characteristic.TargetAirPurifierState.AUTO ? 'auto' : 'off'; this.accessory.queueDataForSetAction(data); callback(null); }
154-164:⚠️ Potential issuePotential conflict between properties writing to the same field.
Both
TargetAirPurifierStatePropertyandRotationSpeedPropertywrite to the same field (fan_mode), but with different value types. This can lead to inconsistent device behavior depending on which property was set most recently.Consider separating these concerns:
- Use different fields for different control aspects, or
- Implement a coordinated approach where one property updates based on the other's state.
For example:
handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; const speed = Math.floor((value as number)); if (speed > 0) { - data['fan_mode'] = speed; + data['fan_speed'] = speed; + data['fan_mode'] = 'manual'; // Signal that we're in manual mode with a specific speed } else { data['fan_mode'] = 'off'; } this.accessory.queueDataForSetAction(data); callback(null); }
🧹 Nitpick comments (3)
src/converters/air_purifier.ts (3)
186-191: Incorrect value type handling inLockPhysicalControlssetter.Similar to the
TargetAirPurifierStateissue, you're treating the incoming value as boolean, but HomeKit'sLockPhysicalControlscharacteristic uses numeric constants.handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; - data['child_lock'] = (value as boolean) ? 'LOCK' : 'UNLOCK'; + data['child_lock'] = (value as number) === hap.Characteristic.LockPhysicalControls.CONTROL_LOCK_ENABLED ? 'LOCK' : 'UNLOCK'; this.accessory.queueDataForSetAction(data); callback(null); }
55-83: Consider adding additional validation in the abstract property class.The
PassthroughAirPurifierPropertyclass provides a solid foundation, but adding additional validation for incoming values could improve robustness.updateState(state: Record<string, unknown>): void { if (this.expose.property in state) { const sensorValue = state[this.expose.property] as CharacteristicValue; if (sensorValue !== null && sensorValue !== undefined) { + // Log unexpected value types for debugging + const convertedValue = this.convertToAirPurifier(sensorValue); + if (convertedValue === undefined) { + this.accessory.log.debug(`Unexpected value for ${this.expose.property}: ${sensorValue}`); + } - this.state = this.convertToAirPurifier(sensorValue) ?? 0; + this.state = convertedValue ?? 0; this.service.updateCharacteristic(this.characteristic, this.state); } } }
224-228: Add more descriptive error for user debugging.When no suitable properties are found, providing more specific guidance about the required properties would help users debug configuration issues.
if (this.properties.length === 0) { - throw new Error(`Air Purifier (${serviceName}) did not receive any suitable exposes entries.`); + throw new Error(`Air Purifier (${serviceName}) did not receive any suitable exposes entries. ` + + `Required properties include: fan_state, fan_mode, fan_speed, or child_lock.`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
src/converters/air_purifier.ts(1 hunks)
🔇 Additional comments (3)
src/converters/air_purifier.ts (3)
1-13: LGTM - Import statements appropriately include all necessary components.The imports are well-organized and include all the necessary components for this implementation, with clear separation between local imports and external library imports.
14-43: LGTM - Well-structured service creator with proper error handling.The
AirPurifierCreatorclass follows the typical pattern for service creators in the codebase, with appropriate filtering of exposes entries and proper error handling in thecreateServicemethod.
194-255: LGTM - Handler implementation follows consistent pattern with good error reporting.The
AirPurifierHandlerimplementation follows a consistent pattern, with proper service configuration, clear property registration, and appropriate error handling. The state update mechanism is well-structured for propagating changes to all properties.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/converters/air_purifier.ts (2)
139-169:⚠️ Potential issueResolve conflict between RotationSpeedProperty and TargetAirPurifierStateProperty
Both classes write to the same property 'fan_mode' but with different value types:
- TargetAirPurifierStateProperty sets it to strings 'auto' or 'off'
- RotationSpeedProperty sets it to either a numeric speed or the string 'off'
This will cause one property to override the other, leading to inconsistent device states.
Consider implementing a coordinator pattern or establish precedence rules for which property takes priority. For example:
handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; const speed = Math.floor((value as number)); if (speed > 0) { - data['fan_mode'] = speed; + // Set both fan_mode to indicate manual mode and fan_speed for the actual speed + data['fan_mode'] = 'manual'; // or another appropriate value for manual mode + data['fan_speed'] = speed; } else { data['fan_mode'] = 'off'; } this.accessory.queueDataForSetAction(data); callback(null); }
112-137:⚠️ Potential issueFix boolean to numeric type handling in TargetAirPurifierStateProperty
The HomeKit spec defines
TargetAirPurifierStatecharacteristic as a numeric value where 0 = MANUAL and 1 = AUTO. However, your implementation is casting the value to boolean and then setting fan_mode to either 'off' or 'auto', which could lead to unexpected behavior.This was previously flagged but not completely addressed.
handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; - data['fan_mode'] = (value as boolean) ? 'auto' : 'off'; + // Handle correctly based on numeric HomeKit values + data['fan_mode'] = (value as number) === hap.Characteristic.TargetAirPurifierState.AUTO ? 'auto' : 'off'; this.accessory.queueDataForSetAction(data); callback(null); }
🧹 Nitpick comments (3)
src/converters/air_purifier.ts (3)
230-232: Consider graceful fallback for missing entriesThe handler throws an error if no suitable exposes entries are found. While this is a reasonable validation, consider implementing a fallback mechanism or more descriptive error message to help users troubleshoot the issue.
if (this.properties.length === 0) { - throw new Error(`Air Purifier (${serviceName}) did not receive any suitable exposes entries.`); + throw new Error(`Air Purifier (${serviceName}) did not receive any suitable exposes entries. Required properties include: ${AirPurifierHandler.propertyFactories.map(f => f.name).join(', ')}`); }
14-43: Add logging for better debuggingConsider adding more detailed logging in the createService method to help with debugging, especially since you mentioned in the PR that this is untested code.
private createService(endpoint: string | undefined, exposes: ExposesEntryWithProperty[], accessory: BasicAccessory): void { try { + accessory.log.debug(`Creating Air Purifier service for endpoint ${endpoint} with ${exposes.length} exposes entries`); const handler = new AirPurifierHandler(endpoint, exposes, accessory); accessory.registerServiceHandler(handler); + accessory.log.debug(`Successfully registered Air Purifier service for endpoint ${endpoint}`); } catch (error) { accessory.log.warn( 'Failed to setup Air Purifier service ' + `for accessory ${accessory.displayName} for endpoint ${endpoint}: ${error}` ); } }
171-196: Improve string value handling for LockPhysicalControlsPropertyThe implementation for LockPhysicalControlsProperty has good string handling in the convert method, but the setter could be more robust by handling non-boolean inputs.
handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; - data['child_lock'] = (value as boolean) ? 'LOCK' : 'UNLOCK'; + // Handle numeric values as well (0 = disabled, 1 = enabled) + const shouldLock = typeof value === 'boolean' ? value : + typeof value === 'number' ? value === 1 : false; + data['child_lock'] = shouldLock ? 'LOCK' : 'UNLOCK'; this.accessory.queueDataForSetAction(data); callback(null); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
src/converters/air_purifier.ts(1 hunks)
🔇 Additional comments (2)
src/converters/air_purifier.ts (2)
85-110: Ensure CurrentAirPurifierState is correctly synchronized with device stateThe CurrentAirPurifierState property implementation looks correct for a read-only characteristic. However, you should test carefully to ensure that the device state correctly maps to the HomeKit states (INACTIVE, IDLE, PURIFYING_AIR).
1-13: Consider adding unit testsSince you mentioned in the PR that this code is untested, I recommend adding unit tests to ensure proper functionality. This is particularly important for property conversions and state handling.
You can use the existing test infrastructure with
npm run testas mentioned by user itavero in the PR comments. This would help catch issues before merging.
Avamander
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/converters/air_purifier.ts (1)
131-136:⚠️ Potential issueInconsistency between HomeKit states and device commands.
The
handleSetOnmethod castsvalueas boolean and maps it to either 'auto' or 'off', but HomeKit's TargetAirPurifierState is actually a numeric enum (0=MANUAL, 1=AUTO). This could lead to unexpected behavior.handleSetOn(value: CharacteristicValue, callback: CharacteristicSetCallback): void { const data = {}; - data['fan_mode'] = (value as boolean) ? 'auto' : 'off'; + // HomeKit TargetAirPurifierState: 0=MANUAL, 1=AUTO + data['fan_mode'] = (value as number) === hap.Characteristic.TargetAirPurifierState.AUTO ? 'auto' : 'off'; this.accessory.queueDataForSetAction(data); callback(null); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
src/converters/air_purifier.ts(1 hunks)
🔇 Additional comments (6)
src/converters/air_purifier.ts (6)
1-43: Well-implemented ServiceCreator pattern.The AirPurifierCreator class correctly implements the ServiceCreator interface with proper error handling and endpoint grouping. The approach of filtering and transforming exposes entries aligns well with the existing patterns in the codebase.
88-90: Good fix for property type check.The property check has been properly updated to use
exposesHasPropertyinstead ofexposesHasNumericProperty, which correctly aligns with the string-based nature of 'fan_state' values ('ON', 'OFF').
107-109: Correct implementation of CurrentAirPurifierState's handleSetOn method.Implementing this method with a simple callback(null) is appropriate since CurrentAirPurifierState is typically read-only in HomeKit.
115-117: Good fix for property type check.The property check has been properly updated to use
exposesHasPropertyinstead ofexposesHasNumericProperty, which correctly aligns with the string-based nature of 'fan_mode' values.
174-176: Good fix for property type check.The property check has been properly updated to use
exposesHasPropertyinstead ofexposesHasNumericProperty, which correctly aligns with the string-based nature of 'child_lock' values ('LOCK', 'UNLOCK').
198-259: Well-structured service handler implementation.The AirPurifierHandler class properly manages the initialization of properties, service configuration, and state updates. The error handling when no suitable properties are found is a good safeguard.
|
@itavero In terms of remaining snags:
Unfortunately these last few snags I couldn't debug properly or they're not fixable from this side, there are way too many layers of abstraction to chew through. I'm considering it currently functionally maximally complete. |
|
@Avamander thank you! I’m looking forward to this code landing. 🥳 |
I tried to follow the lead of other ServiceCreators in this project in order to try and add initial air purifier support for the IKEA Starkvind.
Unfortunately I couldn't find build instructions that would let me actually properly run the new code alongside HomeBridge. This means this PR is currently totally untested. Feedback is still welcome though.
Summary by CodeRabbit
New Features
Bug Fixes