Ledvance zigbee EM EU plug

[LQ107]

Check all that apply

Type of Change

• WWST Certification Request
  • If this is your first time contributing code:
    • I have reviewed the README.md file
    • I have reviewed the CODE_OF_CONDUCT.md file
    • I have signed the CLA
  • I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
• Bug fix
• New feature
• Refactor

Checklist

• I have performed a self-review of my code
• I have commented my code in hard-to-understand areas
• I have verified my changes by testing with a device or have communicated a plan for testing
• I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

Summary of Completed Tests

[LQ107]

I had submit the Smartthings certification ,please help review the drive ,thank you very much .

[aleclorimer]

min_api_version fixes and resolving merge conflicts, but LGTM

[LQ107]

I have add the min_api_version opt value for each test , please help review again ,thank you very much .

[cjswedes]

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.

[LQ107]

yes , the device report SimpleMetering.Divisor or the SimpleMetering.Multiplier attributes

[cjswedes]

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.

[LQ107]

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

[cjswedes]

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:

SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-switch/src/lifecycle_handlers/do_configure.lua

Lines 13 to 19 in 917bdad

	-- Additional one time configuration
	if device:supports_capability(capabilities.energyMeter) or device:supports_capability(capabilities.powerMeter) then
	local clusters = require "st.zigbee.zcl.clusters"
	-- Divisor and multipler for EnergyMeter
	device:send(clusters.SimpleMetering.attributes.Divisor:read(device))
	device:send(clusters.SimpleMetering.attributes.Multiplier:read(device))
	end

Only a fingerprint change is necessary

[LQ107]

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]

Dear reviewers, I have added the min_api_version. Could you please help review again? Thank you very much.

[cjswedes]

The changes here are looking pretty good, but this subdriver should be a subdriver of the zigbee-switch-power subdriver. The aurora-relay subdriver exists for essentially the same reason; just to set the SIMPLE_METERING scaling fields.

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.

[LQ107]

The changes here are looking pretty good, but this subdriver should be a subdriver of the zigbee-switch-power subdriver. The aurora-relay subdriver exists for essentially the same reason; just to set the SIMPLE_METERING scaling fields.

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?

[LQ107]

The changes here are looking pretty good, but this subdriver should be a subdriver of the zigbee-switch-power subdriver. The aurora-relay subdriver exists for essentially the same reason; just to set the SIMPLE_METERING scaling fields.
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]

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]

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:

SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-switch/src/lifecycle_handlers/do_configure.lua

Lines 13 to 19 in 42a6a23

	-- Additional one time configuration
	if device:supports_capability(capabilities.energyMeter) or device:supports_capability(capabilities.powerMeter) then
	local clusters = require "st.zigbee.zcl.clusters"
	-- Divisor and multipler for EnergyMeter
	device:send(clusters.SimpleMetering.attributes.Divisor:read(device))
	device:send(clusters.SimpleMetering.attributes.Multiplier:read(device))
	end

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.

[LQ107]

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:

SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-switch/src/lifecycle_handlers/do_configure.lua

Lines 13 to 19 in 42a6a23

	-- Additional one time configuration
	if device:supports_capability(capabilities.energyMeter) or device:supports_capability(capabilities.powerMeter) then
	local clusters = require "st.zigbee.zcl.clusters"
	-- Divisor and multipler for EnergyMeter
	device:send(clusters.SimpleMetering.attributes.Divisor:read(device))
	device:send(clusters.SimpleMetering.attributes.Multiplier:read(device))
	end

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 .

[cjswedes]

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 zigbee-switch-power subdriver (i.e. vimar, aurora-relay), please move this subdriver to be nested under zigbee-switch-power.