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

fix home assistant complaining GBP/kWH is an invalid currency #77

Closed
wants to merge 1 commit into from

Conversation

si458
Copy link

@si458 si458 commented Nov 9, 2021

Description

fix home assistant complaining GBP/kWH is an invalid currency

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

fix home assistant complaing GBP/kWH is an invalid currency
@ColinRobbins
Copy link
Contributor

Thanks - I thought I had fixed that.

@si458
Copy link
Author

si458 commented Nov 9, 2021

@ColinRobbins this is a different line 😆
you fixed the elec tariff standing, this is for elec tariff rate

@si458
Copy link
Author

si458 commented Nov 9, 2021

@ColinRobbins if you dont change GBP/kWh to GBP
the web ui in home assistant complains in the console saying
RangeError: invalid currency code in NumberFormat(): GBP/kWh
so this needs to be set as GBP

@ColinRobbins
Copy link
Contributor

ColinRobbins commented Nov 9, 2021

I don’t think this is right.
The Energy integration needs the unit rate to by GBP/kWh, otherwise it won’t accept it.
The standing rate is GBP.

@ColinRobbins
Copy link
Contributor

Which page / card are you looking at when you see the console error? I’m not seeing anything in my console!

@si458
Copy link
Author

si458 commented Nov 9, 2021

BEFORE:
Screenshot 2021-11-09 at 18 35 52

AFTER:
Screenshot 2021-11-09 at 18 42 16

CONSOLE ERRORS BEFORE PATCH:
Screenshot 2021-11-09 at 18 37 31

EDIT: uploading right picture does help haha

this is just a standard entities added to my home dashboard

type: entities
entities:
  - sensor.electric_consumption_today
  - sensor.electric_consumption_year
  - sensor.electric_cost_today
  - sensor.electric_tariff_rate
  - sensor.electric_tariff_standing

EDIT AGAIN: i dont believe the kWh is needed as thats being used/calculated in the energy tab and not on the standard dashboards

@ColinRobbins
Copy link
Contributor

ColinRobbins commented Nov 9, 2021

Ah, OK.
I think that probably needs a different fix then.
I think GBP/kWh is correct.
The issue is the device class being DEVICE_CLASS_MONETARY.

@ColinRobbins
Copy link
Contributor

Sorry, device class, not state class - corrected post above.
No sure what the “correct” value should be - looking…

@ColinRobbins
Copy link
Contributor

Not sure I can see anything better than “DEVICE_CLASS_ENERGY”

Copy link
Contributor

@ColinRobbins ColinRobbins left a comment

Choose a reason for hiding this comment

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

Rather than changing line 336, I suggest trying adding the following at line 332:

    @property
    def device_class(self) -> str:
        """Return None as the device class."""
        return None

I've tried it on my test systems, and it seems to solve the issue. I think anything other that None will case issues somewhere.

@si458
Copy link
Author

si458 commented Nov 9, 2021

@ColinRobbins im keeping my pi running for now with this patch set as GBP
and ill see if any issues happen over the next few days
none so fair, but case of wait n see 🤞

@ColinRobbins
Copy link
Contributor

With it set to GBP, you will not be able to add it as an entity with current price in the energy dashboard. It will probably work if set up, but would not work if you remove it and try to add it back. The energy dashboard needs GBP/kWh.

@si458
Copy link
Author

si458 commented Nov 9, 2021

With it set to GBP, you will not be able to add it as an entity with current price in the energy dashboard. It will probably work if set up, but would not work if you remove it and try to add it back. The energy dashboard needs GBP/kWh.

surely the entity with current price option would be sensor.electric_cost_today and not sensor.electric_tariff_rate ?

@si458
Copy link
Author

si458 commented Nov 9, 2021

i currently have mine set to use an entity tracking the total costs and use sensor.electric_cost_today and no issues here

@ColinRobbins
Copy link
Contributor

If you look at the code in https://github.com/home-assistant/core/blob/dev/homeassistant/components/energy/sensor.py About line 300, the integration is expecting the price to end in /wh, /mwh or by default /kWh.
My proposed alternative fix, resolves the issue you reported, and meets the expectation of the energy dashboard.

@ColinRobbins
Copy link
Contributor

The energy dashboard documentation gives an example in ‘USD/kWh’, not ‘USD’.

@si458
Copy link
Author

si458 commented Nov 9, 2021

@ColinRobbins if you look at the developer docs too for the sensor entites it says the values need to be either kWh or a GBP (currency) for either energy or monetary so i think this fix is good enough :)
https://developers.home-assistant.io/docs/core/entity/sensor#available-device-classes

@ColinRobbins
Copy link
Contributor

We’ll have to agree to differ, and let HandyHat decide which approach to take.

@ColinRobbins
Copy link
Contributor

Revisiting this, I think if you use “rate” in the energy dashboard with GBP as the unit, this PR will lead to the error…

Unexpected unit of measurement

Translation Error: The intl string context variable "currency" was not provided to the string "The following entities do not have the expected units of measurement ''{currency}/kWh'' or ''{currency}/Wh'':"

Hence why I believe the current units are correct at GBP/kWh.

@si458
Copy link
Author

si458 commented Nov 22, 2021

Revisiting this, I think if you use “rate” in the energy dashboard with GBP as the unit, this PR will lead to the error…

Unexpected unit of measurement

Translation Error: The intl string context variable "currency" was not provided to the string "The following entities do not have the expected units of measurement ''{currency}/kWh'' or ''{currency}/Wh'':"

Hence why I believe the current units are correct at GBP/kWh.

Where are you seeing this error? I've had no issues/no errors with this pr on my pi for awhile now?

@ColinRobbins
Copy link
Contributor

This error is in the HA Logs.
It occurs if you use the “rate” sensor, with this PR, in the Energy integration to supply the current cost of energy.

@si458
Copy link
Author

si458 commented Nov 22, 2021

How does ur elec look as no issues here at all? Screenshot_20211122-183416_Home Assistant.jpg

@ColinRobbins
Copy link
Contributor

The error occurs if you use the current tariff rate sensor for “use an entity with the current price”.
( I know using the cost sensor is better, but the using the tariff sensor should not result in an error).

@robertalexa
Copy link

Don't mean to intervene but I am with @ColinRobbins on this one.

Here is my reasoning:

  • yesterday i have played with this, tried using use an entity with the current price and tried it with Electric Cost (Today) which is basically GBP (unit of measurement) and that indeed throws an error.

Revisiting this, I think if you use “rate” in the energy dashboard with GBP as the unit, this PR will lead to the error…

Unexpected unit of measurement

Translation Error: The intl string context variable "currency" was not provided to the string "The following entities do not have the expected units of measurement ''{currency}/kWh'' or ''{currency}/Wh'':"

Hence why I believe the current units are correct at GBP/kWh.

So i can confirm changing tariff rate (the entity refered in this PR) to GBP and using it for Energy will error later.

  • I agree that the tariff rate unit of measurement should be GBP/kWh, and not GBP. This is NOT a sum of money (currency) but rather a unit of measurement, like fuel consumption. Fuel consumption 5L makes no sense if you do not match it against a distance e.g. 100km. (same logic applies to miles per galon obviously). Same things is when you go shop for anything, the price for meat is £/kg, otherwise how do you know how much you are getting for your £?

  • So that being said, the approach should be to change the device class rather than change the unit of measurement, as already suggested by Colin

Rather than changing line 336, I suggest trying adding the following at line 332:

    @property
    def device_class(self) -> str:
        """Return None as the device class."""
        return None

I've tried it on my test systems, and it seems to solve the issue. I think anything other that None will case issues somewhere.

PS: I have landed here by basically discovering the same error in the console, see here #88 (comment)

This was referenced Feb 9, 2022
@HandyHat
Copy link
Owner

HandyHat commented Feb 22, 2022

Thank you for the insightful discussion here everyone!
I'm going to close this in favour of @ColinRobbins' solution of unsetting the device class (#138), as I think that makes more sense

@HandyHat HandyHat closed this Feb 22, 2022
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.

4 participants