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: connection_entity_id and add_entities/subtract_entities #199

Conversation

madsciencetist
Copy link
Contributor

  • Fix connection_entity_id
    • If a connection_entity_id was specified, it was not added to the list of entities for which to retrieve the state. It seems that connection_entity_id only happened to work if that entity_id was also used elsewhere.
  • Fix add_entities/subtract_entities normalization with unit conversion
    • When converting energy units, all the conversions take place in energy.ts, and the results can be directly added together. However, when _getMemoizedState was adding the values of add_entities, it didn't realize that the entities to add were already the correct values, and it erroneously normalized them. Now, the same "this has already been converted" logic used for the main entity state is also used for the add_entity state.

If a connection_entity_id was specified, it was not added to the list of
entities for which to retrieve the state. It seems that
connection_entity_id only happened to work if that entity_id was also
used elsewhere.
When converting energy units, all the conversions take place in
energy.ts, and the results can be directly added together. However, when
_getMemoizedState was adding the values of add_entities, it didn't
realize that the entities to add were already the correct values, and it
erroneously normalized them. Now, the same "this has already been
converted" logic used for the main entity state is also used for the
add_entity state.
@MindFreeze
Copy link
Owner

The connection_entity_id fix looks good but the other one is tricky and I want to test it first. Hopefully I'll have time for it this weekend.
Thanks for the fixes

@MindFreeze MindFreeze changed the title Fix connection_entity_id and add_entities/subtract_entities fix: connection_entity_id and add_entities/subtract_entities May 23, 2024
Copy link
Owner

@MindFreeze MindFreeze left a comment

Choose a reason for hiding this comment

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

Please explain where the conversion happens because I don't see it. Btw I want to remove this._getUnitOfMeasurement and move this logic to energy.ts so the unit and state that come into the chart are coupled. But I can do this refactoring later. For now I just need to understand the problem you found

src/chart.ts Outdated Show resolved Hide resolved
@madsciencetist
Copy link
Contributor Author

I agree that _getUnitOfMeasurement() is a hack and that the solution is to refactor such that units are always tracked with the values. It got tricky in that original unit conversion PR because kWh -> gCO2 happens in getStatistics(), but then gCO2 -> kgCO2 happens in normalizeStateValue(), which has no real tracking of the unit of the value it is given.

The new problem I found is as follows. Assume the following configuration:

convert_units_to: monetary
monetary_unit: USD
electricity_price: 0.5
sections:
  - entities:
      - entity_id: sensor.energy_1         # 100 kWh
        add_entities:
          - entity_id: sensor.energy_2     #  20 kWh

We want the bar to evaluate to $60. Instead, this happened:

  • getStatistics() returns $50 for energy_1 and $10 for energy_2 (correct)
  • Inside _getMemoizedState():
    • entity.attributes.unit_of_measurement is kWh but _getUnitOfMeasurement() sees that config.convert_units_to is monetary and thus sets unit_of_measurement = '' to pass to normalizeStateValue(). Then normalizeStateValue('', 50, '') returns 50 (dollars; correct)
    • But now for add_entities:
      • subEntity.state is 10 (dollars; correct)
      • subEntity.attributes.unit_of_measurement is kWh, which is wrong because we converted to dollars, but unlike above we now pass this wrong unit into normalizeStateValue
      • normalizeStateValue('', 10, 'kWh') sees that k and multiplies by 1000, returning 10000 (incorrect)
    • The bar evaluates to $50 + 10000 = $10050

@MindFreeze
Copy link
Owner

I understand now. The problem is that we don't use this._getUnitOfMeasurement there.

What confused me is that you also changed subEntity.attributes.unit_of_measurement || unit_of_measurement to entityConf.unit_of_measurement || subEntity.attributes.unit_of_measurement. Basically reversed the order, which I think is a problem. Can you change it to this._getUnitOfMeasurement(subEntity.attributes.unit_of_measurement || unit_of_measurement) so the order is preserved?

@madsciencetist
Copy link
Contributor Author

Yes, that makes sense

@MindFreeze MindFreeze self-requested a review May 31, 2024 08:08
@MindFreeze MindFreeze merged commit d2c987d into MindFreeze:master May 31, 2024
1 check passed
@madsciencetist madsciencetist deleted the fix_connection_entities_and_added_entities branch May 31, 2024 12:42
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.

2 participants