-
Notifications
You must be signed in to change notification settings - Fork 809
Transfer all Sinopé devices to quirks V2 #4042
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
base: dev
Are you sure you want to change the base?
Conversation
Look like there is a bug with some quirks: |
Quirks v2 no longer match by the whole ep/cluster signature. They only match by manufacturer and model. If you really need to distinguish two identically named devices depending on their cluster signature, I believe you should be able to do it something like this: sinope_device_1_signature = {
MODELS_INFO: [(SINOPE, "RM3250ZB")],
ENDPOINTS: {
1: {
PROFILE_ID: zha_p.PROFILE_ID,
DEVICE_TYPE: zha_p.DeviceType.ON_OFF_OUTPUT,
INPUT_CLUSTERS: [
Basic.cluster_id,
DeviceTemperature.cluster_id,
Identify.cluster_id,
Groups.cluster_id,
Scenes.cluster_id,
OnOff.cluster_id,
Metering.cluster_id,
ElectricalMeasurement.cluster_id,
Diagnostic.cluster_id,
SINOPE_MANUFACTURER_CLUSTER_ID,
],
OUTPUT_CLUSTERS: [
Identify.cluster_id,
Groups.cluster_id,
Ota.cluster_id,
],
}
},
}
# and when calling the v2 QuirkBuilder methods, add:
.filter(signature_matches(sinope_device_1_signature))
# ... That should additionally match by signature, though you ideally don't want to do this. If this is about preventing an entity to be created for one firmware version, there's no good way to do that at the moment. We do have v2 quirks that also expose the entity on older firmware versions though, even if it's not supported. |
So, to answer that: No, if the model name of the device is "RM3250ZB", then the "RM3250ZB-V2" quirk wouldn't match it. Only the first one with "RM3250ZB" would match it. |
If I keep the two as QuirkBuilder(SINOPE, "RM3250ZB") it load correctly and I can see both device in ZHA. But it is shared-ci / Run tests Python 3.12 that fail with AssertionError |
No, it's the same manufacturer and model. Only one of the quirks will apply. Keep both as v2 quirks only match by manufacturer and model and you're adding two quirks with the same manufacturer and model. v1 quirks had the |
The great thing about v2 quirks is that we don't need multiple quirk variants if firmware updates or other variants changed the device signature ever so slightly. For v1 quirks, you'd have to add another quirk, with a new |
Both device expose there signature for cluster but one device have more cluster than the old one:
But they share the same QuirkBuilder(SINOPE, "RM3250ZB") and same device_version=0. Can I put only the device quirks that have all cluster like the second one above. Does ZHA will load the first one above if one cluster is missing, 0x0002 |
.adds(Basic, endpoint_id=1) | ||
.adds(Identify, endpoint_id=1) | ||
.adds(Groups, endpoint_id=1) | ||
.adds(Scenes, endpoint_id=1) | ||
.adds(OnOff, endpoint_id=1) | ||
.adds(Metering, endpoint_id=1) | ||
.adds(Diagnostic, endpoint_id=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't be doing this in v2 quirks. Only add clusters if they're missing from the device signature by default.
This won't do anything, since they're most likely all already exposed by the device itself.
Like, all of the .adds(...)
should be completely removed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm doing this only in the light.py because I had to add line
.replaces_endpoint(1, device_type=zha_p.DeviceType.ON_OFF_LIGHT)
and this was removing all cluster that I didn't .replace
.replaces(CustomDeviceTemperatureCluster)
.replaces(LightManufacturerCluster)
Adding those lines fix the problem.
In Sinopé light DevicesType are reported wrong so I need to do that .replace to change the DeviceType. This work ok in V2.
To re-iterate on the differences vs v1 quirks and v2 quirks: For v2 quirks, it works nothing like that. They match by manufacturer and model only. Now, if I want to replace a cluster only, I just do this: (
QuirkBuilder("IKEA", "TRADFRI control outlet")
.replaces(MyCustomCluster, endpoint_id=1) # the endpoint_id=1 is optional for ep 1, as 1 is the default
.add_to_registry()
) All other clusters on the device are left untouched. For example, all devices have to expose a The example IKEA control outlet above also has an You can kind of consider v2 quirks as the difference of v1 quirks when comparing the But if you want to replace/remove/add a cluster in a v1 quirk, you change that in the Is that a better explanation? |
So I will keep only one copy of QuirkBuilder(SINOPE, "RM3250ZB") and make sure that it contain all .number, .enum, .sensor that are present in the other QuirkBuilder(SINOPE, "RM3250ZB"). |
There is an import error in test_sinope.py. Is it you who fix this ? Also there is a AssertionError: Translation key 'valve_countdown' is shared by quirks with different fallback names: |
Also where can I add missing french translation. |
The Codecov project check doesn't really matter. All lines of the v2 quirks are covered, so that's good. |
Thank you. check it and let me know if we can merge to V2 |
I've found my last bug for dimmer, DM2500ZB. Now all entities are up and running |
@TheJulianJES , I've few questions:
|
Ok I've found how to make an enum out of t.uint16_t
Now can you tell me how to limit a value. For example I receive the value for battery_percentage_remaining which goes to 200% with new battery. How can I set the value red from the attribute not to go above 100% |
Not possible via quirks v2 entities, yet. There's a request for it here: zigpy/zha#364
You can omit the unit and no unit will be shown in HA.
An .prevent_default_entity_creation(endpoint_id=1, cluster_id=IasZone.cluster_id) # disable ZHA entity
.binary_sensor(
IasZone.AttributeDefs.zone_status.name, # though you may want to use your own attribute definition here
IasZone.cluster_id,
unique_id_suffix="ias_1", # to not conflict with unique ID of other entity, as only the attr name is used by default
translation_key="leak_status",
fallback_name="Leak status",
attribute_converter=lambda x: x & 0b00000001, # use only bit 1
)
.binary_sensor(
IasZone.AttributeDefs.zone_status.name, # though you may want to use your own attribute definition here
IasZone.cluster_id,
unique_id_suffix="ias_2", # to not conflict with unique ID of other entity, as only the attr name is used by default
translation_key="tamper_status",
fallback_name="Tamper status",
`, # use only bit 2
)
.add_to_registry() If you have multiple quirks v2 entities for the same attribute, as might be possible for The same might also apply if you have the same quirks v2 entity for multiple endpoint ids, but I can't fully remember right now. Above, I've also shown how you can use
Eh, I don't think you should/want to do that generally. Like, the above way shows how to make separate binary sensors for that (might need to keep the type as an int?).
ZHA already creates a default entity for ZCL specifies that it's Otherwise, |
I've not had a proper look at everything changed in this PR, but if you have multiple quirks that use duplicated entities, you might be able to add a base quirk without any model info, and then clone that base quirk multiple times with added model info and more custom entities. See this example, but try to ignore all the Tuya stuff: zha-device-handlers/zhaquirks/tuya/tuya_valve.py Lines 226 to 351 in 5556090
Doing it this way isn't always helpful, but it might be for this PR (if you have very similar, but slightly differing devices). |
It there a way to express data like battery voltage as integer not float. 100 instead of 100.0 |
You can use |
The sensor for battery_percentage_remaining is not created by my quirk but come from ZHA. |
Using suggested_display_precision=0 give me the following error: |
I've finished to review all sensor, number etc to avoid duplicate with ZHA quirks V2. I also moved all common sensor etc to sinope_base_quirk for thermostat.py. let me know how to fix suggested_display_precision error. |
I've a question. For tank level sensor LM4110-ZB. if the probe is disconneted the sensor gauge_angle will report a value of -2. It there a way we can catch this error in the sensor.py code ? |
We only added this for HA 2025.5.0 (and later). The minimum zigpy version required by zha-quirks doesn't have this additional parameter yet, so the tests in this repo will fail until I bump that version (and this PR being updated/rebased on top of that then). However, it should work locally..
We might be able to add an attribute_converter=lambda x: None if x == -2 else x That would just return |
.sensor( # battery percent | ||
attribute_name=SinopeTechnologiesPowerConfigurationCluster.AttributeDefs.battery_percentage_remaining.name, | ||
cluster_id=SinopeTechnologiesPowerConfigurationCluster.cluster_id, | ||
state_class=SensorStateClass.MEASUREMENT, | ||
unit=PERCENTAGE, | ||
device_class=SensorDeviceClass.BATTERY, | ||
reporting_config=ReportingConfig( | ||
min_interval=30, max_interval=43200, reportable_change=1 | ||
), | ||
translation_key="battery_percentage_remaining", | ||
fallback_name="Battery percentage remaining", | ||
entity_type=EntityType.DIAGNOSTIC, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? If you remove this, does the built-in ZHA battery percentage sensor not appear?
.sensor( # Current summ delivered | ||
attribute_name=SinopeTechnologiesMeteringCluster.AttributeDefs.current_summ_delivered.name, | ||
cluster_id=SinopeTechnologiesMeteringCluster.cluster_id, | ||
state_class=SensorStateClass.TOTAL_INCREASING, | ||
unit=UnitOfEnergy.WATT_HOUR, | ||
device_class=SensorDeviceClass.ENERGY, | ||
reporting_config=ReportingConfig( | ||
min_interval=59, max_interval=1799, reportable_change=60 | ||
), | ||
translation_key="current_summ_delivered", | ||
fallback_name="Current summ delivered", | ||
entity_type=EntityType.STANDARD, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this. ZHA should already create an entity for this Metering
cluster, so we shouldn't add a custom entity.
Does the ZHA entity not appear for some reason?
Ok, and that works as expected now (showing "unavailable" if the probe is not connected)?
Hmm, we have to get this fixed. Were you able to try this on HA 2025.6.0 betas? They already appear very stable to me. If so, does the behavior change on those versions? Also, do the default ZHA entities work if you use the (integrated) v1 quirks, or no quirks for the Sinope devices at all? (you'd have to remove the integrated quirks to test "no quirks" at all) |
if I live it as:
I get an error:
|
Yeah, because However, it doesn't make a ton of sense to define an enum for |
For the missing default ZHA entities, there should be some more info when enabling debug logs in logger:
default: info
logs:
homeassistant.core: debug
homeassistant.components.zha: debug
bellows.zigbee.application: debug
bellows.ezsp: debug
zha: debug
zigpy: debug
zigpy_deconz.zigbee.application: debug
zigpy_deconz.api: debug
zigpy_xbee.zigbee.application: debug
zigpy_xbee.api: debug
zigpy_zigate: debug
zigpy_znp: debug
zhaquirks: debug ( Please try to grab debug logs, without your custom quirks IIRC the The Also, are you on HA 2025.6.0b4 now and the issues still continue? And one last thing, please try to remove(!) and re-pair the device once (without your custom quirks sensors being present, but with the v2 quirks, and maybe also without after that). Removing the device makes sure the zigpy attribute cache in the database is cleaned for that device, so it's properly re-interviewed. It will also remove all your HA entities though. Please also grab diagnostics information after that, so we can possibly compare them. |
Proposed change
Transfer all Sinopé devices to quirks V2 and add support for some new devices.
Add many new attributes in manufacturer cluster.
Additional information
Some manufacturer cluster attributes are already supported in ZHA but attributes values are not the same. ex. attribute 0x0002 for keypad_lockout in manufacturer cluster have three values. Unlocked = 0x00, Locked = 0x01, Partial_lock = 0x02. In ZHA there is 5 values Unlock, lock1, lock2, lock3 and lock4 which have no meaning for Sinopé devices.
Same for on_led_color which have only the color used in Neviweb. We should be able to add mode color.
They should be included in this quirks as they are only available for Sinopé devices.
This PR replace PR #3821.
Checklist
pre-commit
checks pass / the code has been formatted using Black