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

Align consumption sensor names in ViCare integration #127888

Merged
merged 11 commits into from
Oct 20, 2024

Conversation

CFenner
Copy link
Contributor

@CFenner CFenner commented Oct 7, 2024

Proposed change

This aligns the sensor naming for gas / energy consumption sensors and make it explicit that this is about electrical energy consumption.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • 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

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • 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:

@CFenner
Copy link
Contributor Author

CFenner commented Oct 7, 2024

@NoRi2909 what do you think, will this do?

@CFenner CFenner marked this pull request as ready for review October 8, 2024 10:34
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Tests are failing

Comment on lines 237 to 255
"name": "Electrical energy consumption for heating today"
},
"energy_summary_consumption_heating_currentmonth": {
"name": "Heating energy consumption this month"
"name": "Electrical energy consumption for heating this month"
},
"energy_summary_consumption_heating_currentyear": {
"name": "Heating energy consumption this year"
"name": "Electrical energy consumption for heating this year"
},
"energy_summary_consumption_heating_lastsevendays": {
"name": "Heating energy consumption last seven days"
"name": "Electrical energy consumption for heating last seven days"
},
"energy_dhw_summary_consumption_heating_currentday": {
"name": "DHW energy consumption today"
"name": "Electrical energy consumption for DHW today"
},
"energy_dhw_summary_consumption_heating_currentmonth": {
"name": "DHW energy consumption this month"
"name": "Electrical energy consumption for DHW this month"
},
"energy_dhw_summary_consumption_heating_currentyear": {
"name": "DHW energy consumption this year"
"name": "Electrical energy consumption for DHW this year"
Copy link
Member

Choose a reason for hiding this comment

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

Okay I saw that there was a whole discussion (I did not read, sorry for that), but let me ask a quick question, would it make sense to keep the start of the name contain the type? Because if I imagine a device with these entities, on the device page everything looks like Electrical energy consum.... And if I translate it to Dutch Energie consumptie .... (I am not sure how wide it will show, but with this long name, I can see it being cut off, that's my point).

Would it make sense to have Heating electrical energy consumption today? (under the assumption the reason this PR exists is because of the added emphasis on eletrical).

(I think usually I wouldn't make this big of a deal of this, but in this case it can be 12(?) entities, so that would most likely lead to people renaming the things).

(Also, added question, on what level is this information collected? Would it make sense to only show the heating usage on the heating device and DHW on the DHW device?)

@home-assistant home-assistant bot marked this pull request as draft October 8, 2024 12:06
@home-assistant
Copy link

home-assistant bot commented Oct 8, 2024

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

Learn more about our pull request process.

@NoRi2909
Copy link
Contributor

NoRi2909 commented Oct 8, 2024

@joostlek The main point was changing "energy" to "electrical energy" to clarify that those entities just report the electrical power drawn by a gas boiler or fuel cell, not the energy contained in the gas burned. The latter are in "gas consumption" entities, reporting in cubic meters.

Looking at the "gas consumption" entities keeping "Heating" and "DHW" at the beginning of all entities to get them grouped together makes sense.

Note: In German we can translate "Electrical energy consumption" as "Stromverbrauch" (vs. "Gasverbrauch") to make it much shorter (something that doesn't happen that often).

@joostlek
Copy link
Member

joostlek commented Oct 8, 2024

So that means you agree with what I'm saying?

@NoRi2909
Copy link
Contributor

NoRi2909 commented Oct 8, 2024

@joostlek For me the word order like

  DHW electrical energy consumption today

and likewise is OK – my suggestion that started this was just about adding "electrical" here.

But I wanted to give @CFenner the chance to comment as well as he submitted the final proposed change.

@joostlek
Copy link
Member

joostlek commented Oct 8, 2024

Yea, I also want to do that, but I wanted to double check if I understood correctly

@NoRi2909
Copy link
Contributor

NoRi2909 commented Oct 8, 2024

@joostlek Could you explain to a non-programmer like me where we're stuck with the change?

I had some more thoughts about the suggested changes and we might just shorten the names in English just like I mentioned for the planned German translations. It just did not "click" in my head that we could do exactly the same in English:

By simply using "electricity" instead of expanding "energy" to "electrical energy" in the names.

All those entities begin with "energy_" anyhow and the unit is kWh for all of them.
So it remains inherently clear that those are all reporting energy measurements:

 English                                German
 DHW electricity consumption  …         Warmwasser-Stromverbrauch …     (4 variants)
 Heating electricity consumption …      Heiz-Stromverbrauch …           (4 variants)

So that matches perfectly with the existing gas consumptions measured in m³:

 English                                German
 DHW gas consumption  …                 Warmwasser-Gasverbrauch …       (8 variants)
 Heating gas consumption …              Heiz-Gasverbrauch …             (8 variants)

Those really avoid the limited space problem we have in the Entities lists.
Perhaps you can try again with those instead on the next run, too?

@CFenner CFenner marked this pull request as ready for review October 11, 2024 19:47
@home-assistant home-assistant bot requested a review from joostlek October 11, 2024 19:47
@CFenner
Copy link
Contributor Author

CFenner commented Oct 11, 2024

@joostlek the translation and translation keys are diverging now. Would it be safe to adjust them as well or will this lead to a bunch of new entities due to an entity id change?

@CFenner
Copy link
Contributor Author

CFenner commented Oct 16, 2024

align with #127274

@NoRi2909
Copy link
Contributor

NoRi2909 commented Oct 16, 2024

@CFenner I also noticed a while ago that the English "Circulation pump" is bit misleading at it misses a reference to heating. Thus it has the current German translation "Zirkulationspumpe" that Germans will associate with hotwater:

"circulation_pump": {
"name": "Circulation pump"
},

compare to

"domestic_hot_water_circulation_pump": {
"name": "DHW circulation pump"
},

So for consistency it makes sense to add "heating" to the first string:

"Heating circulation pump"

In German this will then translate as "Heizkreispumpe" or "Heizungsumwälzpumpe".

Here I fixed this for my own installation by renaming the entity in HA, and as this is the only pump in our heating it is in fact the "Heizkreispumpe":

Screenshot 2024-10-16 16 09 43

Note that currently "Running" is still translated as "Wird ausgeführt" which is complete nonsense in the HA context. I fixed that to now say: "Betriebszustand: In Betrieb / Außer Betrieb". Makes sense because in German not everything that runs "läuft" or "läuft nicht" as a status.

@joostlek joostlek merged commit 1f9c06e into home-assistant:dev Oct 20, 2024
31 checks passed
@NoRi2909
Copy link
Contributor

Great we got this fixed. I already matched everything in the German translation.

@CFenner Please have a look at my comment above regarding the heating circulation pump.
In addition there might be some more cleanup necessary regaging PV vs. solar.

We do have a bunch of entities that contain photovoltaic:

"photovoltaic_power_production_current": {
"name": "Solar power"
},
"photovoltaic_energy_production_today": {
"name": "Solar energy production today"
},
"photovoltaic_energy_production_this_week": {
"name": "Solar energy production this week"
},
"photovoltaic_energy_production_this_month": {
"name": "Solar energy production this month"
},
"photovoltaic_energy_production_this_year": {
"name": "Solar energy production this year"
},
"photovoltaic_energy_production_total": {
"name": "Solar energy production total"
},
"photovoltaic_status": {
"name": "Solar state",
"state": {
"ready": "Standby",
"production": "Producing"
}

And a second cluster with solar:

"solar_power_production_today": {
"name": "Solar energy production today"
},
"solar_power_production_this_week": {
"name": "Solar energy production this week"
},
"solar_power_production_this_month": {
"name": "Solar energy production this month"
},
"solar_power_production_this_year": {
"name": "Solar energy production this year"

Given that the latter are in a block that also contains different solar temperature entities these seem to be for thermal solar systems, while the first block is for electrical / PV systems.

Thus the first group above should have their entity names changed from "Solar …" to "PV …".

@CFenner
Copy link
Contributor Author

CFenner commented Oct 20, 2024

@NoRi2909 let's fix this in a dedicated PR / discuss in an issue. I think there are solar power and solar thermal devices in vicare.. maybe it's related to that..

@CFenner CFenner deleted the consumption branch October 20, 2024 17:23
10100011 pushed a commit to 10100011/core that referenced this pull request Oct 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants