-
Notifications
You must be signed in to change notification settings - Fork 29
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 fil pilote #495
base: development
Are you sure you want to change the base?
Add fil pilote #495
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @NatMarchand - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
ea2615a
to
d58dec6
Compare
Add ApplianceType mixin for legrand modules supporting it Make rooms with NLC radiator supporting climate Change async_therm_set to add fil pilote value
@@ -121,6 +130,7 @@ async def async_therm_set( | |||
self, | |||
mode: str, | |||
temp: float | None = None, | |||
fp: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is fp? Can we find a better name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fil pilote (Pilot wire in english ?)
I've chose this for the consistency with what's used in the API but I'd be eager to change :
For example the /homestatus endpoint returns this :
{
status:"ok"
time_server:1712686604
body: {
home: {
id:"xxxx"
rooms:[{
id:"xxxx"
therm_setpoint_end_time:1712693783
therm_setpoint_fp:"frost_guard"
therm_setpoint_mode:"home"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, still I can't make much sense of that name. Not your fault, clearly, don't get me wrong. Could you explain the purpose? I can't make sense of the docs on that. Could it be 'Room heating configuration' or 'Home heating mode' (fp) vs. 'Heating schedule' (mode)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In France we have a standard for electric radiator which is fil pilote. It's a fourth wire which sends commands to a radiator (comfort, eco, frost guard, off).
When you have a Legrand cable outlet configured as a radiator, you can drive the radiator. However, in the API the command is configured on the room as you'd do for a thermostat (without a temperature).
That's what I'm trying to achieve on my PR : make the rooms available as thermostat in home assistant and be able to set commands on radiators.
It can be seen as a "room heating preset" and can be driven in the Legrand app by a schedule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just name it something like pilot wire to have a speaking name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure ! Will do ;)
Any news on this request? |
Still working on it, I ain't got much time to dedicate. |
Let me know if I can help you. |
This PR is still in draft state. Please set it as "ready for review" if you're happy your changes |
Thanks but currently I can't test more as the current weather is quite hot and I can't turn on radiator 😉 |
@NatMarchand any progress ? It's starting to be cold again :) I just recently switched from Zigbee2MQTT to Home+Control gateway for my radiators and was disapointed to see the HA Netatmo integration didn't allow to control them correctly. I'd be happy to help if you can tell more about what's still preventing the PR to be ready, and how you test the changes in HA |
Add ApplianceType mixin for legrand modules supporting it
Make rooms with NLC radiator supporting climate
Change async_therm_set to add fil pilote value