Skip to content
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

Inovelli New Zigbee Dimmer #1435

Merged
merged 44 commits into from
May 23, 2022
Merged

Inovelli New Zigbee Dimmer #1435

merged 44 commits into from
May 23, 2022

Conversation

InovelliUSA
Copy link
Contributor

Prepping files for the release of our new zigbee dimmer.

codyhackw and others added 3 commits March 15, 2022 17:06
Creating Inovelli Quirk Files for an upcoming release
Creating the Inovelli VZM31-SN Quirk for an upcoming release
Creating Inovelli Quirk Files
@InovelliUSA InovelliUSA changed the title New Zigbee Dimmer Inovelli New Zigbee Dimmer Mar 16, 2022
codyhackw and others added 13 commits March 17, 2022 13:34
Typo identified on Line 30, correcting as required.
Updates to address some validation errors.
Updated for isort
Updated for isort
Updated w/ Black
Updated w/ Black
Removing Line 103 due to flake8 error D211
}

replacement = {
MODELS_INFO: [("Inovelli", "VZM31-SN")],
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this from replacement

ENDPOINTS: {
1: {
PROFILE_ID: zha.PROFILE_ID,
DEVICE_TYPE: DeviceType.DIMMABLE_LIGHT,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Adminiuga should we map DeviceType.DIMMER_SWITCH to the light platform so that quirks don't have to change the device type like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

PLEASE do. In development of this device I had to override in the very beginning to even get any dim levels. All dimmers need 0-100% brightness_pct attribute, etc, as provided by the light domain.

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't DeviceType.DIMMER_SWITCH a "remote" device? Device really should report itself as "Dimmable_Light".
Check Section 7.5.5 of Zigbee HA Profile, document 05-3520-29. DIMMER_SWITCH is mandated to have client on/off, level control and identity clusters.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@kreene1987 what kind of dimmer is it? two gang or single gang?

Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't DeviceType.DIMMER_SWITCH a "remote" device? Device really should report itself as "Dimmable_Light".
Check Section 7.5.5 of Zigbee HA Profile, document 05-3520-29. DIMMER_SWITCH is mandated to have client on/off, level control and identity clusters.
image

Great point. I didn’t check this before hand.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is a single gang device, then the endpoint id 1 should be of a DIMMING_LIGHT device type, and the 2nd endpoint should be of a DIMMER_SWITCH device type. Similar to how GE46857 implemented it, none of the others got it right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to how GE46857 implemented it, none of the others got it right.

Can you link me to where this quirk is stored? I couldn't find GE or Jasco as a manuf in the quirks folder.

We'll get right on it for testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not change the firmware of the device so that it reports itself as the correct device types? You can do it with a quirk but if you can change the firmware to be more accurate it may help other platforms as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I meant that the correct fix is to update the device firmware. There's no quirk for GE 45857, but you can find signature in https://github.com/home-assistant/core/blob/bc194cd209d4261bf7e15aa89eac09be5685c3db/tests/components/zha/zha_devices_list.py#L1674

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not change the firmware of the device so that it reports itself as the correct device types? You can do it with a quirk but if you can change the firmware to be more accurate it may help other platforms as well.

Thanks for the feedback all, we will discuss with the engineer.

@dmulcahey
Copy link
Collaborator

@InovelliUSA I believe you need to remove the - from the file name as well

@InovelliUSA
Copy link
Contributor Author

Thanks, we will check it out.

codyhackw and others added 9 commits March 24, 2022 18:22
Updating per recommended changes
Update parameter for firmware 9
Update for firmware v9
Update with second signature depending on firmware.
Swapping v9 class to topmost.
Updates per review recommendations
Format fix
Update deprecated command
@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2022

Codecov Report

Merging #1435 (8dd2da0) into dev (d60fa3d) will decrease coverage by 0.11%.
The diff coverage is 85.18%.

@@            Coverage Diff             @@
##              dev    #1435      +/-   ##
==========================================
- Coverage   80.47%   80.35%   -0.12%     
==========================================
  Files         225      229       +4     
  Lines        6505     6827     +322     
==========================================
+ Hits         5235     5486     +251     
- Misses       1270     1341      +71     
Impacted Files Coverage Δ
zhaquirks/inovelli/__init__.py 75.00% <75.00%> (ø)
zhaquirks/inovelli/VZM31SN.py 100.00% <100.00%> (ø)
zhaquirks/tuya/ts004f.py 80.64% <0.00%> (-19.36%) ⬇️
zhaquirks/tuya/ts0601_dimmer.py 93.75% <0.00%> (-6.25%) ⬇️
zhaquirks/tuya/ts0601_siren.py 93.96% <0.00%> (-6.04%) ⬇️
zhaquirks/__init__.py 77.89% <0.00%> (-1.48%) ⬇️
zhaquirks/tuya/mcu/__init__.py 98.68% <0.00%> (-1.32%) ⬇️
zhaquirks/xiaomi/aqara/plug_mmeu01.py 78.12% <0.00%> (-0.83%) ⬇️
zhaquirks/xiaomi/mija/smoke.py 89.47% <0.00%> (-0.53%) ⬇️
zhaquirks/xbee/__init__.py 41.46% <0.00%> (-0.35%) ⬇️
... and 55 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d60fa3d...8dd2da0. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 4, 2022

Pull Request Test Coverage Report for Build 2258930270

  • 46 of 54 (85.19%) changed or added relevant lines in 2 files are covered.
  • 597 unchanged lines in 32 files lost coverage.
  • Overall coverage decreased (-0.1%) to 80.357%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zhaquirks/inovelli/init.py 24 32 75.0%
Files with Coverage Reduction New Missed Lines %
zhaquirks/tuya/ts0211.py 1 84.21%
zhaquirks/tuya/mcu/init.py 2 98.04%
zhaquirks/tuya/ts0601_dimmer.py 2 93.75%
zhaquirks/xiaomi/aqara/ctrl_ln.py 2 88.89%
zhaquirks/xiaomi/mija/smoke.py 2 89.47%
zhaquirks/aurora/aurora_dimmer.py 3 82.61%
zhaquirks/ikea/init.py 3 70.0%
zhaquirks/tuya/ts0601_electric_heating.py 3 57.58%
zhaquirks/danfoss/thermostat.py 4 82.76%
zhaquirks/tuya/ts130f.py 4 83.02%
Totals Coverage Status
Change from base Build 1976830509: -0.1%
Covered Lines: 5486
Relevant Lines: 6827

💛 - Coveralls

codyhackw and others added 2 commits April 14, 2022 09:55
Additional Parameter added
Copy link
Contributor

@puddly puddly left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

We've recently restructured our ZCL command and attribute definitions to hold more information. Could you make some changes to ensure this PR can be merged?

zhaquirks/inovelli/__init__.py Outdated Show resolved Hide resolved
zhaquirks/inovelli/__init__.py Outdated Show resolved Hide resolved
zhaquirks/inovelli/__init__.py Outdated Show resolved Hide resolved
zhaquirks/inovelli/__init__.py Outdated Show resolved Hide resolved
zhaquirks/inovelli/__init__.py Outdated Show resolved Hide resolved
codyhackw and others added 3 commits April 25, 2022 09:04
Updated for v10 firmware changes
Syntax updates and v10 firmware changes
@codyhackw
Copy link
Contributor

Thanks for the PR!

We've recently restructured our ZCL command and attribute definitions to hold more information. Could you make some changes to ensure this PR can be merged?

@puddly I believe the most recent update should have the requested changes covered if you're able to confirm?

codyhackw and others added 2 commits April 28, 2022 21:55
Parameter size change with v10 firmware
Update parameter size w/ v10 firmware
@dmulcahey dmulcahey merged commit fb5b38e into zigpy:dev May 23, 2022
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.

8 participants