-
Notifications
You must be signed in to change notification settings - Fork 524
WWSTCERT-10225/10228/10231/10234 add driver to frient EMI devices #2730
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.
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
base: main
Are you sure you want to change the base?
WWSTCERT-10225/10228/10231/10234 add driver to frient EMI devices #2730
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 488 suites 0s ⏱️ Results for commit 3c48090. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 3c48090 |
| - title: "Pulse Configuration" | ||
| name: pulseConfiguration | ||
| description: "Number of pulses the meter outputs per unit" | ||
| required: false | ||
| preferenceType: integer | ||
| definition: | ||
| minimum: 50 | ||
| maximum: 10000 | ||
| default: 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulse Configuration is the number of pulses the meter outputs per unit. It may be different depending on the meter, so we need an option to adjust it, so that the device reads measurements properly from a wide range of meters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these profiles should have different names, I think.
The first two are going to be frient-specific because of the preferences, and the third has a misleading name since it uses components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,15 @@ | |||
| -- Copyright 2025 SmartThings, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| attribute = SimpleMetering.attributes.CurrentSummationDelivered.ID, | ||
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = data_types.Uint48, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this and the above need to be Uint48?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what base data type for these attruibutes is
| @@ -0,0 +1,362 @@ | |||
| -- Copyright 2025 SmartThings, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,8 @@ | |||
| -- Copyright 2025 SmartThings, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,15 @@ | |||
| -- Copyright 2025 SmartThings, Inc. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| } | ||
| }, | ||
| sub_drivers = { | ||
| require("frient/EMIZB-151") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match formatting -> frient.EMIZB-151
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,340 @@ | |||
| -- Copyright 2025 SmartThings | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,303 @@ | |||
| -- Copyright 2025 SmartThings | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -0,0 +1,396 @@ | |||
| -- Copyright 2025 SmartThings | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| lazy_load_if_possible("frient"), | ||
| lazy_load_if_possible("shinasystems"), | ||
| lazy_load_if_possible("bituo"), | ||
| lazy_load_if_possible("frient/EMIZB-151") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistake, I think this should live as a subdriver within the frient init. Thoughts @greens ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to subdriver section
| deviceProfileName: power-energy-consumption-report | ||
| - id: "frient A/S/EMIZB-132" | ||
| deviceLabel: frient Energy Monitor | ||
| manufacturer: Develco Products A/S |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these no longer necessary?
| raw_value = raw_value * multiplier / divisor * 1000 | ||
|
|
||
| -- The result is already in watts, no need to multiply by 1000 | ||
| device:emit_component_event(device.profile.components['main'], capabilities.powerMeter.power({ value = raw_value, unit = "W" })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when emitting for the main component, you can just use device:emit_event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this seems identical to the defaults except for the default value, which you could set elsewhere (like in added) with
device:set_field(zigbee_constants.SIMPLE_METERING_DIVISOR_KEY, SIMPLE_METERING_DEFAULT_DIVISOR)
instead of overriding the default behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason the configuration of attribute does not work without this handler here
| device:send(ElectricalMeasurement.attributes.ACPowerDivisor:read(device)) | ||
| device:send(ElectricalMeasurement.attributes.ACPowerMultiplier:read(device)) | ||
| device:send(ElectricalMeasurement.attributes.ACVoltageMultiplier:read(device)) | ||
| device:send(ElectricalMeasurement.attributes.ACVoltageDivisor:read(device)) | ||
| device:send(ElectricalMeasurement.attributes.ACCurrentMultiplier:read(device)) | ||
| device:send(ElectricalMeasurement.attributes.ACCurrentDivisor:read(device)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect these should be sent already by device:refresh() since they're configured attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| if divisor == 0 then | ||
| divisor = 1 | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking for 0 every time here, you could just make sure that when you set a divisor, you just throw out the value if it's 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| local active_power_handler = function(component) | ||
| local handler = function(driver, device, value, zb_rx) | ||
| local raw_value = value.value | ||
| -- By default emit raw value | ||
| local multiplier = device:get_field(zigbee_constants.ELECTRICAL_MEASUREMENT_MULTIPLIER_KEY) or 1 | ||
| local divisor = device:get_field(zigbee_constants.ELECTRICAL_MEASUREMENT_DIVISOR_KEY) or 1 | ||
|
|
||
| if divisor == 0 then | ||
| divisor = 1 | ||
| end | ||
|
|
||
| raw_value = raw_value * multiplier / divisor | ||
|
|
||
| device:emit_component_event(device.profile.components[component], capabilities.powerMeter.power({ value = raw_value, unit = "W" })) | ||
| end | ||
|
|
||
| return handler | ||
| end | ||
|
|
||
| local rms_voltage_handler = function(component) | ||
| local handler = function(driver, device, value, zb_rx) | ||
| local raw_value = value.value | ||
| -- By default emit raw value | ||
| local multiplier = device:get_field(zigbee_constants.ELECTRICAL_MEASUREMENT_AC_VOLTAGE_MULTIPLIER_KEY) or 1 | ||
| local divisor = device:get_field(zigbee_constants.ELECTRICAL_MEASUREMENT_AC_VOLTAGE_DIVISOR_KEY) or 1 | ||
|
|
||
| if divisor == 0 then | ||
| divisor = 1 | ||
| end | ||
|
|
||
| raw_value = raw_value * multiplier / divisor | ||
|
|
||
| device:emit_component_event(device.profile.components[component], capabilities.voltageMeasurement.voltage({ value = raw_value, unit = "V" })) | ||
| end | ||
|
|
||
| return handler | ||
| end | ||
|
|
||
| local rms_current_handler = function(component) | ||
| local handler = function(driver, device, value, zb_rx) | ||
| local raw_value = value.value | ||
| -- By default emit raw value | ||
| local multiplier = device:get_field(zigbee_constants.ELECTRICAL_MEASUREMENT_AC_CURRENT_MULTIPLIER_KEY) or 1 | ||
| local divisor = device:get_field(zigbee_constants.ELECTRICAL_MEASUREMENT_AC_CURRENT_DIVISOR_KEY) or 1 | ||
|
|
||
| if divisor == 0 then | ||
| divisor = 1 | ||
| end | ||
|
|
||
| raw_value = raw_value * multiplier / divisor | ||
|
|
||
| device:emit_component_event(device.profile.components[component], capabilities.currentMeasurement.current({ value = raw_value, unit = "A" })) | ||
| end | ||
|
|
||
| return handler | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these three could be consolidated further by having the mul/div keys, unit, and the attribute as arguments as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| raw_value = 1 | ||
| end | ||
|
|
||
| device:set_field(zigbee_constants.SIMPLE_METERING_DIVISOR_KEY, raw_value, { persist = true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing a lot of added value over the default handling here.
Is the device often sending these updates with the mfg-specific bit set? It shouldn't be, since this is a standard attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did not send it often. Switched back to default handler
| { | ||
| cluster = SimpleMetering.ID, | ||
| attribute = SimpleMetering.attributes.InstantaneousDemand.ID, | ||
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = data_types.Int24, | ||
| reportable_change = 1 | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default reportable_change is 5. We want it to be as sensitive as possible
| { | ||
| cluster = ElectricalMeasurement.ID, | ||
| attribute = ElectricalMeasurement.attributes.ActivePower.ID, | ||
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = data_types.Int16, | ||
| reportable_change = 5 | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| { | ||
| cluster = SimpleMetering.ID, | ||
| attribute = SimpleMetering.attributes.CurrentSummationDelivered.ID, | ||
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = data_types.Uint48, | ||
| reportable_change = 1 | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as the defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| zigbee_constants.ELECTRICAL_MEASUREMENT_AC_VOLTAGE_MULTIPLIER_KEY = "_electrical_measurement_ac_voltage_multiplier" | ||
| zigbee_constants.ELECTRICAL_MEASUREMENT_AC_CURRENT_MULTIPLIER_KEY = "_electrical_measurement_ac_current_multiplier" | ||
| zigbee_constants.ELECTRICAL_MEASUREMENT_AC_VOLTAGE_DIVISOR_KEY = "_electrical_measurement_ac_voltage_divisor" | ||
| zigbee_constants.ELECTRICAL_MEASUREMENT_AC_CURRENT_DIVISOR_KEY = "_electrical_measurement_ac_current_divisor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd feel more comfortable if you just made these strings local to this driver rather than writing to the zigbee_constants map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it
| -- Copyright 2025 SmartThings | ||
| -- | ||
| -- Licensed under the Apache License, Version 2.0 (the "License"); | ||
| -- you may not use this file except in compliance with the License. | ||
| -- You may obtain a copy of the License at | ||
| -- | ||
| -- http://www.apache.org/licenses/LICENSE-2.0 | ||
| -- | ||
| -- Unless required by applicable law or agreed to in writing, software | ||
| -- distributed under the License is distributed on an "AS IS" BASIS, | ||
| -- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| -- See the License for the specific language governing permissions and | ||
| -- limitations under the License. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update this copyright/license statement to the shorter and more concise one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| local ATTRIBUTES = { | ||
| { | ||
| cluster = SimpleMetering.ID, | ||
| attribute = SimpleMetering.attributes.CurrentSummationDelivered.ID, | ||
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = data_types.Uint48, | ||
| reportable_change = 1 | ||
| }, | ||
| { | ||
| cluster = SimpleMetering.ID, | ||
| attribute = SimpleMetering.attributes.InstantaneousDemand.ID, | ||
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = data_types.Int24, | ||
| reportable_change = 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both the same as the defaults as far as I can tell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difference in reportable change for InstantaneousDemand. I removed the other one
| device.thread:call_with_delay(5, function() | ||
| do_refresh(self, device) | ||
| end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to call refresh 5 seconds after you just called it at the top of this function?
| device:configure() | ||
|
|
||
| if device:supports_capability(capabilities.battery) then | ||
| device:send(PowerConfiguration.attributes.BatteryVoltage:configure_reporting(device, 30, 21600, 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not handled by device:configure already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. Removed
| device:send(PowerConfiguration.attributes.BatteryVoltage:configure_reporting(device, 30, 21600, 1)) | ||
| end | ||
| for _, fingerprint in ipairs(ZIGBEE_POWER_METER_FINGERPRINTS) do | ||
| if device:get_model() == fingerprint.model and fingerprint.preferences then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't you just check for the existence of the preference itself rather than including a separate boolean in the fingerprints map?
| local function instantaneous_demand_handler(driver, device, value, zb_rx) | ||
| local raw_value = value.value | ||
| --- demand = demand received * Multipler/Divisor | ||
| local multiplier = device:get_field(zigbee_constants.SIMPLE_METERING_MULTIPLIER_KEY) or 1 | ||
| local divisor = device:get_field(zigbee_constants.SIMPLE_METERING_DIVISOR_KEY) or SIMPLE_METERING_DEFAULT_DIVISOR | ||
| if raw_value < -8388607 or raw_value >= 8388607 then | ||
| raw_value = 0 | ||
| end | ||
|
|
||
| raw_value = raw_value * multiplier / divisor * 1000 | ||
|
|
||
| local raw_value_watts = raw_value | ||
| device:emit_event_for_endpoint(zb_rx.address_header.src_endpoint.value, capabilities.powerMeter.power({ value = raw_value_watts, unit = "W" })) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems functionally the same as the default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The device sometimes sends weird values, so we added a guard to ignore these values
| device:emit_event_for_endpoint(zb_rx.address_header.src_endpoint.value, capabilities.powerMeter.power({ value = raw_value_watts, unit = "W" })) | ||
| end | ||
|
|
||
| local function energy_meter_handler(driver, device, value, zb_rx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's different between this handler and the sub-driver's version?
|
|
||
| local device_init = function(self, device) | ||
| for _, fingerprint in ipairs(ZIGBEE_POWER_METER_FINGERPRINTS) do | ||
| if device:get_model() == fingerprint.model and fingerprint.battery then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just check for whether MIN_BAT exists rather than use the extra boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. Changed
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests