Skip to content

Update lifecycle handling for Eve Energy and Aqara subdrivers #2116

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nickolas-deboom
Copy link
Contributor

@nickolas-deboom nickolas-deboom commented May 9, 2025

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

With the changes from
#2041, the matter switch subdrivers for Eve and Aqara should be updated to maintain consistency. This involves:

  • Moving the initialization code from device_init into do_configure
  • Implement the driverSwitched lifecycle event
  • Additionally, the TEST_CONFIGURE field can be removed from the Aqara subdriver, leveraging a technique used in other test files to set device.profile.id for a mock device.
  • A few other minor changes to increase consistency between subdrivers

Lastly, while testing the Aqara Cube T1 Pro, it was found that the LongPress event was being emitted in place of InitialPress. This PR add support for handling LongPress, in case different versions of the device emit different event types.

Note that some of the changes are purely cosmetic meant to increase consistency across the subdrivers and main driver.

Summary of Completed Tests

Tested with Eve Energy and Aqara Cube.

Copy link

github-actions bot commented May 9, 2025

Copy link

github-actions bot commented May 9, 2025

Test Results

   66 files    426 suites   0s ⏱️
2 185 tests 2 185 ✅ 0 💤 0 ❌
3 735 runs  3 735 ✅ 0 💤 0 ❌

Results for commit 57452e8.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented May 9, 2025

File Coverage
All files 91%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/embedded-cluster-utils.lua 38%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/init.lua 92%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/aqara-cube/init.lua 92%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/eve-energy/init.lua 90%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 57452e8

@nickolas-deboom nickolas-deboom force-pushed the update-lifecycle-events-for-eve-and-aqara-subdrivers branch from 16cc922 to cb70ab8 Compare May 9, 2025 15:50
-- after 3 seconds of cubeAction, to automatically change the action status of Plugin UI or Device Card to noAction
local CUBEACTION_TIMER = "__cubeAction_timer"
local CUBEACTION_TIME = 3

local function is_aqara_cube(opts, driver, device)
local name = string.format("%s", device.manufacturer_info.product_name)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

device.manufacturer_info does not exist for child devices. Moving this statement inside the if statement prevents an error from occurring here.

@nickolas-deboom nickolas-deboom force-pushed the matter-switch-update-field-names branch 3 times, most recently from ea07e1b to 653030b Compare May 9, 2025 18:47
@nickolas-deboom nickolas-deboom changed the base branch from matter-switch-update-field-names to main May 12, 2025 20:00
@nickolas-deboom nickolas-deboom changed the base branch from main to matter-switch-update-field-names May 12, 2025 20:00
@nickolas-deboom nickolas-deboom force-pushed the matter-switch-update-field-names branch from 653030b to ea1236c Compare May 13, 2025 15:40
Base automatically changed from matter-switch-update-field-names to main May 13, 2025 15:48
With the changes from #2041, the matter-switch subdrivers should be
updated to maintain consistency. This involves:
* Moving the initialization code from device_init into do_configure
* Implement the driverSwitched lifecycle event
* Additionally, improve the lifecycle event testing for the Aqara
  subdriver, removing the `TEST_CONFIGURE` field and instead leveraging a
  technique used in other test files to set `device.profile.id` for a mock
  device.
@nickolas-deboom nickolas-deboom force-pushed the update-lifecycle-events-for-eve-and-aqara-subdrivers branch from cb70ab8 to 43e5825 Compare May 13, 2025 16:41
The device can also emit LongPress events, and the subdriver needs to be
updated to account for this.
@nickolas-deboom
Copy link
Contributor Author

nickolas-deboom commented May 13, 2025

Hi @DongHoon-Ryu , I made a few changes to the Aqara subdriver in this PR, to be more consistent with the changes from #2041 and a few minor changes to increase consistency with other subdrivers. Could you please review when you have a chance? One thing I noticed during testing is that the cube was sending the LongPress event rather than InitialPress. Have you seen this before? I wonder if this behavior is firmware-dependent. I added a handler for the LongPress event similar to the InitialPress handler to account for this.

@nickolas-deboom nickolas-deboom force-pushed the update-lifecycle-events-for-eve-and-aqara-subdrivers branch from 9771c42 to 57452e8 Compare May 13, 2025 18:01
function()
test.socket.device_lifecycle:__queue_receive({ mock_device.id, "added" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be removing this part of the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The profile update occurs in doConfigure now which is handled in the test init function to be more consistent with other test files

@@ -236,20 +251,11 @@ test.register_coroutine_test(
test.register_coroutine_test(
"Handle single press sequence in case of exhausted endpoint",
function()
test.socket.device_lifecycle:__queue_receive({ mock_device_exhausted.id, "added" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be removing this part of the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above this was moved to the init function

local subscribe_request = CLUSTER_SUBSCRIBE_LIST[1]:subscribe(mock_device)
for i, clus in ipairs(CLUSTER_SUBSCRIBE_LIST) do
if i > 1 then
subscribe_request:merge(clus:subscribe(mock_device))
end
end
test.socket.matter:__expect_send({mock_device.id, subscribe_request})
test.socket.device_lifecycle:__queue_receive({ mock_device.id, "added" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this logic being added to the init test? Shouldn't these be implemented as separate tests if required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way each test case will run after already having gone through added, doConfigure, and infoChanged, it's done this way in other test files as well, although not all of them include every lifecycle event.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants