Skip to content

Fix incorrect power factor device class usage in Fronius#84374

Merged
frenck merged 1 commit into
devfrom
frenck-2022-2305
Dec 21, 2022
Merged

Fix incorrect power factor device class usage in Fronius#84374
frenck merged 1 commit into
devfrom
frenck-2022-2305

Conversation

@frenck
Copy link
Copy Markdown
Member

@frenck frenck commented Dec 21, 2022

Proposed change

Fixes an case of incorrect device class usage in the Fronius integration.

The power factor sensors have the power_factor device class, but do not provide a valid unit for that device class. The only valid unit is %, but it isn't a percentage that these sensors provide in this integration.

Therefore, I've removed the device class in this PR.

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

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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:

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.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @nielstron, @farmio, mind taking a look at this pull request as it has been labeled with an integration (fronius) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of fronius can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign fronius Removes the current integration label and assignees on the issue, add the integration domain after the command.

@farmio
Copy link
Copy Markdown
Member

farmio commented Dec 21, 2022

This sensor returns a float 0..1. Should we instead multiply it by 100 and add % as unit?
I think it would be nicer to have unit and device class set.

Afaic power factor is often written as 0..1 without unit (eg. on power supplys).
From Wikipedia I read

Since the units are consistent, the power factor is by definition a dimensionless number between -1 and 1.

https://en.wikipedia.org/wiki/Power_factor

@frenck
Copy link
Copy Markdown
Member Author

frenck commented Dec 21, 2022

Should we instead multiply it by 100 and add % as unit?

🤷 That was not in my scope; I just spotted this bug.

Afaic power factor is often written as 0..1 without unit (eg. on power supply).

Home Assistant expects a percentage (as documented).

Copy link
Copy Markdown
Member

@farmio farmio left a comment

Choose a reason for hiding this comment

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

🤷

@epenet
Copy link
Copy Markdown
Contributor

epenet commented Dec 21, 2022

@farmio I had a quick look at the code and it doesn't seem so easy to adjust the code and multiply by 100.
Do you want to provide a superseeding PR? Or a follow-up PR to reinstate the device class and the multiplication factor.

@farmio
Copy link
Copy Markdown
Member

farmio commented Dec 21, 2022

Yes sure, I can do a follow-up. I'd just add a lambda to the entity descriptions.

@epenet If you ask me, the correct way would be to support dimensionless numbers for power factor in HA. But you are the unit expert here.

@frenck
Copy link
Copy Markdown
Member Author

frenck commented Dec 21, 2022

If you ask me, the correct way would be to support dimensionless numbers for power factor in HA.

That is an architecture discussion, not a PR discussion.

@frenck frenck merged commit fa55ba7 into dev Dec 21, 2022
@frenck frenck deleted the frenck-2022-2305 branch December 21, 2022 21:43
@github-actions github-actions Bot locked and limited conversation to collaborators Dec 22, 2022
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