Aeotec Home Energy Meter Gen8 #1981
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 66 files ± 0 426 suites +6 0s ⏱️ ±0s Results for commit e368cad. ± Comparison against base commit 0db6c68. This pull request removes 2 and adds 58 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against e368cad |
|
Hey @Brianj94 - Tagging you for your awareness :) |
| @@ -35,6 +35,36 @@ zwaveManufacturer: | |||
| productType: 0x0002 | |||
| productId: 0x0001 | |||
| deviceProfileName: base-electric-meter | |||
| - id: "0x0371/0x0003/0x0033" #HEM Gen8 1 Phase EU | |||
| deviceLabel: Aeotec Home Engery Meter Gen8 Consumption | |||
There was a problem hiding this comment.
I believe "Engery" is a typo in all of these join names
| - id: "0x0371/0x0003/0x0034" # HEM Gen8 3 Phase EU | ||
| deviceLabel: Aeotec Home Engery Meter Gen8 Consumption | ||
| manufacturerId: 0x0371 | ||
| productType: 0x0003 |
There was a problem hiding this comment.
in general what we've done with other Aeotec devices is consolidated fingerprints by omitting the productType, which you could do here for the 1 and 3 phase meters
| version: 1 | ||
| categories: | ||
| - name: CurbPowerMeter | ||
| preferences: |
There was a problem hiding this comment.
if there are no preferences, this line can be omitted. same above
There was a problem hiding this comment.
nit: you change back and forth between 2- and 4-space tab widths
| local child_device_key = find_hem8_child_device_key_by_endpoint(endpoint); | ||
| local child_device = device:get_child_by_parent_assigned_key(child_device_key) | ||
|
|
||
| if child_device then |
There was a problem hiding this comment.
Rather than repeating yourself here, you could just create a variable like device_to_emit_with that defaults to device and just overwrite it if a child_device is found. Avoids this if statement with the duplicated code.
There was a problem hiding this comment.
You are absolutely right.
There was a problem hiding this comment.
you should add the common functionality between the sub-drivers to this parent driver here, like the refresh function and the meter_report_handler
There was a problem hiding this comment.
I have intentionally moved the refresh and meter_report_handler functions to the sub-drivers because they each target different endpoints, which are defined in the sub-drivers. How could I implement this efficiently in the parent driver?
There was a problem hiding this comment.
I wasn't noticing the differences in the HEM8_DEVICES map. There's still probably a way to do it, but in light of those differences I imagine it's more complicated.
|
Hello @mh-zwave Thank you for your PR. Please review the above comments and address them so that the review can proceed at your nearest opportunity. Thank you. |
- add copyright to source files - add missing tests - fix typos - consolidated fingerprints - fix line indentations - rework meter_report_handler function
|
| @@ -0,0 +1,190 @@ | |||
| -- Copyright 2022 SmartThings | |||
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests