core icon indicating copy to clipboard operation
core copied to clipboard

Add support for Tuya fault sensors

Open MrAdam opened this issue 1 year ago • 8 comments

Proposed change

Added support for Tuya fault sensors, indicating any active fault sensors on a Tuya product.

I noticed that my MeacoDry Arete Two 12L exposed a field called fault, with a type of Bitmap, and the following spec:

{
  "label": [
    "tankfull",
    "defrost",
    "E1",
    "E2",
    "L2",
    "L3",
    "L4",
    "wet"
  ]
}

I then found out through Tuya documentation that this is a general field, so I decided to try and implement detection and parsing of this field.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

MrAdam avatar Sep 02 '24 18:09 MrAdam

Hey there @tuya, @zlinoliver, @frenck, mind taking a look at this pull request as it has been labeled with an integration (tuya) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of tuya can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign tuya Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Sep 02 '24 18:09 home-assistant[bot]

I'm aware that some code changes might be needed, for example extracting the new logic in is_on(self) to separate methods. I just wanted to get some feedback on my code before I did the final prettifying.

MrAdam avatar Sep 02 '24 18:09 MrAdam

I've done some cleanup of the code and extracted the new logic to separate methods. I hope maybe @frenck will get some time to look at it at some point - I'm very open to feedback.

MrAdam avatar Sep 12 '24 18:09 MrAdam

@frenck any chance you can take a look at my PR?

MrAdam avatar Oct 17 '24 10:10 MrAdam

@frenck I fixed the ruff and mypy errors encountered after you merged the latest dev branch into my PR, so it should be fine to review now. I had some problems getting the devcontainer to work when I developed this, which is why I missed those checks - got it working now though.

MrAdam avatar Nov 13 '24 18:11 MrAdam

@frenck, any chance you have time to look at my PR, or can help me find someone else who can? I know you are busy, but it's quite a small PR 🤞

MrAdam avatar Dec 05 '24 07:12 MrAdam

@frenck pinging you again. It's been almost 5 months, so I hope you will get some time to look at this.

MrAdam avatar Jan 29 '25 10:01 MrAdam

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Feb 28 '25 21:02 home-assistant[bot]

@frenck it looks like several PR's have been opened since I opened mine back in September, trying to solve the same issue: https://github.com/home-assistant/core/pull/129437 https://github.com/home-assistant/core/pull/137771

We should probably figure out which one should be merged, and close the others as they solve the same problem.

@epenet and @jbouwh I can see that you have been reviewing one of the other PR's but it looks like the author hasn't gotten back yet - maybe you can help me get this reviewed and merged, so we can get this feature added?

MrAdam avatar May 13 '25 06:05 MrAdam

I think this PR looks like the most promising of the three alternatives.

@MrAdam Could you please add some tests though? Note that we really want tests to set up the integration and check the resulting state in the state machine unlike what the tests in the alternative PRs do.

Please feel free to reach out to me on Discord if you need some help with implementing tests.

emontnemery avatar May 26 '25 12:05 emontnemery

I think this PR looks like the most promising of the three alternatives.

@MrAdam Could you please add some tests though? Note that we really want tests to set up the integration and check the resulting state in the state machine unlike what the tests in the alternative PRs do.

Please feel free to reach out to me on Discord if you need some help with implementing tests.

I'll give it a try. Do you have a reference to an integration that does it in the best possible way, just so I can see how it should be structured?

MrAdam avatar May 27 '25 07:05 MrAdam

I leave some review comments. Also, I think we need to expand this code to whole bitmap types not for faults only

frickua avatar May 27 '25 13:05 frickua

I resolve some own coments on this PR https://github.com/MrAdam/core/pull/1

frickua avatar May 27 '25 13:05 frickua

@MrAdam, take a look, for pr to this branch, maybe it would be simple to merge it https://github.com/MrAdam/core/pull/1

About bitmaps - padding is needed only when converted to binary string representation, bitwise shift with & works well with "all" numbers. Tuya API returning dec number, so loss is not possible. Also, if shift used - array length no need any more.

frickua avatar May 28 '25 07:05 frickua

It will probably take some time before I can look at this again (I have started looking into tests), as I just started a new job this week, and am a bit swamped.

MrAdam avatar Jun 03 '25 15:06 MrAdam

@emontnemery I've added some tests, and tried to work with the state machine - I hope you will take a look and see if it's done correctly 👍

MrAdam avatar Jun 18 '25 17:06 MrAdam

I can see that coverage is failing, due to me not having done any negative tests yet - I'll get that fixed soon.

MrAdam avatar Jun 20 '25 05:06 MrAdam

Replaced by #148485 If you want to add E1 / E2 / L2 / L3 / L4 this can be done in a follow-up PR

epenet avatar Jul 10 '25 14:07 epenet