Add device classes for electrical measurement #36800

fabiocastagnino · 2020-06-14T18:11:41Z

Proposed change

I want to add 4 new device_classs, to handle the electrical measurement

fixes home-assistant/architecture#397

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)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

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
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

homeassistant · 2020-06-14T18:11:44Z

Hi @fabiocastagnino,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

MartinHjelmare

This is a good idea, I think. We have a requirement that changes to our entity model needs approval in an architecture issue.

Please open an issue:
https://developers.home-assistant.io/docs/core/entity#changing-the-entity-model

Also extend our developer docs here:
https://developers.home-assistant.io/docs/core/entity/sensor#available-device-classes

fabiocastagnino · 2020-06-15T16:53:18Z

Hi,
I've:

opened the issue: Add device classes for electrical measurement architecture#397
extend developer docs: Add device classes for electrical measurement developers.home-assistant#579
updated docs: Add device classes for electrical measurement home-assistant.io#13756

Adminiuga · 2020-06-15T17:41:08Z

Does DEVICE_CLASS_POWER_FACTOR has to be it's own device class? Isn't "power factor" a characteristic inherently associated with DEVICE_CLASS_POWER sensors?
in other words, if we don't have a DEVICE_CLASS_HUMIDITY_RELATIVE and DEVICE_CLASS_HUMIDITY_ABSOLUTE, what is the compelling case to have DEVICE_CLASS_POWER_FACTOR?

fabiocastagnino · 2020-06-15T18:42:55Z

Your observation it's correct, but to have a correct power measurement you need both information.
You suggest to put all measurement of the "Power Triangle" (Real Power, Relative Power, Apparent Power, Power Factor) under DEVICE_CLASS_POWER ?
In this case I'll review the PR.

Adminiuga · 2020-06-15T20:12:07Z

Just asking :) I think this is fine, cause even we don't have the DEVICE_CLASS_HUMIDITY_ABSOLUTE/RELATIVE, there's DEVICE_CLASS_DOOR and DEVICE_CLASS_GARAGE_DOOR and DEVICE_CLASS_GARAGE

MartinHjelmare · 2020-06-15T21:50:22Z

I think it's good to keep power factor separate.

homeassistant/components/sensor/__init__.py

MartinHjelmare

Please keep all lists sorted 🔡

homeassistant/components/sensor/device_condition.py

tests/testing_config/custom_components/test/sensor.py

MartinHjelmare · 2020-06-16T19:26:37Z

@bramkragten do we need to add anything in the frontend when adding a new sensor device class?

bramkragten · 2020-06-18T16:36:38Z

If the sensors will just have a unit of measurement, I think we don't have to change anything.

MartinHjelmare · 2020-06-18T16:40:44Z

Do we want to set a specific default icon per class?

bramkragten · 2020-06-18T17:05:59Z

Yes, you can set one https://github.com/home-assistant/frontend/blob/dev/src/common/entity/sensor_icon.ts#L6-L7

fabiocastagnino · 2020-06-18T17:54:49Z

Yes, we can use:
current: "hass:flash",
energy: "hass:flash",
power_factor: "hass:flash",,
voltage: "hass:current-ac",

PR: home-assistant/frontend#6193
locally work

fabiocastagnino · 2020-06-18T19:19:12Z

PR: home-assistant/frontend#6193 ready
All PR locally works:

MartinHjelmare

Some small comments. Otherwise looks good.

homeassistant/components/sensor/translations/it.json

tests/testing_config/custom_components/test/sensor.py

homeassistant/components/sensor/device_condition.py

homeassistant · 2020-06-19T14:26:13Z

Hi @fabiocastagninohorsa,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

tests/testing_config/custom_components/test/sensor.py

MartinHjelmare

Looks good!

dgomes

Someone needs to restart the CI

MartinHjelmare · 2020-07-10T12:50:24Z

If the branch is rebased the new CI should run automatically.

(cherry picked from commit 2409fe19ed43bef568a0cca826652867d3a2d71a)

fabiocastagnino · 2020-07-10T16:18:59Z

I've squashed and rebased the branch, waiting for CI

fabiocastagnino · 2020-07-10T18:44:42Z

2 tests (test_get_conditions, test_get_triggers) fail for the same reason.
Tests load power_factor sensor without unit of measurement, as configured, but device_condition/device_trigger seems that it is required
This can fix the tests:

I'm not sure about the fix (in the test or in the device_condition/device_trigger).
What is the expected behaviour in the original design?

MartinHjelmare · 2020-07-14T08:13:54Z

It seems the sensor device automations use the unit of measurement in the automation description.

@balloob what do you suggest?

MartinHjelmare · 2020-08-07T13:56:26Z

Let's set % as unit of measurement for power factor device class so we can unblock this.

fabiocastagnino · 2020-08-07T20:07:45Z

ok, tomorrow I'll push the change in all PR

fabiocastagnino · 2020-08-08T12:33:13Z

ready

MartinHjelmare

Thanks!

probot-home-assistant bot added core integration: sensor new-feature labels Jun 14, 2020

homeassistant added cla-needed cla-signed and removed cla-needed labels Jun 14, 2020

This was referenced Jun 14, 2020

Add new sensor device classes home-assistant/home-assistant.io#13755

Closed

Add device classes for electrical measurement home-assistant/home-assistant.io#13756

Merged

MartinHjelmare changed the title ~~added device classes for electrical measurement~~ Add device classes for electrical measurement Jun 14, 2020

MartinHjelmare reviewed Jun 14, 2020

View reviewed changes

This was referenced Jun 15, 2020

Add device classes for electrical measurement home-assistant/architecture#397

Closed

Add device classes for electrical measurement home-assistant/developers.home-assistant#579

Merged

fabiocastagnino marked this pull request as ready for review June 15, 2020 17:27

MartinHjelmare reviewed Jun 15, 2020

View reviewed changes

homeassistant/components/sensor/__init__.py Outdated Show resolved Hide resolved

MartinHjelmare reviewed Jun 16, 2020

View reviewed changes

homeassistant/components/sensor/device_condition.py Outdated Show resolved Hide resolved

tests/testing_config/custom_components/test/sensor.py Outdated Show resolved Hide resolved

fabiocastagnino mentioned this pull request Jun 18, 2020

Add icons for new sensor device classes home-assistant/frontend#6193

Merged

9 tasks

MartinHjelmare reviewed Jun 19, 2020

View reviewed changes

homeassistant/components/sensor/translations/it.json Outdated Show resolved Hide resolved

tests/testing_config/custom_components/test/sensor.py Outdated Show resolved Hide resolved

homeassistant/components/sensor/device_condition.py Show resolved Hide resolved

homeassistant added cla-needed and removed cla-needed labels Jun 19, 2020

MartinHjelmare reviewed Jun 19, 2020

View reviewed changes

tests/testing_config/custom_components/test/sensor.py Outdated Show resolved Hide resolved

MartinHjelmare approved these changes Jun 19, 2020

View reviewed changes

dgomes approved these changes Jul 10, 2020

View reviewed changes

added device classes for electrical measurement

5328697

(cherry picked from commit 2409fe19ed43bef568a0cca826652867d3a2d71a)

fabiocastagnino added 2 commits August 8, 2020 14:08

upadte power factor unit (%)

55cecb1

update power factor unit (%)

6850b2b

MartinHjelmare approved these changes Aug 8, 2020

View reviewed changes

MartinHjelmare merged commit 02d572a into home-assistant:dev Aug 8, 2020

This was referenced Aug 9, 2020

Add energy device class to Toon sensors #38686

Merged

Add current device class to WLED current sensor #38687

Merged

mletenay mentioned this pull request Oct 8, 2020

Cannot import name 'DEVICE_CLASS_CURRENT' mletenay/home-assistant-goodwe-inverter#12

Closed

Cougar mentioned this pull request Dec 8, 2020

Correct power factor range (and unit) home-assistant/architecture#469

Closed

fsaris mentioned this pull request Aug 22, 2021

Report power factor as persentage wimb0/home-assistant-saj-r5-modbus#51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add device classes for electrical measurement #36800

Add device classes for electrical measurement #36800

fabiocastagnino commented Jun 14, 2020 •

edited by MartinHjelmare

Loading

homeassistant commented Jun 14, 2020

MartinHjelmare left a comment

fabiocastagnino commented Jun 15, 2020

Adminiuga commented Jun 15, 2020 •

edited

Loading

fabiocastagnino commented Jun 15, 2020

Adminiuga commented Jun 15, 2020 •

edited

Loading

MartinHjelmare commented Jun 15, 2020

MartinHjelmare left a comment

MartinHjelmare commented Jun 16, 2020

bramkragten commented Jun 18, 2020

MartinHjelmare commented Jun 18, 2020

bramkragten commented Jun 18, 2020

fabiocastagnino commented Jun 18, 2020 •

edited

Loading

fabiocastagnino commented Jun 18, 2020

MartinHjelmare left a comment

homeassistant commented Jun 19, 2020

MartinHjelmare left a comment

dgomes left a comment

MartinHjelmare commented Jul 10, 2020

fabiocastagnino commented Jul 10, 2020

fabiocastagnino commented Jul 10, 2020

MartinHjelmare commented Jul 14, 2020

MartinHjelmare commented Aug 7, 2020

fabiocastagnino commented Aug 7, 2020

fabiocastagnino commented Aug 8, 2020

MartinHjelmare left a comment

Add device classes for electrical measurement #36800

Add device classes for electrical measurement #36800

Conversation

fabiocastagnino commented Jun 14, 2020 • edited by MartinHjelmare Loading

Proposed change

Type of change

Checklist

homeassistant commented Jun 14, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

fabiocastagnino commented Jun 15, 2020

Adminiuga commented Jun 15, 2020 • edited Loading

fabiocastagnino commented Jun 15, 2020

Adminiuga commented Jun 15, 2020 • edited Loading

MartinHjelmare commented Jun 15, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Jun 16, 2020

bramkragten commented Jun 18, 2020

MartinHjelmare commented Jun 18, 2020

bramkragten commented Jun 18, 2020

fabiocastagnino commented Jun 18, 2020 • edited Loading

fabiocastagnino commented Jun 18, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

homeassistant commented Jun 19, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

dgomes left a comment

Choose a reason for hiding this comment

MartinHjelmare commented Jul 10, 2020

fabiocastagnino commented Jul 10, 2020

fabiocastagnino commented Jul 10, 2020

MartinHjelmare commented Jul 14, 2020

MartinHjelmare commented Aug 7, 2020

fabiocastagnino commented Aug 7, 2020

fabiocastagnino commented Aug 8, 2020

MartinHjelmare left a comment

Choose a reason for hiding this comment

fabiocastagnino commented Jun 14, 2020 •

edited by MartinHjelmare

Loading

Adminiuga commented Jun 15, 2020 •

edited

Loading

Adminiuga commented Jun 15, 2020 •

edited

Loading

fabiocastagnino commented Jun 18, 2020 •

edited

Loading