Allow reporting minimum to be specified in device file for electricityMeter configuration #8123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Koenkk
reviewed
Oct 13, 2024
src/lib/modernExtend.ts
Outdated
| export interface ElectricityMeterArgs { | ||
| cluster?: 'both' | 'metering' | 'electrical'; | ||
| current?: false | MultiplierDivisor; | ||
| power?: false | MultiplierDivisor; | ||
| power?: false | MultiplierDivisorMin; |
There was a problem hiding this comment.
Suggested change
| power?: false | MultiplierDivisorMin; | |
| power?: false | (MultiplierDivisor & ReportingConfigWithoutAttribute); |
There was a problem hiding this comment.
I can't use ReportingConfigWithoutAttribute because its elements of min/max/change aren't optional so would need to allow all three to be specified. I was trying to make this PR the lightest touch possible. This works.
power?: false | (MultiplierDivisor & {min?: ReportingConfigTime});
Koenkk
reviewed
Oct 13, 2024
src/lib/modernExtend.ts
Outdated
| @@ -1836,7 +1837,7 @@ export function electricityMeter(args?: ElectricityMeterArgs): ModernExtend { | |||
| // Check if this property has a divisor and multiplier, e.g. AC frequency doesn't. | |||
| if ('divisor' in property) { | |||
| // In case multiplier or divisor was provided, use that instead of reading from device. | |||
| if (property.forced) { | |||
| if (property.forced && property.forced.divisor) { | |||
There was a problem hiding this comment.
Suggested change
| if (property.forced && property.forced.divisor) { | |
| if (property.forced?.divisor || property.forced?.multiplier) { |
There was a problem hiding this comment.
Even with the optional property chaining the compiler complains that:
Property 'divisor' does not exist on type 'false | MultiplierDivisor | (MultiplierDivisor & { min?: ReportingConfigTime; })'.
Property 'divisor' does not exist on type 'false'.ts(2339)
Only fix I could find was to include a test for forced:
if (property.forced && (property.forced.divisor || property.forced.multiplier)) {
Koenkk
reviewed
Oct 13, 2024
Koenkk
reviewed
Oct 15, 2024
Koenkk
reviewed
Oct 15, 2024
Koenkk
reviewed
Oct 15, 2024
Co-authored-by: Koen Kanters <koenkanters94@gmail.com>
Co-authored-by: Koen Kanters <koenkanters94@gmail.com>
|
I updated the code a bit to also allow specifying the |
Merged
|
Yes, this all looks good and it tests ok with my E13-A21 bulb. Thanks for your help with it. |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Sengled E13-A21 smart bulb will not report instantaneous demand unless the reporting min is set to 5 seconds or lower. The Sengled hub sets it to 3 seconds but testing shows that 5 seconds is ok. Setting it to 6 seconds or longer stops reporting.
This change allows the min value to be set in the device configuration. This was proposed here #8045 (comment)
Also included is a change to the default change value for instantaneous demand from 5 to 0.005 (kWh) as raised here #7917
Note that the Sengled bulb has a max power consumption of 12.5W so a reporting change value of 5W is still rather large and it therefore only reports on big dimmer adjustments. However I can live with that and just set that change value via the UI. The PR for the min value however isn't about optimal value, the device just won't report with the current default of 10 seconds.