Matter Switch: Refactor Driver File Organization #2401
Conversation
hcarter-775
requested review from
greens,
tpmanley,
cjswedes,
ctowns,
HunsupJung and
nickolas-deboom
|
Invitation URL: |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 03d5f69 |
Test Results 71 files 452 suites 0s ⏱️ Results for commit 03d5f69. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
missing copyright statement
Yes, there is a non-zero percentage. We are still watching these numbers to see when is appropriate to remove these altogether, but for now have elected to keep them in the driver. |
| clusters.ElectricalEnergyMeasurement.attributes.PeriodicEnergyImported | ||
| } | ||
| } | ||
| local attribute_handlers = require "generic-handlers.attribute_handlers" |
There was a problem hiding this comment.
nit: IMO we should have our sub folders using snake_case, but its sort of the wild west; the lua libs follow this, but we dont for the driver and subdriver directory names ¯\(ツ)/¯
There was a problem hiding this comment.
100% agree, just didn't want to rock the boat too hard
hcarter-775
force-pushed
the
refactor-switch-driver
branch
from
52bfc0d to
eaba018
Compare
There was a problem hiding this comment.
This looks good to me - could you provide a little more commentary on the testing you did in the Summary of Completed Testing section. Did your spot testing include testing all capabilities, driver restart, etc.? Also, do we have coverage of all device types (have we tested an energy device)? If not, I would call out if any device types are covered by unit tests only. But, the refactor makes sense to me and I assume the unit tests would fail if there was any obvious problem, so overall I think this looks good to me.
Great job with the refactor, this looks very well organized!
|
|
||
| -- Include driver-side definitions when lua libs api version is < 11 | ||
| if version.api < 11 then | ||
| clusters.ValveConfigurationAndControl = require "embedded_clusters.ValveConfigurationAndControl" |
There was a problem hiding this comment.
This require statement and others for embedded clusters fail for some reason, saying Received unexpected error: ./embedded_clusters/ValveConfigurationAndControl/init.lua:2: module 'ValveConfigurationAndControl.server.attributes' not found
There was a problem hiding this comment.
Ah, the embedded cluster definitions just need to be updated to reflect the new path, i.e.
local ValveConfigurationAndControlServerAttributes = require "ValveConfigurationAndControl.server.attributes"
should be
local ValveConfigurationAndControlServerAttributes = require "embedded_clusters.ValveConfigurationAndControl.server.attributes" here
Description of Change
This PR aims to re-organize the Matter Switch driver to be easier to read and more self-documenting in its nature.
It does not change any current handling at all, and only re-organizes where the variables and functions are defined, that is, moving them under different files and meta-tables.
The general directory structure is as follows, within
src/:embedded-clustersgeneric-handlerssub-driversutilsSummary of Completed Tests
Spot tested on the following VDA device types: OnOff Switch, Dimmer Switch, Color Temp Light, Extended Color Light, Mounted OnOff Control, Mounted Dimmable Load Control.