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 NodOn SIN-4-FP-21 quirk #3364

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Conversation

ikruglov
Copy link

@ikruglov ikruglov commented Sep 15, 2024

Proposed change

This PR adds a quirk for SIN-4-FP-21 (PilotWire). The device reports itself as a SmartPlug but it has special manufacturer-specific cluster which allows setting the following modes:

        Off = 0x00
        Comfort = 0x01
        Eco = 0x02
        FrostProtection = 0x03
        ComfortMinus1 = 0x04
        ComfortMinus2 = 0x05

Additional information

The result look like this in Home Assitant:
Screenshot 2024-09-22 at 22 16 55

Checklist

  • The changes are tested and work correctly
  • pre-commit checks pass / the code has been formatted using Black
  • Tests have been added to verify that the new code works

Copy link

codecov bot commented Sep 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.74%. Comparing base (fa734ad) to head (606521e).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3364      +/-   ##
==========================================
+ Coverage   88.71%   88.74%   +0.02%     
==========================================
  Files         306      308       +2     
  Lines        9820     9843      +23     
==========================================
+ Hits         8712     8735      +23     
  Misses       1108     1108              

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

@TheJulianJES TheJulianJES changed the title NodOn: quirk for SIN-4-FP-21 Add NodOn SIN-4-FP-21 quirk Sep 16, 2024
@TheJulianJES TheJulianJES added the new quirk Adds support for a new device label Sep 16, 2024
@ikruglov ikruglov force-pushed the nodon-pilot-wire branch 3 times, most recently from ed08401 to 228e8a9 Compare September 22, 2024 13:47
@ikruglov
Copy link
Author

ikruglov commented Sep 22, 2024

@TheJulianJES I've re-pushed commits to use the v2 interface of QuirkBuilder().
Unfortunately, I didn't find an example of how to write tests for it. The tests that I wrote for the previous version didn't work due to compatibility issues, I believe.

If you think tests are necessary here, please give me an example of any test that uses the v2 interface.

@TheJulianJES TheJulianJES added the v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA. label Sep 24, 2024
@pounard
Copy link

pounard commented Oct 9, 2024

Any news on this? I would love this to be merged, I myself own a bunch of those modules.

@lboue
Copy link

lboue commented Oct 11, 2024

Please validate. I need this quirk as well

@ikruglov
Copy link
Author

ikruglov commented Oct 12, 2024

@TheJulianJES, I have a question. I didn't find a better place to ask it.

There is a slight problem with this quirk. An entity added to HA gets the '_none' suffix (see attached picture). I would expect it to get the '_pilot_wire_mode' suffix. Of course, one can rename the HA entity, but getting it right from the start is better.

I did a lot of debugging on my side but got lost, to be honest. I see that the ZHA objects receive the correct _unique_id_suffix value (which was my initial suspicion). But HA's entity still has '_none'.

This problem is not only with an enum but also with a configuration number. I'm preparing a PR to allow configuring impulse mode on NodOn switches (similar to this). My entity also has the _none suffix.

I'd appreciate your help or guidance.

P.S. With no success, I've tried setting translation_key and fallback_name attributes in quirks.

Screenshot 2024-10-12 at 11 09 06

@TheJulianJES
Copy link
Collaborator

The none suffix is caused by the fact that HA doesn't have a value for the translation key. In future versions, HA will fall back to `fallback_name.
When your PR is merged, the quirks bump merged to ZHA, the ZHA bump merged to HA, we'd need to add the translation key to HA then.

@TheJulianJES TheJulianJES added the needs review This PR should be reviewed soon label Oct 15, 2024
@ikruglov
Copy link
Author

The none suffix is caused by the fact that HA doesn't have a value for the translation key. In future versions, HA will fall back to `fallback_name.

When your PR is merged, the quirks bump merged to ZHA, the ZHA bump merged to HA, we'd need to add the translation key to HA then.

Thank you for the answer. Can you point out where I should add it in HA? I want to try it locally. I will change HA's code in my installation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review This PR should be reviewed soon new quirk Adds support for a new device v2 quirk Quirks using v2 API. Might add custom entities that need translation keys in HA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants