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

Add Device Support: Parkside water valve _TZE200_htnnfasr #1574

Merged
merged 13 commits into from
May 25, 2022

Conversation

rforro
Copy link
Contributor

@rforro rforro commented May 21, 2022

Adding support for LIDL PARKSIDE Water Valve

Following is supported by the quirk:

  • On/Off state reporting
  • Change open timer duration
  • Countdown reporting
  • Frost lock reporting
  • Frost lock reseting

Support not planned:

  • Week Scheduler (use HA automation instead)

This PR closes this #933 and #1452

This can be merged before solving following issue #1566 because quirk will stay as is.

image

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2022

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.29%. Comparing base (996c26a) to head (afffb23).
Report is 579 commits behind head on dev.

Files Patch % Lines
zhaquirks/tuya/ts0601_valve.py 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1574      +/-   ##
==========================================
+ Coverage   80.27%   80.29%   +0.02%     
==========================================
  Files         231      231              
  Lines        6904     6927      +23     
==========================================
+ Hits         5542     5562      +20     
- Misses       1362     1365       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rforro
Copy link
Contributor Author

rforro commented May 21, 2022

Please look at the folder structure. I put the quirk in LIDL folder, but pytest is uses regular tuya namingtest_tuya_water_valve.py (future valves can be put there)

@coveralls
Copy link

coveralls commented May 21, 2022

Pull Request Test Coverage Report for Build 2382658234

  • 22 of 25 (88.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 80.294%

Changes Missing Coverage Covered Lines Changed/Added Lines %
zhaquirks/tuya/ts0601_valve.py 10 13 76.92%
Totals Coverage Status
Change from base Build 2381351596: 0.02%
Covered Lines: 5562
Relevant Lines: 6927

💛 - Coveralls

@MattWestb
Copy link
Contributor

I have pointing out before that the LIDK folder shall being merged to the tuya for not getting one "Z2M" structure then having 15 different names / folders for the more or less the same device,
Like MOES is using nearly all tuya devices and making one folder for them is useless the most device is also sold by other brands and in the end all it tuya devices.

GREAT WORK DONE Augsburg !!

@javicalle
Copy link
Collaborator

I'm not sure now, but I believe that there's no need for the TuyaNewPowerConfigurationCluster.
The TuyaLocalCluster class is needed for write actions on fake clusters, but being th PowerConfiguration a read-only cluster, I'd say it's not needed.
Could you validate that the DoublingPowerConfigurationCluster cluster is enough?

There is also another approach that could be usefull for casting that Tuya spell around the devices.
My suggestion would be:

  • new BewitchedDevice class (or the name that you consider more appropriate):
class BewitchedDevice(CustomDevice):
    """Cast a magic spell for Tuya devices that needed."""

    def __init__(self, *args, **kwargs):
        """Initialize with task."""
        super().__init__(*args, **kwargs)

        # cast_tuya_magic_spell(self, tries=3)
        self._init_plug_task = asyncio.create_task(self.spell())

    async def spell(self) -> None:
        """Initialize device so that all endpoints become available."""
        basic_cluster = self.endpoints[1].in_clusters[0]

        _LOGGER.debug("Device class will cast Tuya Magic Spell")
        attr_to_read = [4, 0, 1, 5, 7, 0xFFFE]
        await basic_cluster.read_attributes(attr_to_read)
  • use the new class wherever it is needed:
class ParksidePSBZS(BewitchedDevice, CustomDevice):
    """LIDL Parkside water without implemented scheduler."""

    signature = {
        MODELS_INFO: [("_TZE200_htnnfasr", "TS0601")],  # HG06875
        ENDPOINTS: {
            # <SimpleDescriptor endpoint=1 profile=260 device_type=0
            .../...

Some comments:

  • IMHO @MattWestb has earned the right to name the new class
  • if I am not wrong, the spell cast is only needed in MCU devices, so I would put the new class in zhaquirks.tuya.mcu.__init__.py

What do you think?

@javicalle
Copy link
Collaborator

Ahhh! and I agree to put the class inside the Tuya folder(tuya/ts0601_valve.py). Probably I would keep the ParksidePSBZS for now until we can see if it is a particular device or is a generic one.

@MattWestb
Copy link
Contributor

@javicalle We was first needing it for the famous TS004F DSR but our hubart friends is using it on many MCU devices that looks need it being casted for getting all function working OK (most temp and humidity sensors with display) and also the updated LIDL power strips is needing it for using all 3 endpoint (its one "normal" Zigbee device).

I think it can being good implanting the tMS (tuya Magic Spell) casting for all MCU devices but its need testing so not braking devices if its changing the device functionality of the device.
Perhaps implanting it in your new MCU and converting all MCU device to it then you is getting them ready for it so having control if somthing is not working OK and can fixing it easy and not forcing all MCU devices at one time now and old implementation) and getting large problems with broken devices for our users.

One side note of tuya MCU devices is the Zigbee module from the factory not having its real manufacture name then its being flashed its using one standard factory ones but then connecting to one tuya MCU its getting one updated manufacture name its storing in the NVM (flash region "user data") and is using it in the Zigbee network (have getting one dumped user data and flash from one tuya TRV that is verifying the data settings of the Zigbee module). And i think the tMS casting is also making some things being sent from the zigbee module to the MCU for getting it initiated OK.

@rforro
Copy link
Contributor Author

rforro commented May 21, 2022

Could you validate that the DoublingPowerConfigurationCluster cluster is enough?

For some reason it wasn't enough, because I've begun with that class. I saw it in the other Irrigation valve PR, but had no success.

Ahhh! and I agree to put the class inside the Tuya folder(tuya/ts0601_valve.py)

I don't like the generic word valve, because it can be the Thermostatic Valve or Gas Valve. ts0601_water_valve is easier to understand.

@javicalle
Copy link
Collaborator

javicalle commented May 21, 2022

For some reason it wasn't enough, because I've begun with that class. I saw it in the other Irrigation valve PR, but had no success.

Ok, then I would prefer without the 'new' part. Just TuyaPowerConfigurationCluster or maybe TuyaDoublePowerConfigurationCluster.

I don't like the generic word valve, because it can be the Thermostatic Valve or Gas Valve. ts0601_water_valve is easier to understand.

I see your point, but IMO the question would be which devices use this implementation and not 'what are these devices used for' (water, gas, ...) Recently, another 'valve' has appeared that can be used for water or gas:

IMO I would put all the valves in the same class. But it's not something that bothers me too much.

@rforro
Copy link
Contributor Author

rforro commented May 22, 2022

Ok, then I would prefer a new without the 'new' part. Just TuyaPowerConfigurationCluster or maybe TuyaDoublePowerConfigurationCluster.

The original TuyaPowerConfigurationCluster is doubling as well, i think its normal in tuya world.

class TuyaPowerConfigurationCluster(LocalDataCluster, PowerConfiguration):
"""PowerConfiguration cluster for battery-operated thermostats."""
def __init__(self, *args, **kwargs):
"""Init."""
super().__init__(*args, **kwargs)
self.endpoint.device.battery_bus.add_listener(self)
def battery_change(self, value):
"""Change of reported battery percentage remaining."""
self._update_attribute(
self.attributes_by_name["battery_percentage_remaining"].id, value * 2
)

@MattWestb
Copy link
Contributor

The tuya MCU is sending battery power left as 100% that is OK for there cloud but Zigbee standard is saying it shall being sent 200% then the battery is full.
.
Some "wild ones" Zigbee devices is using 100% (all IKEA sleepers) and need one quirk for fixing it sending 200% to ZHA and its looks nice 100% in the HA GUI and not 50% without the quirk.

@rforro
Copy link
Contributor Author

rforro commented May 25, 2022

all proposed changes have been applied, but two things:

Do any of you know how to test following methods:

 async def bind(self):
        """
        Bind cluster.
        When adding this device tuya gateway issues factory reset,
        we just need to reset the frost lock, because its state is unknown to us.
        """
        result = await super().bind()
        await self.write_attributes({self.attributes_by_name["frost_lock_reset"].id: 0})
        return result

and

    def __init__(self, *args, **kwargs):
        """Initialize with task."""
        super().__init__(*args, **kwargs)
        self._init_device_task = asyncio.create_task(self.spell())

    async def spell(self) -> None:
        """Initialize device so that all endpoints become available."""
        attr_to_read = [4, 0, 1, 5, 7, 0xFFFE]
        basic_cluster = self.endpoints[1].in_clusters[0]
        await basic_cluster.read_attributes(attr_to_read)

it can be merged without this tests, but the coverage percentage is lower.

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.

6 participants