zigpy / zha-device-handlers

ZHA device handlers bridge the functionality gap created when manufacturers deviate from the ZCL specification, handling deviations and exceptions by parsing custom messages to and from Zigbee devices.
Apache License 2.0
714 stars 661 forks source link

[Technical Support] General discussion regarding Tuya TRVs - for people implementing the quirks #1183

Closed PLTorrent closed 2 years ago

PLTorrent commented 2 years ago

Hi,

As @MattWestb and @jacekk015 suggested I open a technical topic for all the tech questions regarding the implementation of the quirks for Tuya TRV.

And to start off here is a first one: Changing the range of available temperature settings. For current Saswell quirk (https://github.com/zigpy/zha-device-handlers/blob/dev/zhaquirks/tuya/ts0601_trv_sas.py) the temp range is set to 7-30 (default) however the device uses 5 -30. The setting used in quirk is not being applied.

The way I managed to change this is:

class SaswellThermostatCluster(TuyaThermostatCluster):
    """Thermostat cluster for Tuya thermostats."""

    #    cluster_id = Thermostat.cluster_id

    def __init__(self, *args, **kwargs):
        """Init."""
        super().__init__(*args, **kwargs)
        self._update_attribute(self.attridx["min_heat_setpoint_limit"], 500)
        self._update_attribute(self.attridx["max_heat_setpoint_limit"], 3000)

this is not working:

 _CONSTANT_ATTRIBUTES = {
        MIN_HEAT_SETPOINT_ATTR: 500,
        MAX_HEAT_SETPOINT_ATTR: 3000,
        CTRL_SEQ_OF_OPER_ATTR: Thermostat.ControlSequenceOfOperation.Heating_Only,  # the device supports heating mode
    }

Do you know of any other/better way of doing this?

PS. Check if this is working correctly for Moes/Beca TRV as those seem to read those values from TRV but, are those values implemented in HA?

jacekk015 commented 2 years ago
class SaswellThermostatCluster(TuyaThermostatCluster):
    def __init__(self, *args, **kwargs):
        """Init."""
        super().__init__(*args, **kwargs)
        self.endpoint.device.thermostat_bus.add_listener(self)
        self.endpoint.device.thermostat_bus.listener_event(
            "temperature_change",
            "min_heat_setpoint_limit",
            500,
        )
        self.endpoint.device.thermostat_bus.listener_event(
            "temperature_change",
            "max_heat_setpoint_limit",
            3000,
        )

https://github.com/zigpy/zha-device-handlers/blob/dev/zhaquirks/tuya/__init__.py#L559

Your way does the same, but in shorter path.

MattWestb commented 2 years ago

PS: this is setting the ZHA cards min and max setpoint temperature and not in the device (if the device is supporting it perhaps),

But setting it OK is making the device is not getting out of range data and getting problem and the GUI is looking nicer.

PLTorrent commented 2 years ago

@jacekk015 Basically you code does exactly the same, only around ;]

@MattWestb yes, exactly. Saswell device does not have any override of the default min/max setpoints.

MattWestb commented 2 years ago

The same with my Siterwell i think only "Moes" types have it in the TRV as attributes.

PLTorrent commented 2 years ago

@jacekk015 @MattWestb I just sniffed the scheduling with ZBOSS for the Saswell but I wonder if there is any point in implementing it since ZHA/HA does not seem to have any usable interface for it, does it?

How the scheduling is implemented UI-wise for Moes/Beca?

jacekk015 commented 2 years ago

Beca received Schedule on the attributes level. You can see that post.

PLTorrent commented 2 years ago

You mean from "Manage Clusters"?

Searched the Beca thread for some screenshots but couldnt find any from the scheduler

jacekk015 commented 2 years ago

yes https://github.com/zigpy/zha-device-handlers/issues/1123#issuecomment-979998074

PLTorrent commented 2 years ago

How do I enable Preset dropdown in thermostat like this?

image

I have added this to my quirk:

class SaswellThermostatCluster(TuyaThermostatCluster):
    """Thermostat cluster for Tuya thermostats."""

    def __init__(self, *args, **kwargs):
        """Init."""
        super().__init__(*args, **kwargs)
        self._update_attribute(self.attridx["min_heat_setpoint_limit"], 500)
        self._update_attribute(self.attridx["max_heat_setpoint_limit"], 3000)

    class Preset(t.enum8):
        """Working modes of the thermostat."""

        Away = 0x00
        Schedule = 0x01
        Manual = 0x02

Also I have implemented all the mapping and events but still no Preset dropdown. What am I missing?

image
MattWestb commented 2 years ago

Moes is doing it in ZHA https://github.com/home-assistant/core/blob/296b73874054eec31d91f0db1b9a625ad5d47c32/homeassistant/components/zha/climate.py#L609-L622

MaxSmart ans Seiter is having doing the same but its not merged in ZHA.

PLTorrent commented 2 years ago

This is crazy system design.. you have access to zigpy classes only but ZHA creates entities... and they wonder why people prefer Z2M over native ZHA...

MattWestb commented 2 years ago

I think the best way is Zigpy is only having one quirk that is tunneling all DP commands to ZHA or one other integration and all magic is being done there for not having being squeezed true Zigbee commands and having access to all HA good things. "tuya Zigbee Extension" ?

MattWestb commented 2 years ago

One with presets for siterwell TRVs. climateSite.zip You can putting in your device IDs and it shall working for your devices.

PLTorrent commented 2 years ago

@MattWestb this way there always will be problems. I think that it should be possible to control what entities with what names and properties are created by ZHA in HA from the quirk level. This would be universal.

MattWestb commented 2 years ago

I agree with you but as long the maintainer dont agree its nt possible getting it implanted. The Zigbee standard is open for adding extensions with adding function with manufacture code to commands and attributes but they is saying no.

I was suggesting adding "extra attributes" with ZHA-quirk as manufacture for adding the extra function but no luck.

dmulcahey commented 2 years ago

Quirks are for translating Zigbee clusters that are not spec compliant to compliant clusters. Think of them like translators. They have nothing to do with HA itself and they can technically benefit any software that leverages Zigpy. There are other automation systems that use Zigpy already. The only deviation from this is device action triggers and they may be moved in the future.

MattWestb commented 2 years ago

David is it one wrong thinking making one quirk that is tunneling all tuya DP commands and is doing the device handling one level over Zigpy (perhaps ZHA or some other extension) then its very complex devices (not all but most).

Can you also making one comment how the best way getting all devices that is having getting new or updated support in updating request but cant being merged (it was 14 devices but 3 more is added last days) ?

All tuya TRVs is being looked on only the original Moes is not completely rewritten but is also in the pipe if all is going well.

The original quirk for my siterwell TRV is loosing half of its function and some of the implanted is broken. All is fixed in the updated quirk plus little extra function is putted in with good coding in the quirk.

If we is not making somthing we can deleting all tuya TRVs then its so bad supported in ZHA and i thinking it very bad for all in the end.

Thanks in advance !!!

jacekk015 commented 2 years ago

All tuya TRVs is being looked on only the original Moes is not completely rewritten but is also in the pipe if all is going well.

That's actually wrong since 20 minutes or so: https://github.com/zigpy/zha-device-handlers/issues/1178#issuecomment-980294492 Rewritten Moes is in above comment. Next is Maxsmart cosmetics + schedule. Next in queue is: TS0601 _TZE200_a4bpgplm tuya trv - this weekend for sure. https://github.com/zigpy/zha-device-handlers/issues/1159#

MattWestb commented 2 years ago

New device is first priory but Siterwell and Saswell TRVs is very urgent getting working OK then they is having basic functionality (setting and reporting temperature) and with luck little more and users is being very disappointed if cant using all good stuff in there devices.

The good thing is deCONZ is not merging any new devices for the summer (only some extra ID is added the last days but no new devices) so they is having larger problems.

Also good thing getting all tuya TRVs using the same coding style an being easier making changes and adding functions.

Also shall all tuya TRVs being in one large (MEGA) quirk or being spitted in families ?

In the end its the maintainer must saying with road shall being used for getting all devices good integrated in ZHA.

Have Alexei some thinking ?

jacekk015 commented 2 years ago

Siterwell you have in your hands, Matt. Saswell I don't touch because @PLTorrent is coding it. I can help if asked. Since some time I've wrote Maxsmart, Beca, Siterwell and extended Moes. As a programmer perspective, before I've wrote one line of code, one week took me studying the connections between all that Python spaghetti. It's doable but it's not easy if you don't dive in deep into the code. I don't see any possibility that some medium level programmer jumps in and write 2-3 devices. Now you can wake me up in the night and ask about that code for TRV's - I will tell you everything.

Mostly I use Python for converters like Excel->Excel or CSV->SQL Server I don't use 10 levels of classes there.

Every programmer should tell that one MEGA file gives MEGA hard approach to maintain.

MattWestb commented 2 years ago

Is it possible making one handle in the quirk that can being used for selecting version of presets in ZHA, like variable tuya_preset3 is giving 3 presets i ZHA and tuya_preset6 is giving 6 presets without adding all devices in ZHA and having all device unique thing in the quirk ?

If getting updating of all "old" TRVs i think its better splitting them but then need writing tests for all. Today its the normal Moes and Siterwell that have test code implanted and its being broken if changing the code.

MattWestb commented 2 years ago

If you is deep diving in the Zigbee documents you is getting the same problems then its many hundred of devs and engineers that have putting all parts together and and in the end all manufactures is doing there variant of it but then tuya is doing all there way outside the standard.

For MAxSmarts TRVs, are you planing implanting: "Calibrating local temperature ⭕" in the update ?

I have flagging: "Setting / reporting Scheduled times and setpoint temperatures ⬜ (Upcoming future on cluster and local)".

If all TRVs that supporting Scheduled in the TRV and if the data format is the same i think its good using the same attribute for all Scheduled functions so its possible using the same Scheduled "application" in ZHA in the future.

jacekk015 commented 2 years ago

Now I would feel stupid if I would not add those functions to the TRVs I own myself ;-)

Moes, Beca, Maxsmart. None of them have same approach to Scheduling.

MattWestb commented 2 years ago

Its still tuya devices and need earning there name !!

That was bad for us :-((

PLTorrent commented 2 years ago

@PLTorrent is coding it.

I pretty much have it ready bar the scheduling since there is no viable way to present it in HA. Also lack of Presets for this TRV is blocking me.

@jacekk015 your feedback/guidance was most helpful and impressive.

As for the scheduling on Sas it is done via one endpoint and weekdays are bitmasked so it is possible to send settings for any number of days in case the schedule for those days is to be the same. Then the device confirms with a separate (different) endpoint for each day. ;)

And finally all the switches have the same on_off name so unaware user will have no way of knowing which switch is changing what so without entity renaming/suffixing in ZHA it is just a mess

PLTorrent commented 2 years ago

https://github.com/home-assistant/core/issues/60450

MattWestb commented 2 years ago

System mode auto is if you have one climatic that can heating and cooling. Then all our TRV only can heating its not fitting in the auto mode type. Some Thermostat is having both heat and cooling setpoints and can being used in 3 system mode Heat, Cooling and Auto.

PLTorrent commented 2 years ago

@MattWestb thanks for explanation ;]

PLTorrent commented 2 years ago

@MattWestb @jacekk015

Saswell SEA802 quirk: Saswell_SEA802.zip

Currently only supports SEA802. "_TZE200_yw7cahqs", "_TZE200_c88teujp", "_TZE200_azqp6ssj", "_TZE200_9gvruqf5", "_TZE200_zuhszj9s",

Additionaly changed climate.py with added Saswell: climate.zip

Want to test it a bit more, but for future reference should I open a new issue with this or put it into previous closed issues regarding this TRV?

Also those I have no way of testing and they have different endpoint config from _TZE200

("_TYST11_KGbxAXL2", "GbxAXL2"), ("_TYST11_c88teujp", "88teujp"), ("_TYST11_azqp6ssj", "zqp6ssj"), ("_TYST11_yw7cahqs", "w7cahqs"), ("_TYST11_9gvruqf5", "gvruqf5"), ("_TYST11_zuhszj9s", "uhszj9s"),

TBD:

  1. Temp correction
  2. Battery monitoring
  3. Scheduling if viable
MattWestb commented 2 years ago

Sas have released devices first with TYST11 Zigbee modules (that have hidden tuya cluster) and then little later the same device with TZE200 Zigbee module with one exception is GbxAXL2 but all looks having the same tuya MCU. My feeling is that its only putting the same "new extra cluster" for the TYST11 versions and it shall working OK.

The local temperature calibration (if its implanted) is used for getting the device reporting local temperature more often by sending updated of it to the device from one automation.

The matrix with one new found that not being added:

Saswell | SEA801 | _TYST11_KGbxAXL2 | GbxAXL2 | ✅ Sas II | 798 Saswell | SEA802 | _TZE200_c88teujp | TS0601 | ✅ Sas I | 576 Saswell | SEA802 | _TYST11_c88teujp | 88teujp | ✅ Sas II | 576 HiHome | WZB-TRVL | _TZE200_zuhszj9s | TS0601 | ✅ Sas I | 1064 HiHome | WZB-TRVL | _TYST11_zuhszj9s | uhszj9s | ✅ Sas II | 798 Hama | 00176592 | _TZE200_yw7cahqs | TS0601 | ✅ Sas I | 923 Hama | 00176592 | _TYST11_yw7cahqs | w7cahqs | ✅ Sas II | 798 RTX | ZB-RT1 | _TZE200_azqp6ssj | TS0601 | ✅ Sas I | 923 RTX | ZB-RT1 | _TYST11_azqp6ssj | zqp6ssj | ✅ Sas II | 1064 Sanico | - | _TZE200_9gvruqf5 | TS0601 | ✅ Sas I | 1064 Sanico | - | _TYST11_9gvruqf5 | gvruqf5 | ✅ Sas II | 1064 Saswell | SEA802 | TZE200_zr9c0day | TS0601 | ⭕ Sas I

Its some earlier issues and also PR that have being closed so i think its better doing one new issue and trying getting user testing the new quirk for there TRVs.

Also adding preset to ZHA is one braking thing if not the quirk is not in place in Zigpy or the old quirk is left there also if the TYST11 is not moved to the new quirk.

PLTorrent commented 2 years ago

https://github.com/zigpy/zha-device-handlers/issues/1189

MattWestb commented 2 years ago

The code looks very nice and clean !!!! Also good comments if the function of the device DPs :-))

Hope users can getting all extras working without getting crazy if the not named endpoints / cluster but you have doing one good guide :-)) (I was trying explaining for one Site user but i think shi have giving up getting all in)

Can you explaining the SASWELL_LIMESCALE_PROTECT_ATTR is it anti calking function ?

If you like getting little more battery information you can using this from tuya INIT: https://github.com/zigpy/zha-device-handlers/blob/014175e5d1d720e2d662880b0a3b3903fdbc892d/zhaquirks/tuya/__init__.py#L735-L746

Great work done !!

I trying putting i the tuya TRV matrix.

PLTorrent commented 2 years ago

The code looks very nice and clean !!!! Also good comments if the function of the device DPs :-))

Hope users can getting all extras working without getting crazy if the not named endpoints / cluster but you have doing one good guide :-)) (I was trying explaining for one Site user but i think shi have giving up getting all in)

Thank you!

Can you explaining the SASWELL_LIMESCALE_PROTECT_ATTR is it anti calking function ?

Yes

If you like getting little more battery information you can using this from tuya INIT:

https://github.com/zigpy/zha-device-handlers/blob/014175e5d1d720e2d662880b0a3b3903fdbc892d/zhaquirks/tuya/__init__.py#L735-L746

This device only reports battery low via uint8 on/off field, no reporting of battery voltage, etc. so IMHO no point in adding that.

jacekk015 commented 2 years ago

@PLTorrent Nicely coded, nice code re-usage. I feel a bit lazy looking now ;-) I'm tuning my Maxsmart with 126 schedule attributes. TRV -> HA already working. Need to create reverse communication and it should be the end.

MattWestb commented 2 years ago

@PLTorrent Is the Sas haveReporting valve position % ⬜ implanted in the firmware ? I think its coming from the first gen Moes like the Siter was having and was never working then function is not in the firmware.

jacekk015 commented 2 years ago

BTW In case you wouldn't be happy with lazy room temp monitoring, and your TRV responds with local temp after calibration or local temp command, you could create a task and query TRV for local temp occasionally.

I did it for Maxsmart, because sometimes it reports often, and sometimes after hours. In SaswellThermostat class, in def init

    def __init__(self, *args, **kwargs):
        """Init."""
        super().__init__(*args, **kwargs)
        self.endpoint.device.thermostat_bus.add_listener(self)
        self.endpoint.device.thermostat_bus.listener_event(
            "temperature_change",
            "min_heat_setpoint_limit",
            MAXSMART_MIN_TEMPERATURE_VAL,
        )
        self.endpoint.device.thermostat_bus.listener_event(
            "temperature_change",
            "max_heat_setpoint_limit",
            MAXSMART_MAX_TEMPERATURE_VAL,
        )
        task = asyncio.create_task(self.update_local_temp_task(self))

and somewhere below

    async def update_local_temp_task(self, arg):
        """Python task to update local temp more frequently than 60 minutes."""
        while True:
            await asyncio.sleep(300)  # 300 seconds = 5 minutes
            await MaxsmartManufClusterSelf[
                self.endpoint.device.ieee
            ].endpoint.tuya_manufacturer.write_attributes(
                {MAXSMART_TEMPERATURE_ATTR: 0}, manufacturer=None
            )
PLTorrent commented 2 years ago

@PLTorrent Nicely coded, nice code re-usage. I feel a bit lazy looking now ;-)

Thank you!

@PLTorrent Is the Sas have Reporting valve position % ⬜ implanted in the firmware ? I think its coming from the first gen Moes like the Siter was having and was never working then function is not in the firmware.

It is not.

BTW In case you wouldn't be happy with lazy room temp monitoring, and your TRV responds with local temp after calibration or local temp command, you could create a task and query TRV for local temp occasionally.

I do not want to go in this direction, since it means shortened battery life. I do not use TRV for reporting room temperature anyway, so no need for that IMHO.

jacekk015 commented 2 years ago

@PLTorrent For OnOff map_attribute could be simplified:

    def map_attribute(self, attribute, value):
        """Map standardized attribute value to dict of manufacturer values."""
        if attribute == "on_off":
            return {SASWELL_CHILD_LOCK_ATTR: value}

0,1 are both ways converted automatically between False/True

MattWestb commented 2 years ago

Its looks most TRVs is using none, 3 or many presets in ZHA. The normal 2 3 presets is:

            PRESET_NONE,
            PRESET_AWAY,
            PRESET_SCHEDULE,

            if record.value == 0:
                self._preset = PRESET_AWAY
            if record.value == 1:
                self._preset = PRESET_SCHEDULE
            if record.value == 2:
                self._preset = PRESET_NONE

Beca and some other is using more like:

            PRESET_NONE,
            PRESET_AWAY,
            PRESET_SCHEDULE,
            PRESET_ECO,
            PRESET_BOOST,
            PRESET_TEMP_MANUAL,

            if record.value == 0:
                self._preset = PRESET_AWAY
            if record.value == 1:
                self._preset = PRESET_SCHEDULE
            if record.value == 2:
                self._preset = PRESET_NONE
            if record.value == 4:
                self._preset = PRESET_ECO
            if record.value == 5:
                self._preset = PRESET_BOOST
            if record.value == 7:
                self._preset = PRESET_TEMP_MANUAL

I was thinking is it better using 2 "standard presets" for devices and better having one for every TRV-quirk that need it ? If having one for every its better if not all preset is used then is not added and the user is not getting unused presets in the GUI.

"Standard presets" is using lesser classes / code but its little confusing then all is mixes up.

PLTorrent commented 2 years ago

@MattWestb not sure what you have in mind. If you mean climate.py from ZHA then there is only the 6 presets version for MOES family. The Saswell needs only 3 presets if we want the presets used but it is not implemented in ZHA. Can be done without presets using on_off switches only. Don't know about other thermostats though.

MattWestb commented 2 years ago

I knowing and Beca is having one extra (then Moes) but is not using one (or 2) of the Moes one. Siter and Sas is having the same numbers of working modes but i think it good having presets if the device have working modes and not switches but i also not having problem if sas is using switches. The best is if ZHA climatic component was having better handles for standard function like batter low / %, window open, Children lock, schedule and so on. Also good not getting so much switches that the user must "finding" the function off. Also if user is baying different tuya TRV is good having the normal functions working the same way and the extra some device is having still must being made different for getting the functions of the device in ZHA (device card) GUI.

The bad thing with presets is that for the moment we must adding new devices in the quirk and ZHA but that i can also living with. One thing is getting ZHA "importing" presets from the quirk but its not possible for the moment.

In the end its tuya devices and its all the time coming new versions that working little different so its not possible making one "tuya standard" for all of them.

More ideas and feedback is very welcome.

MattWestb commented 2 years ago

Was looking little in the updated tuya dev docks and i think its some interesting things that can helping our mess with some tuya devices. https://developer.tuya.com/en/docs/iot-device-dev/tuya-zigbee-universal-docking-access-standard?id=K9ik6zvofpzql#subtitle-6-Private%20cluster

TY_DATA_QUERY | 0x03 | GW send, trigger MCU side to report all current information, no zcl payload. Note: Device side can make a policy, data better not to report centrally -- | -- | -- TUYA_MCU_VERSION_REQ | 0x10 | Gw->Zigbee gateway query MCU version TUYA_MCU_VERSION_RSP | 0x11 | Zigbee->Gw MCU return version or actively report version

Command 0x10 and 0x11 is implanted for some month ago in tuya INIT but i think we is not using them only that some curtins was needing the response for working OK.

Command 0x03 is not implanted and shall not being used for normal working devices but i think it can being good using for getting the data from the device then debugging how the device is working and the code is reacting with the device but i think it shall not being used for getting update from devices (like my Siter TRV that only reporting every hour).

Its more interesting information of MCU <-> Zigbee module communication but its little out of scope for our work them most we cant see or changing but can being good to knowing how tuya is working behind the DP protocol.

Perhaps querying the MCU version can helping with devices that looks behaving differently with the dame device IDs.

Also little interesting tuya is having 2 types of Zigbe serial firmware, One for sleeping end devices (our TRVs) one one for routers that is having the radio on all the time and then different versions for the Zigbee module the manufacturer is using.

I dont knowing if the device ID is written in the Zigbee module or if its querying it on startup from the zuya MCU but its still interesting to understanding little more of tuya devices that can helping fixing some strange problems user and there devices is having

jacekk015 commented 2 years ago

0x03 is needed, for example, for Tuya siren - tried to code this but I can't do this remotely - don't have this device. https://github.com/zigpy/zha-device-handlers/issues/877 It has to be send in the configuration part of connecting device. Z2M did it. I think admins should take a look on this.

https://github.com/Koenkk/zigbee-herdsman/commit/e0b6ceaeca24ede57930f2552b2332b7108ca211 https://github.com/Koenkk/zigbee-herdsman-converters/commit/368983787934a8a5b64e6169d4f518c930926d75 https://github.com/Koenkk/zigbee2mqtt/issues/7051

MattWestb commented 2 years ago

I have one similar case with switching mode of the tuya TS004F dimmer switch that can changing mode by writing one attribute then the device is in configure mode. I have looking in sniffs and Hubart and later Z2M is using the findings for getting it working but not in ZHA 😢. tuya is sending one leave with rejoining and then the device is rejoining its in "init mode" for some seconds and then is sending one device acunsment is the ZBGW setting one attribute and is changing working mode and is allocating 3 new endpoints. If doing it later the device is switching mode but is not allocating new endpoints the its in normal mode.

challs commented 2 years ago

Hey @MattWestb thanks for your comment asking about writing tests. I do have experience in writing py.test code. Is there anything specific that you were looking for?

MattWestb commented 2 years ago

@jacekk015 have making much code for new and updating all tuya TRV and we have doing some PRs but we cant getting them thru then we dont knowing how to do the test code. Some more user is also helping making the coding of the quirks for there devices but in the end its ending with working quirks but no merger PRs in ZHA.

I hope we can getting pushed in the right direction and getting the tests in place with some help and knowledge for some with experience.

I hope @jacekk015 is still having interest getting the test being done and his great work getting in side the mainline code and users dont need patching HA containers and installing external quirks for getting there TRVs working in ZHA.

challs commented 2 years ago

Hey, I have had a first look at the tests and commented over on the pull request.

@jacekk015 in the PR you said

Unfortunately I don't know the Python test approach so that parts is out of my knowledge. I've seen plenty of bytes sent RAW which I do not understand.

These have taken from the raw message logs. So you set up a test which checks that the given message results in the desired behaviour.

For example, I captured these log message from pressing a button on the remote I'm trying to implement:

[bellows.zigbee.application] Received incomingMessageHandler frame with [<EmberIncomingMessageType.INCOMING_UNICAST: 0>, EmberApsFrame(profileId=260, clusterId=6, sourceEndpoint=1, destinationEndpoint=1, options=<EmberApsOption.APS_OPTION_ENABLE_ROUTE_DISCOVERY: 256>, groupId=0, sequence=190),
   192, -52, 0xc527, 255, 255, b'\x01\x1c\xfd\x00']
[zigpy.zcl] [0xc527:1:0x0006] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=CLUSTER_COMMAND manufacturer_specific=False is_reply=False disable_default_response=False> manufacturer=None tsn=28 command_id=253>

The raw message is the last parameter shown on the first message. I made a test for the button event like this:

ZCL_TUYA_1001_SWITCH_ON = b'\x01\x1c\xfd\x00'

@pytest.mark.parametrize("quirk", (zhaquirks.tuya.ts1001.TuyaDimRemote1001,))
async def test_ts1001_state_report(zigpy_device_from_quirk, quirk):
    """Test ts1001 4 button switch."""

    switch_dev = zigpy_device_from_quirk(quirk)

    switch_cluster = switch_dev.endpoints[1].out_clusters[OnOff.cluster_id]
    switch_listener = ClusterListener(switch_cluster)

    hdr, args = switch_cluster.deserialize(ZCL_TUYA_1001_SWITCH_ON)
    switch_cluster.handle_message(hdr, args)

    # We expect to have seen an event sent and a command (response to device)
    assert len(switch_listener.cluster_commands) == 1
    assert len(switch_listener.events_sent) == 1
    assert len(switch_listener.attribute_updates) == 0
    assert switch_listener.events_sent[0][0] == SHORT_PRESS

The test sets up a mock device and cluster, and then sends the message to the cluster. The ClusterListener is just a helper class that subscribes to messages and remembers which messages it has seen.

challs commented 2 years ago

Hey, I have had a first look at the tests and commented over on the pull request.

I need some input on that - I don't have the device and I need to know how best to handle the missing temperature offset setting in the tests. Does anyone here have one of these?

@jacekk015 in the PR you said

Unfortunately I don't know the Python test approach so that parts is out of my knowledge. I've seen plenty of bytes sent RAW which I do not understand.

These have taken from the raw message logs. So you set up a test which checks that the given message results in the desired behaviour.

For example, I captured these log message from pressing a button on the remote I'm trying to implement:

[bellows.zigbee.application] Received incomingMessageHandler frame with [<EmberIncomingMessageType.INCOMING_UNICAST: 0>, EmberApsFrame(profileId=260, clusterId=6, sourceEndpoint=1, destinationEndpoint=1, options=<EmberApsOption.APS_OPTION_ENABLE_ROUTE_DISCOVERY: 256>, groupId=0, sequence=190),
   192, -52, 0xc527, 255, 255, b'\x01\x1c\xfd\x00']
[zigpy.zcl] [0xc527:1:0x0006] ZCL deserialize: <ZCLHeader frame_control=<FrameControl frame_type=CLUSTER_COMMAND manufacturer_specific=False is_reply=False disable_default_response=False> manufacturer=None tsn=28 command_id=253>

The raw message is the last parameter shown on the first message. I made a test for the button event like this:

ZCL_TUYA_1001_SWITCH_ON = b'\x01\x1c\xfd\x00'

@pytest.mark.parametrize("quirk", (zhaquirks.tuya.ts1001.TuyaDimRemote1001,))
async def test_ts1001_state_report(zigpy_device_from_quirk, quirk):
    """Test ts1001 4 button switch."""

    switch_dev = zigpy_device_from_quirk(quirk)

    switch_cluster = switch_dev.endpoints[1].out_clusters[OnOff.cluster_id]
    switch_listener = ClusterListener(switch_cluster)

    hdr, args = switch_cluster.deserialize(ZCL_TUYA_1001_SWITCH_ON)
    switch_cluster.handle_message(hdr, args)

    # We expect to have seen an event sent and a command (response to device)
    assert len(switch_listener.cluster_commands) == 1
    assert len(switch_listener.events_sent) == 1
    assert len(switch_listener.attribute_updates) == 0
    assert switch_listener.events_sent[0][0] == SHORT_PRESS

The test sets up a mock device and cluster, and then sends the message to the cluster. The ClusterListener is just a helper class that subscribes to messages and remembers which messages it has seen.

I've also been working on making some test fixtures to set things up and make it easier to write some tests.

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

jzielke84 commented 1 year ago

@PLTorrent The latest HA Update seems to have broken this quirk:

Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/config_entries.py", line 365, in async_setup
    result = await component.async_setup_entry(hass, self)
  File "/usr/src/homeassistant/homeassistant/components/zha/__init__.py", line 101, in async_setup_entry
    setup_quirks(config)
  File "/usr/local/lib/python3.10/site-packages/zhaquirks/__init__.py", line 409, in setup
    importer.find_module(modname).load_module(modname)
  File "<frozen importlib._bootstrap_external>", line 548, in _check_name_wrapper
  File "<frozen importlib._bootstrap_external>", line 1063, in load_module
  File "<frozen importlib._bootstrap_external>", line 888, in load_module
  File "<frozen importlib._bootstrap>", line 290, in _load_module_shim
  File "<frozen importlib._bootstrap>", line 719, in _load
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/config/quirks/trv_saswell.py", line 21, in <module>
    from zhaquirks.tuya.ts0601_trv_sas import BATTERY_REPORTED
ImportError: cannot import name 'BATTERY_REPORTED' from 'zhaquirks.tuya.ts0601_trv_sas' (/usr/local/lib/python3.10/site-packages/zhaquirks/tuya/ts0601_trv_sas.py)

Could you be so kind and try to fix this?