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

Use sensor device class for graph and precision #18099

Merged
merged 4 commits into from
Oct 23, 2023

Conversation

piitaya
Copy link
Member

@piitaya piitaya commented Oct 2, 2023

Proposed change

Use sensor device class to determine if the state is numeric or not.
This fixes 2 issues :

  • in entity settings : the precision could be selected for custom devices classes even if the state wasn't numeric
  • in history : sensors with aqi, ph or power_factor device class and without state_class doesn't render as a graph (core issue: "AQI" & "pH" device classes: cannot specify "" uom #18439)

This PR also removes the # unit for counter, input_number, number to be consistent with other domains.

Needs home-assistant/core#101257

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

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

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

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

@jbouwh
Copy link
Contributor

jbouwh commented Oct 3, 2023

The code works correctly with the related backend PR.

Comment on lines 436 to 438
const numericStateFromHistory =
!currentState &&
stateInfo.find((state) => state.a && attributesHaveUnits(state.a));
stateInfo.find((state) => state.a && isNumericFromAttributes(state.a));
Copy link
Member

Choose a reason for hiding this comment

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

We can move this in an else when isNumeric is false?

Copy link
Member

@bramkragten bramkragten Oct 5, 2023

Choose a reason for hiding this comment

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

Shouldnt we use the same checks here as we do below for isNumeric? So also use the domain and isNumericSensorEntity?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have device_class in the history.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then I think we should just do a check on the domain before we go through the historic states

@frenck frenck added backend merged The backend PR for this frontend PR has been merged and removed wait for backend labels Oct 6, 2023
@frenck
Copy link
Member

frenck commented Oct 6, 2023

Backend has been merged.

domain === "sensor" &&
isNumericSensorEntity(currentState, sensorNumericalDeviceClasses));

if (isNumeric) {
Copy link
Member

Choose a reason for hiding this comment

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

isNumeric can also be true if there is no state, but the domain is a numeric domain. We should still use the numericStateFromHistory then if available

Copy link
Member

Choose a reason for hiding this comment

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

I guess in that case for now it will be correct, because it will not have a UOM and will use the correct BLANK_UNIT, but I think that is pretty error prone...?

I would prefer:

      unit = currentState?.attributes.unit_of_measurement || numericStateFromHistory?.a.unit_of_measurement || BLANK_UNIT;

src/data/history.ts Outdated Show resolved Hide resolved
bramkragten
bramkragten previously approved these changes Oct 18, 2023
@piitaya piitaya enabled auto-merge (squash) October 18, 2023 13:26
@piitaya piitaya disabled auto-merge October 18, 2023 13:27
@piitaya piitaya force-pushed the sensor_numeric_device_class branch from 69648cb to 521ff53 Compare October 23, 2023 09:26
@piitaya piitaya requested a review from bramkragten October 23, 2023 09:26
@bramkragten bramkragten merged commit aeaf091 into dev Oct 23, 2023
12 checks passed
@bramkragten bramkragten deleted the sensor_numeric_device_class branch October 23, 2023 13:01
@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backend merged The backend PR for this frontend PR has been merged cla-signed hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants