Update range of characteristic if out of range numeric value is received #985
Conversation
…ceived from Zigbee2MQTT
WalkthroughThis pull request introduces a comprehensive update to logging and characteristic handling across multiple converter classes. The changes primarily focus on enhancing logging capabilities by passing the accessory's log object to monitor callbacks. Additionally, the code now includes a mechanism to automatically update numeric characteristic ranges when out-of-range values are received from Zigbee2MQTT. The modifications span various sensor and device handler classes, including switches, locks, lights, climate controls, and battery handlers. Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
🪧 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 (2)
src/converters/battery.ts (1)
144-144: Enhance error handling or gracefully handle exceptions in monitors
Passing the logger to each monitor is helpful for debugging. However, consider whether any monitor could throw an exception within its callback; if so, you might want to wrap calls in a try/catch block to gracefully log errors.CHANGELOG.md (1)
10-13: Clarify the behavior for out-of-range automatic updates.
Great addition describing automatic range updates. Consider explicitly stating how the new range gets recalculated (e.g., is it set to the min/max boundary or extended beyond the defined limit?).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)src/converters/action.ts(3 hunks)src/converters/basic_sensors/basic.ts(3 hunks)src/converters/battery.ts(1 hunks)src/converters/climate.ts(1 hunks)src/converters/cover.ts(1 hunks)src/converters/light.ts(1 hunks)src/converters/lock.ts(1 hunks)src/converters/monitor.ts(3 hunks)src/converters/switch.ts(1 hunks)
🔇 Additional comments (18)
src/converters/basic_sensors/basic.ts (4)
7-7: Good addition of the logger import.
ImportingBasicLoggeris consistent with the new logging enhancements.
14-14: Appropriate addition of thelogproperty.
Having a dedicated logger at the handler level is beneficial for debugging sensor state updates.
30-30: Verify the presence of a valid logger instance.
While assigningthis.log = accessory.logis straightforward, ensure thataccessory.logis properly defined and notundefinedin edge cases.
98-98: Enhanced logging within the update flow.
Passing the logger to each monitor’s callback is a convenient way to centralize debugging information. Looks good.src/converters/switch.ts (1)
119-119: Effective logging parameter for monitor callbacks.
Forwardingthis.accessory.logto the callback helps maintain consistent logging across different state updates.src/converters/action.ts (4)
9-9: Consistent logger import for unified logging.
This import matches the approach used in other handlers, enabling uniform logging features.
58-58: Clear introduction of thelogproperty.
Defining aprivate readonly logensures consistent use of the same logger instance throughout the handler.
66-66: Proper assignment of the logger instance.
Makes all logging calls easy to maintain and read.
119-119: Logging support in state updates.
Passingthis.logto the monitor’s callback improves visibility and debugging for action triggers.src/converters/lock.ts (1)
130-130: Consistent lock-state logging approach.
Includingthis.accessory.login the callbacks ensures the lock’s state transitions can be clearly traced.src/converters/monitor.ts (5)
2-2: Validate the necessity of theBasicLoggerimport
The added import forBasicLoggeris appropriate since we are now leveraging this type in the monitor callbacks. No issues here.
7-7: Updated interface signature
Adding thelogger: BasicLoggerparameter to the monitor’scallbackfunction broadens logging capabilities across different monitors. This interface change looks consistent with the rest of the PR.
19-26: Potential validation of transformed value
Withincallback, the transformed value is passed toupdateCharacteristic. Consider handling unexpected numeric ranges or invalid string conversions by applying further checks or catching thrown errors intransformValueFromMqtt.
32-52: Checks and updates for numeric-in-range
This newupdateRangeIfNeededmethod ensures characteristics adapt dynamically to unexpected out-of-range values. This is valuable for robust device handling. However, be mindful of how frequent range changes can shape user experience in HomeKit, as it might confuse users if min/max are constantly changing.
65-65: Nested monitor callbacks
The nested callback forwarding of theloggeris consistent with the new interface. This design preserves the logging context across deeper or compound monitors.src/converters/climate.ts (1)
271-271: Consistent logging enhancement
Forwarding the accessory’s log to each monitor standardizes debugging output and ensures consistent logging across climate-related monitors. No issues noted.src/converters/cover.ts (1)
203-203: Uniform logging approach
Similar to the other handlers, passingthis.accessory.logto each monitor ensures consistent logging and helps in diagnosing cover position updates or related issues. This change appears coherent with the rest of the system updates.src/converters/light.ts (1)
227-227: Ensure all monitors accept thelogargument.
This line passesthis.accessory.logas a second argument to each monitor’s callback. Please confirm that allCharacteristicMonitorimplementations are updated to handle two arguments, to avoid runtime errors.Run this script to search for
callback(signatures in the codebase and review if they all declare the second parameter:✅ Verification successful
Let me try a different search strategy to find monitor callback implementations.
Let me fix the regex syntax and try another search approach.
All monitor callbacks correctly handle the logger parameter
Based on the search results, I can verify that all
CharacteristicMonitorimplementations are safe. Here's why:
- The
CharacteristicMonitorinterface insrc/converters/monitor.tsexplicitly defines the callback signature with both parameters:export interface CharacteristicMonitor { callback(state: Record<string, unknown>, logger: BasicLogger): void; }
- All monitor implementations extend either
BaseCharacteristicMonitoror directly implementCharacteristicMonitor:
BaseCharacteristicMonitor(abstract base class)NestedCharacteristicMonitorPassthroughCharacteristicMonitorMappingCharacteristicMonitorBinaryConditionCharacteristicMonitorNumericCharacteristicMonitorColorTemperatureToHueSatMonitorColorXyCharacteristicMonitorSince TypeScript enforces interface compliance, all these implementations must correctly handle both parameters of the callback method, including the logger parameter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that monitors accept the second parameter (logger). ast-grep --pattern $'function callback($_, $_) { $$$ }'Length of output: 58
Script:
#!/bin/bash # Search for class implementations containing 'callback' ast-grep --pattern 'class $_ implements CharacteristicMonitor { $$$ callback($$$) { $$$ } $$$ }' # Also search for direct callback implementations rg "callback.*\(.*\).*{" -A 2Length of output: 244
Script:
#!/bin/bash # Search for callback implementations rg "callback\s*\([^)]*\)\s*{" -A 2 # Also search for CharacteristicMonitor interface/type definition rg "CharacteristicMonitor" -A 5Length of output: 29889
|
|
🆗 Published SonarCloud and code coverage data. |
|
🚀 This pull request is included in v1.11.0-beta.8. See Release 1.11.0-beta.8 for release notes. |
Summary by CodeRabbit
Summary by CodeRabbit
Changelog Updates
Logging Improvements
Error Handling