SmartThingsCommunity / SmartThingsEdgeDrivers

Apache License 2.0
269 stars 463 forks source link

Update matter-switch for Aqara Cube #1693

Closed DongHoon-Ryu closed 1 month ago

DongHoon-Ryu commented 1 month ago

REQ-15926, REQ-16285, IOTE-4217, IOTE-4266

Check all that apply

Type of Change

Checklist

Description of Change

Summary of Completed Tests

github-actions[bot] commented 1 month ago

Channel deleted.

github-actions[bot] commented 1 month ago

Test Results

   64 files    396 suites   0s :stopwatch: 1 938 tests 1 938 :white_check_mark: 0 :zzz: 0 :x: 3 362 runs  3 362 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 71c9dac6.

:recycle: This comment has been updated with latest results.

github-actions[bot] commented 1 month ago

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

Minimum allowed coverage is 90%

Generated by :monkey: cobertura-action against 71c9dac6eeee9095a6b8d4a4acca6c9091316486

hcarter-775 commented 1 month ago

I still think the change I suggested in this comment on the last PR would be a good addition, to add to the clarity of this subdriver.

hcarter-775 commented 1 month ago

I suggest pulling the logic in device_added out of device_added, and moving it directly into init. Currently, this logic will be called twice now when the device is first created (aka on added and then init, which may cause multiple try_update_metadata calls, something we don't want). Further, I think some gating should be added to keep the try_update_metadata from being called multiple times like this anyway.

ctowns commented 1 month ago

For reference @DongHoon-Ryu , the driver lifecycle events occur at different times for different situations. However, the simple explanation is this:

  1. driver init - this will occur every time the driver restarts. This means that any set up/initialization that needs to occur for the device each time the driver restarts needs to occur here. This is commonly where we set up our subscribe requests.
  2. added/doConfigure - these events only happen once in the lifetime of the device (there are exceptions for doConfigure but they are not relevant in this case). These lifecycle handlers will only run once when a device is onboarded, and then they will likely not be run again during the devices lifetime. These lifecycle events are usually used for handling one-time configurations, such as profile-switching when onboarding a device, or creating component-to-endpoint maps.

In this case, it seems like there is some logic in the device_added handler that should be in the device_init function. Any code that needs to run each time the driver is initialized needs to be in the device_init function. The driver is initialized on each restart, and the driver restarts each time we deploy an update to the driver. In the future, we should make sure that we test the driver restart/driver switch test case so we can catch issues like this.

You can see our developer documentation on these events here: https://developer.smartthings.com/docs/edge-device-drivers/driver.html#lifecycle-event-handlers

DongHoon-Ryu commented 1 month ago

@hcarter-775 I'm sorry. I missed the proposal regarding TEST_CONFIGURE last time. I think it's a good idea. First of all, I improved it with the method you suggested, and if I try_update_metadata at the device_init stage, test case crash occurs in the test framework, so I used TEST_CONFIGURE as a way to avoid it.