Ledvance zigbee EM EU plug#3009
Conversation
…dd driver registration processing
…/SmartThingsEdgeDrivers into ledvance_zigbee_meter_plug
…ter_handler issue
…/SmartThingsEdgeDrivers into ledvance_zigbee_meter_plug
…ation of the test file
…/SmartThingsEdgeDrivers into ledvance_zigbee_meter_plug
LQ107
left a comment
There was a problem hiding this comment.
I have add the min_api_version opt value for each test , please help review again ,thank you very much .
There was a problem hiding this comment.
Does the device report SimpleMetering.Divisor or the SimpleMetering.Multiplier attributes? The defaults will query these and set them if the device reports them.
If that is the case, this could be a fingerprint only change.
There was a problem hiding this comment.
yes , the device report SimpleMetering.Divisor or the SimpleMetering.Multiplier attributes
There was a problem hiding this comment.
Since this is being reported by the device, the subdriver is not necessary. Please make this a fingerprint only change or explain why overriding the reported value on device init is necessary for the integration.
There was a problem hiding this comment.
Since the values of SimpleMetering.Divisor or SimpleMetering.Multiplier used by the device are different from those of the Smarthings standard driver, a custom sub driver needs to be used
There was a problem hiding this comment.
I see now our defaults arent actually issuing a read or configuring those attributes. So I see why the subdriver is needed, and there is some precedence of just setting these values based on device model.
There is some precedence of setting these values (i.e. aurora-relay subdriver), but that should not be needed. The doConfigure handler for the zigbee switch driver issues reads for these values and the default handlers set the fields:
Only a fingerprint change is necessary
There was a problem hiding this comment.
Thanks for answer ,If only a fingerprint is added to the main driver, the plug cannot display the correct energy meter value.
The PLUG COMPACT EU EM T has passed SmartThings Certification. Previously, when I verified this model by adding only a fingerprint, the displayed energy meter value was 100 times the actual value. Therefore, a sub-driver needs to be added for this model.
Since the PLUG EU EM T is the same as the PLUG COMPACT EU EM T, I have added the sub-driver fingerprint for the PLUG EU EM T as well.
LQ107
left a comment
There was a problem hiding this comment.
Dear reviewers, I have added the min_api_version. Could you please help review again? Thank you very much.
|
Edit: I see that we do read the SimpleMetering.Divisor and SimpleMetering.Multiplier attributes in the zigbee switch default configure, so I dont think this subdriver is needed; those reads will trigger the default handlers when the device is joined which persist that value. This should be a fingerprint only driver. Please test adding just the fingerprint. |
I only add a fingerprint only driver, but show energy is incorrect , the displayed energy meter value was 100 times the actual value , I judge the values of SimpleMetering.Divisor or SimpleMetering.Multiplier used by the device are different from those of the Smarthings standard driver ,how can I solve this issue? |
I reset the PLUG and add the PLUG to the Smartthings HUB again ,the Plug add a fingerprint only driver,the Plug can show the correct Energy meter ,thank you very much ,I will submit the driver again . |
LQ107
left a comment
There was a problem hiding this comment.
Dear reviewers, thank you for your comments. I have updated the 'PLUG EU EM T' driver by adding the missing device fingerprints only. No other logic has been changed. Please help review it again. Thanks.
cjswedes
left a comment
There was a problem hiding this comment.
I am not convinced this subdriver is needed. I would expect the attributes to be read by the default do_configure handler of the zigbee switch driver:
And then since the zigbee-switch driver registers for default handlers for powerMeter and energyMeter, I would expect responses from the device to end up setting the field.
I was able to remove the ledvance-metering-plug subdriver from the top level subdrivers file, meaning it is not loaded at all. I then wrote a test validating that this is the behavior when only adding the fingerprint to the driver:
local mock_device = test.mock_device.build_test_zigbee_device(
{
profile = t_utils.get_profile_definition("switch-power-energy.yml"),
zigbee_endpoints = {
[1] = {
id = 1,
manufacturer = "LEDVANCE",
model = "PLUG COMPACT EU EM T",
server_clusters = { 0x0006, 0x0702 }
}
}
}
)
test.register_coroutine_test(
"configure should set the fields if the device response to the read requests",
function()
test.socket.zigbee:__set_channel_ordering("relaxed")
assert(mock_device:get_field(zigbee_constants.SIMPLE_METERING_MULTIPLIER_KEY) == nil)
assert(mock_device:get_field(zigbee_constants.SIMPLE_METERING_DIVISOR_KEY) == nil)
--- Do configure by default issues the reads for the device.
test.socket.device_lifecycle:__queue_receive({ mock_device.id, "doConfigure" })
test.socket.zigbee:__expect_send({
mock_device.id,
SimpleMetering.attributes.Divisor:read(mock_device)
})
test.socket.zigbee:__expect_send({
mock_device.id,
SimpleMetering.attributes.Multiplier:read(mock_device)
})
test.socket.zigbee:__expect_send({
mock_device.id,
SimpleMetering.attributes.CurrentSummationDelivered:read(mock_device)
})
test.socket.zigbee:__expect_send({
mock_device.id,
SimpleMetering.attributes.InstantaneousDemand:read(mock_device)
})
test.socket.zigbee:__expect_send({
mock_device.id,
OnOff.attributes.OnOff:read(mock_device)
})
test.socket.zigbee:__expect_send({
mock_device.id,
zigbee_test_utils.build_bind_request(
mock_device,
zigbee_test_utils.mock_hub_eui,
SimpleMetering.ID
)
})
test.socket.zigbee:__expect_send({
mock_device.id,
zigbee_test_utils.build_bind_request(
mock_device,
zigbee_test_utils.mock_hub_eui,
OnOff.ID
)
})
test.socket.zigbee:__expect_send({
mock_device.id,
SimpleMetering.attributes.InstantaneousDemand:configure_reporting(
mock_device, 5, 3600, 5
)
})
test.socket.zigbee:__expect_send({
mock_device.id,
SimpleMetering.attributes.CurrentSummationDelivered:configure_reporting(
mock_device, 5, 3600, 1
)
})
test.socket.zigbee:__expect_send({
mock_device.id,
OnOff.attributes.OnOff:configure_reporting(
mock_device, 0, 0x12C
)
})
mock_device:expect_metadata_update({ provisioning_state = "PROVISIONED" })
assert(mock_device:get_field(zigbee_constants.SIMPLE_METERING_MULTIPLIER_KEY) == nil)
assert(mock_device:get_field(zigbee_constants.SIMPLE_METERING_DIVISOR_KEY) == nil)
-- If the device responds to the reads, the driver will save the new values,
test.socket.zigbee:__queue_receive({ mock_device.id, SimpleMetering.attributes.Divisor:build_test_attr_report(mock_device, 100) })
test.socket.zigbee:__queue_receive({ mock_device.id, SimpleMetering.attributes.Multiplier:build_test_attr_report(mock_device, 1) })
test.wait_for_events()
assert(mock_device:get_field(zigbee_constants.SIMPLE_METERING_MULTIPLIER_KEY) == 1)
assert(mock_device:get_field(zigbee_constants.SIMPLE_METERING_DIVISOR_KEY) == 100)
end
)
I packaged the driver here (https://bestow-regional.api.smartthings.com/invite/OzMgLmVWRD29). Please test with the real device.
I would prefer that this PR removes the ledvance-metering-plug subdriver and just adds the fingerprints for the devices. The only thing preventing that is if the devices do not respond to the read requests the driver issues for the multiplier and divisor.
Thanks for your feedback ,I use the driver that you packaged ,I found a issue ,that the Energy meter value displayed is incorrect ,the above screenshot show the energy meter is 7.0kWh, but the actual energy meter is 70.0Wh , and I change the driver to the sub driver and the Energy is displayed correct ,the energy meter value is 70.0Wh , and the following screenshot is the Plug change driver to sub driver . I think the PLUG COMPACT EU EM T can keep the current sub driver ,thank you very much . |
|
Thank you for your prompt tests and responses. Lets keep the subdriver, I am not sure why its not working as expected. Since there are other subdrivers that do similar things in the |
Thanks for your feedback ,I will move sub driver to zigbee-switch-power |
LQ107
left a comment
There was a problem hiding this comment.
Dear Reviewers,
Thank you for your comments. I have moved the sub-driver to the zigbee-switch-power directory and removed the old sub-driver load path. Please review it again. Thank you very much for your time.
cjswedes
left a comment
There was a problem hiding this comment.
This is very close to merging. Conflicts need to be resolved.
Thanks for you comment , whether the min_api_versio=17 is correct? |
|
Invitation URL: |
Test Results 73 files ±0 515 suites ±0 0s ⏱️ ±0s For more details on these errors, see this check. Results for commit 73632d4. ± Comparison against base commit 42a6a23. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 73632d4 |
|
Dear Reviewers, |
LQ107
left a comment
There was a problem hiding this comment.
Dear Reviewers, I have updated the sub-driver. Could you please review it again? Thank you very much.
cjswedes
left a comment
There was a problem hiding this comment.
LGTM once tests are passing
| zigbee_test_utils.prepare_zigbee_env_info() | ||
|
|
||
| local function test_init() | ||
| test.disable_startup_messages() |
There was a problem hiding this comment.
This is one reason tests are failing (the startup messages should be disabled, otherwise init runs automatically). The other reason is because since this is now nested under the zigbee-switch-power subdriver, you need to add the ledvance fingerprints to that list of fingerprints also https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/blob/main/drivers/SmartThings/zigbee-switch/src/zigbee-switch-power/fingerprints.lua


Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests