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

Improve Legrand 064882 cable outlet heat mode #3031

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

Conversation

piitaya
Copy link

@piitaya piitaya commented Mar 11, 2024

Proposed change

Following #2807

Pilot wire mode has been renamed to heat mode.
Enum for device_mode has been removed as it's not used.
The value is now exposed and you can read and write the cluster of the attribute. This will allow the creation of a climate entity in ZHA to control the heat mode.

Additional information

A PR has be open in Home Assistant Core to add the support using climate entity : home-assistant/core#113059

To use the climate entity, user will needed to pass the device in wire pilot mode by toggling the new switch entity : wire pilot mode

image

image

image

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 Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.94%. Comparing base (86fafd6) to head (144fe4c).
Report is 9 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #3031      +/-   ##
==========================================
+ Coverage   87.86%   87.94%   +0.08%     
==========================================
  Files         301      301              
  Lines        9219     9249      +30     
==========================================
+ Hits         8100     8134      +34     
+ Misses       1119     1115       -4     

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

@dmulcahey
Copy link
Collaborator

dmulcahey commented Mar 11, 2024

After the cluster changes are cleaned up we should look at converting this to a v2 quirk instead which would remove the need to open a PR in HA.

@dmulcahey
Copy link
Collaborator

After the cluster changes are cleaned up we should look at converting this to a v2 quirk instead which would remove the need to open a PR in HA.

legrand.patch

@piitaya
Copy link
Author

piitaya commented Mar 11, 2024

@dmulcahey How does it work with translation_key ? Where the translations file should be located?

@dmulcahey
Copy link
Collaborator

@dmulcahey How does it work with translation_key ? Where the translations file should be located?

Translations will default to the attribute name. Strings.json will still need a HA PR

@piitaya
Copy link
Author

piitaya commented Mar 11, 2024

I updated to quirk v2.
I didn't add the enum to create the select entity because of multiple reason :

  1. entity_type seems to not work with standard. Also, why is it called entity_type instead of entity_category? Standard is not a category, the category should be set to None instead.
  2. the ZCL select entity replace _ by spaces in Home Assistant that make hard the usage of entity translations. IMO, it should convert the enum in lowercase and keep the _.
    What do you think @dmulcahey?

I also have an issue with tests

@dmulcahey
Copy link
Collaborator

tests will pass after the Zigpy bump I think.

@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Mar 11, 2024

entity_type seems to not work with standard. Also, why is it called entity_type instead of entity_category? Standard is not a category, the category should be set to None instead

Yeah, some classes like ZCLEnumSelectEntity (in ZHA) override the entity category if it's None for now.
The main use case is to provide config entities via quirks v2. I think this would be the first case where we'd have a select entity that's not a config entity.
#3030 even mentions that this device could provide a climate entity in HA. This is not possible via quirks v2, but might be worth considering?
FYI, the metadata and entity type is matched a ZHA class here.

the ZCL select entity replace _ by spaces in Home Assistant that make hard the usage of entity translations. IMO, it should convert the enum in lowercase and keep the _.

Yeah, the enum states aren't translatable at the moment. There's this PR: home-assistant/core#109309, but I see some issues with state restoration when coming from an old version (due to _/ ) for ZHAEnumSelectEntity (used for non-ZCL stuff like IasWd).
That would need to be addressed (and the test for quirks v2 to see if translation keys are provided would need to be expanded).
It's still something I plan on looking at/working on, but the enums (split between quirks and ZHA atm) would all stay the same at least. Like you mentioned, ZHA would just lower-case them for the translation keys.

I also have an issue with tests

A helper for creating a quirk v2 device is added with #3032

@piitaya
Copy link
Author

piitaya commented Mar 11, 2024

#3030 even mentions that this device could provide a climate entity in HA. This is not possible via quirks v2, but might be worth considering?

Yes I was considering this too. I'm not sure my python level is enough for that but I can give a try 🙂 As I know, there is no standard attribute for the preset in the thermostat cluster right?

The idea is to map the 6 heat mode into 2 hvac modes and 5 presets :

  • heat - splitted into 5 presets :
    • comfort preset
    • comfort-1 preset
    • comfort-2 preset
    • eco preset
    • away preset
  • off

I already did something similar in my home assistant qubino custom component : https://github.com/piitaya/home-assistant-qubino-wire-pilot

@TheJulianJES
Copy link
Collaborator

TheJulianJES commented Mar 11, 2024

Yes I was considering this too. [...] As I know, there is no standard attribute for the preset in the thermostat cluster right?

Correct, at least not one that's mapped to HA presets for the "generic thermostat implementation" in ZHA.
Some thermostats are implemented like this for presets: homeassistant/components/zha/climate.py#L578-L667 (and further below are some other examples).
The "comfort -1" and "comfort -2" presets might be an issue though (since they don't seem to exist in HA)?

(Also, other devices like the Aqara E1 thermostat currently just implements the preset mode (manual, auto, away) as a config select entity. Of course, it would be better if it were added to the climate entity though.)

@luke7101
Copy link

#3030 even mentions that this device could provide a climate entity in HA. This is not possible via quirks v2, but might be worth considering?

Yes I was considering this too. I'm not sure my python level is enough for that but I can give a try 🙂 As I know, there is no standard attribute for the preset in the thermostat cluster right?

The idea is to map the 6 heat mode into 2 hvac modes and 5 presets :

  • heat - splitted into 5 presets :

    • comfort preset
    • comfort-1 preset
    • comfort-2 preset
    • eco preset
    • away preset
  • off

I already did something similar in my home assistant qubino custom component : https://github.com/piitaya/home-assistant-qubino-wire-pilot

Just my 2 cents :
pilot wire has 6 modes: comfort, comfort-1, comfort-2, eco, away and off (in my experience comfort-1 and -2 aren't used very often)
The off order is a "soft off" ie the heater stops heating but it's still powered.

Legrand 064882 cable outlet provides a "hard off" state, by toggling the switch in standard mode, that completely turns off the power (ie no power on the L wire)

It's a big advantage compared to the other devices I tried because, for example:

  • you can completely switch off the power over summer to prevent your kids to turn the heater on unintentionally, convenient if the heater doesn't have a physical switch
  • you can avoid electric smog when you don't absolutely need to heat (heaters have huge electric field >100V/m at 30 cm) still present in the soft off pilot wire state, and if you would like to turn it off automatically in the inter-season without toggling the circuit breaker once or twice a day !

What I'm trying to say is that you maybe should keep the 2 different off modes, mapping it for example like that;

  • heat - splitted into 4 (or 6 presets ?) :

    • comfort preset
    • comfort-1 preset?
    • comfort-2 preset?
    • eco preset
    • away preset
    • off preset (soft off)
  • off (hard off mode)

@piitaya piitaya marked this pull request as ready for review March 15, 2024 09:19
@piitaya piitaya marked this pull request as draft March 19, 2024 09:53
@piitaya piitaya marked this pull request as ready for review March 19, 2024 20:02
@piitaya
Copy link
Author

piitaya commented Mar 22, 2024

I removed the thermostat cluster, the zha climate integration uses the manufacturer cluster.

attr_def = self.find_attribute(attr)
attr_id = attr_def.id
if attr_id == WIRE_PILOT_MODE_ATTR:
attrs[0x0000] = [0x02, 0x00] if value else [0x01, 0x00]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0x0000 -> LegrandCluster.AttributeDefs.device_mode.id

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do the byte arrays represent? We should name them at a minimum.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really known why it uses array instead of boolean. My assumption is that legrand is using same attribute for various devices.

type=t.data16,
is_manufacturer_specific=True,
)
led_dark = ZCLAttributeDef(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do and should it have some sort of config entity in the quirk?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was here before so I didn't touched it. I will look in details and I may provide a config switch for those attributes if it makes sense in another PR.

type=t.Bool,
is_manufacturer_specific=True,
)
led_on = ZCLAttributeDef(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a switch config entity for this to the quirk below?

@luke7101
Copy link

Hi, will this PR be merged some day, or is there a way to test its current progress ?
Thanks!

@piitaya
Copy link
Author

piitaya commented Apr 12, 2024

I need to write tests in ha core but I don't have much time at the moment. I keep you updated 😊

@luke7101
Copy link

Great ! Thanks 😉

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.

None yet

5 participants