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

Full-featured sensors for buying and selling #72

Merged
merged 5 commits into from
Dec 2, 2024

Conversation

prehor
Copy link
Contributor

@prehor prehor commented Nov 13, 2024

  • Full set of sensors for buying and selling electricity and gas, including hourly prices, hour prices order, cheapest hours and most expensive hours
  • Add an hour variable to the buying and selling price templates.
  • More entity modernization
  • Fixed localized entity ids

Example buying template with EG.D Distribuce distribution tariff sensor:

{% set distribution_tariff = state_attr('binary_sensor.distribution_tariff', 'HDO_HOURLY')[as_local(hour)] | default(0) | float %}
{% set spot_fee = 0.35 %}
{% set vat = 1.21 %}
{{ (value + spot_fee + distribution_tariff) * vat }}

Fixies:

@kukulich
Copy link
Contributor

kukulich commented Nov 14, 2024

You've made changes in _attr_unique_id and it will be massive BC break.

Btw is there a reason why you made so many changes not relevant to the topic in one commit?

@kukulich
Copy link
Contributor

self.entity_id = self._attr_unique_id is also a BC break. And it looks it may also break the possibility to customize entity ID in UI.

@prehor
Copy link
Contributor Author

prehor commented Nov 14, 2024

You've made changes in _attr_unique_id and it will be massive BC break.

I have checked many times that generated entity_id and uniqe_id are same as version 0.5.2. I may take a look at it over the weekend and double check to be sure.

Btw is there a reason why you made so many changes not relevant to the topic in one commit?

I was developing it against version 0.5.2 and the change is huge. Unfortunately, before I got to PR, @rnovacek merged your entities modernization and when I did the rebase, I had to rework the whole thing. During rework I had to solve a number of issues, including the one you address in #74, and also that the entity_ids were generated based on the localized entity names.

@prehor
Copy link
Contributor Author

prehor commented Nov 14, 2024

self.entity_id = self._attr_unique_id is also a BC break. And it looks it may also break the possibility to customize entity ID in UI.

Without it, entity_ids are generated from localized entity names :-(

@kukulich
Copy link
Contributor

self.entity_id = self._attr_unique_id is also a BC break. And it looks it may also break the possibility to customize entity ID in UI.

Without it, entity_ids are generated from localized entity names :-(

And that’s absolutely normal and expected behaviour.

@kukulich
Copy link
Contributor

I have checked many times that generated entity_id and uniqe_id are same as version 0.5.2. I may take a look at it over the weekend and double check to be sure.

You’ve added trade.lower() to unique id so it does not look the ids are same.

@prehor prehor marked this pull request as draft November 15, 2024 08:41
@prehor
Copy link
Contributor Author

prehor commented Nov 15, 2024

I have checked many times that generated entity_id and uniqe_id are same as version 0.5.2. I may take a look at it over the weekend and double check to be sure.

You’ve added trade.lower() to unique id so it does not look the ids are same.

This is because both version 0.6.4 and your modified entities use lowercase unique_ids.

@kukulich
Copy link
Contributor

This is because both version 0.6.4 and your modified entities use lowercase unique_ids.

I don't understand. However you've modified unique_id (see https://github.com/rnovacek/homeassistant_cz_energy_spot_prices/pull/72/files#diff-e513f41036a445059cb6bcd3e04d77008230c70ae2314434c4c9eb3ddc6408bcR107) and it's BC break.

Copy link
Owner

@rnovacek rnovacek left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Couple of nitpicks, nothing serious. We just need to make sure we maintain compatibility with older releases, could you double check that?

@rnovacek
Copy link
Owner

This is because both version 0.6.4 and your modified entities use lowercase unique_ids.

I don't understand. However you've modified unique_id (see https://github.com/rnovacek/homeassistant_cz_energy_spot_prices/pull/72/files#diff-e513f41036a445059cb6bcd3e04d77008230c70ae2314434c4c9eb3ddc6408bcR107) and it's BC break.

I think the changes to unique_ids are only in unreleased binary_sensors.py file, so should be fine. In the sensors.py file it looks alright, just replacing _spot_ with _{trade.lower()}_ which is the same.

@kukulich
Copy link
Contributor

@rnovacek

I think the changes to unique_ids are only in unreleased binary_sensors.py file, so should be fine. In the sensors.py file it looks alright, just replacing _spot_ with _{trade.lower()}_ which is the same.

It will break my installation but ok I understand your position.

However I think this is still wrong: https://github.com/rnovacek/homeassistant_cz_energy_spot_prices/pull/72/files#diff-e513f41036a445059cb6bcd3e04d77008230c70ae2314434c4c9eb3ddc6408bcR213

@prehor prehor force-pushed the feat-buy-sell-proces branch from 0590d95 to bb18eed Compare November 21, 2024 18:15
@prehor
Copy link
Contributor Author

prehor commented Nov 21, 2024

self.entity_id = self._attr_unique_id is also a BC break.

Quite the opposite. Entities will retain their original English entity_id used up to version 0.6.4 and will not be constructed from the localized entity names, which are more descriptive, including the English ones. Other integrations behave the same way, for example Battery-Notes.

And it looks it may also break the possibility to customize entity ID in UI.

No. The entity_id attribute is used only during initialization of the sensor and is stored in the entity registry. If the entity_id attribute is not specified, the entity_id is constructed from the localized name attribute. When entity_id is changed in the UI, the new value is saved to the entity registry.

I would leave the decision to @rnovacek on whether to localize the entity_id or not.

@prehor
Copy link
Contributor Author

prehor commented Nov 21, 2024

This is because both version 0.6.4 and your modified entities use lowercase unique_ids.

I don't understand. However you've modified unique_id (see https://github.com/rnovacek/homeassistant_cz_energy_spot_prices/pull/72/files#diff-e513f41036a445059cb6bcd3e04d77008230c70ae2314434c4c9eb3ddc6408bcR107) and it's BC break.

Let me try to explain it better. Each sensor class generates three sensors for spot prices, purchase prices, and selling prices. The prices used in the sensor are determined by the trade parameter, which can have vales Spot, Buy, or Sell. Therefore, the unique_id is generated using the _{ trade.lower() }_ to ensure they are distinct. Sensors for spot prices will include _spot_ in the unique_id, as in version 0.6.4. Newly created sensors will include _buy_ or _sell_. The same applies their entity_id attribute, which contains _spot_, _buy_, or _sell_, depending on which prices it represents.

@kukulich
Copy link
Contributor

Quite the opposite. Entities will retain their original English entity_id used up to version 0.6.4 and will not be constructed from the localized entity names, which are more descriptive, including the English ones.

I had installed the integration before my PR with translations and I still have english entity ids. Only new user should have the new localized entity ids because there are generated only for new entities.

Other integrations behave the same way, for example Battery-Notes.

Native HA integrations mostly don't because localized entity ids are expected - there are more user-friendly.

No. The entity_id attribute is used only during initialization of the sensor and is stored in the entity registry. If the entity_id attribute is not specified, the entity_id is constructed from the localized name attribute. When entity_id is changed in the UI, the new value is saved to the entity registry.

I've checked the HA core and you are probably right because there are some integration (minority of integrations) that use the same pattern. I have to admit I was not expected this and I was also wrong.

@kukulich
Copy link
Contributor

This is because both version 0.6.4 and your modified entities use lowercase unique_ids.

I don't understand. However you've modified unique_id (see https://github.com/rnovacek/homeassistant_cz_energy_spot_prices/pull/72/files#diff-e513f41036a445059cb6bcd3e04d77008230c70ae2314434c4c9eb3ddc6408bcR107) and it's BC break.

Let me try to explain it better. Each sensor class generates three sensors for spot prices, purchase prices, and selling prices. The prices used in the sensor are determined by the trade parameter, which can have vales Spot, Buy, or Sell. Therefore, the unique_id is generated using the _{ trade.lower() }_ to ensure they are distinct. Sensors for spot prices will include _spot_ in the unique_id, as in version 0.6.4. Newly created sensors will include _buy_ or _sell_. The same applies their entity_id attribute, which contains _spot_, _buy_, or _sell_, depending on which prices it represents.

I understand your reasons. I'm just not happy you've changed ids for binary sensors because it's BC break - yes, maybe only for me because I use main branch with all my merged PRs.

@prehor
Copy link
Contributor Author

prehor commented Nov 21, 2024

@rnovacek

I think the changes to unique_ids are only in unreleased binary_sensors.py file, so should be fine. In the sensors.py file it looks alright, just replacing _spot_ with _{trade.lower()}_ which is the same.

It will break my installation but ok I understand your position.

I looked into the HEAD, and both the original sensors and the new binary sensors use only lowercase letters in the unique_id, so your installation should not be broken.

@kukulich
Copy link
Contributor

kukulich commented Nov 21, 2024

@rnovacek

I think the changes to unique_ids are only in unreleased binary_sensors.py file, so should be fine. In the sensors.py file it looks alright, just replacing _spot_ with _{trade.lower()}_ which is the same.

It will break my installation but ok I understand your position.

I looked into the HEAD, and both the original sensors and the new binary sensors use only lowercase letters in the unique_id, so your installation should not be broken.

The problem is that you've changed the id.

@prehor
Copy link
Contributor Author

prehor commented Nov 21, 2024

@rnovacek

I think the changes to unique_ids are only in unreleased binary_sensors.py file, so should be fine. In the sensors.py file it looks alright, just replacing _spot_ with _{trade.lower()}_ which is the same.

It will break my installation but ok I understand your position.

I looked into the HEAD, and both the original sensors and the new binary sensors use only lowercase letters in the unique_id, so your installation should not be broken.

The problem is that you've changed the id.

OK, I hope I finally understand what bothers you. The issue isn't with using { trade.lower() } as I initially thought, but with the prefix binary_sensor used in the unique_id. Your binary sensors have unique_ids set to sensor.spot_electricity_is_cheapest, sensor.spot_electricity_is_cheapest_{self.hours}_hours_block, sensor.spot_electricity_has_tomorrow_data, and sensor.spot_gas_has_tomorrow_data. However, the same, and therefore duplicate, unique_id are also used by the identically named non-binary sensors. That's why I changed the prefix to binary_sensor, to make the unique_id unique again.

@kukulich
Copy link
Contributor

I think the unique id is relevent only inside each platform. I’m not sure about it (not sure where in HA core to check) but I expect it because there’s now problem with current ids in my installation :)

@prehor
Copy link
Contributor Author

prehor commented Nov 21, 2024

Thanks for the contribution. Couple of nitpicks, nothing serious. We just need to make sure we maintain compatibility with older releases, could you double check that?

I rechecked the unique_id and entity_id against version v0.6.4, but this time on a fresh installation of Home Assistant. I found four errors in the entity_id that I missed last time. I fixed them, and now all sensors from version v0.6.4 have the same unique_id and entity_id, regardless of localization.

It might be necessary to adjust and add tests, but that's beyond my abilities.

@prehor prehor requested a review from rnovacek November 21, 2024 23:39
@prehor
Copy link
Contributor Author

prehor commented Nov 21, 2024

I think the unique id is relevent only inside each platform. I’m not sure about it (not sure where in HA core to check) but I expect it because there’s now problem with current ids in my installation :)

The entity registry is stored in the file .storage/core.entity_registry in the configuration directory. It lists all entities regardless of their type. Try installing the Spooky integration, which should help uncover any issues.

@prehor prehor marked this pull request as ready for review November 21, 2024 23:52
@kukulich
Copy link
Contributor

I think the unique id is relevent only inside each platform. I’m not sure about it (not sure where in HA core to check) but I expect it because there’s now problem with current ids in my installation :)

The entity registry is stored in the file .storage/core.entity_registry in the configuration directory. It lists all entities regardless of their type. Try installing the Spooky integration, which should help uncover any issues.

I've checked HA core sources and it looks the unique_id has to be unique only in the current domain and platform.

And see these lines copied from my core.entity_registry. Both have same unique_id because unique_id is really resolved only in the current domain and platform.

{"aliases":[],"area_id":null,"categories":{},"capabilities":null,"config_entry_id":"adc237427536078e54a34c8811c86c02","created_at":"2024-11-09T08:46:41.217662+00:00","device_class":null,"device_id":null,"disabled_by":null,"entity_category":null,"entity_id":"binary_sensor.spot_electricity_is_cheapest","hidden_by":null,"icon":null,"id":"189d7377ef0ec8057267915072964729","has_entity_name":true,"labels":[],"modified_at":"2024-11-09T13:21:36.992957+00:00","name":null,"options":{"conversation":{"should_expose":false}},"original_device_class":null,"original_icon":"mdi:cash-clock","original_name":"Aktuální spotová cena elektřiny je nejlevnější","platform":"cz_energy_spot_prices","supported_features":0,"translation_key":"current_spot_electricity_is_cheapest","unique_id":"sensor.spot_electricity_is_cheapest","previous_unique_id":null,"unit_of_measurement":null},
{"aliases":[],"area_id":null,"categories":{},"capabilities":null,"config_entry_id":"adc237427536078e54a34c8811c86c02","created_at":"1970-01-01T00:00:00+00:00","device_class":null,"device_id":null,"disabled_by":null,"entity_category":null,"entity_id":"sensor.spot_electricity_is_cheapest","hidden_by":null,"icon":null,"id":"6b0901305e2dd18b845ba7764be00a4b","has_entity_name":true,"labels":[],"modified_at":"2024-11-22T07:03:08.776407+00:00","name":null,"options":{"conversation":{"should_expose":false}},"original_device_class":null,"original_icon":"mdi:cash-clock","original_name":"Aktuální spotová cena elektřiny je nejlevnější (Zastaralé)","platform":"cz_energy_spot_prices","supported_features":0,"translation_key":"current_spot_electricity_is_cheapest","unique_id":"sensor.spot_electricity_is_cheapest","previous_unique_id":null,"unit_of_measurement":null},

@prehor
Copy link
Contributor Author

prehor commented Nov 28, 2024

I think the unique id is relevent only inside each platform. I’m not sure about it (not sure where in HA core to check) but I expect it because there’s now problem with current ids in my installation :)

The entity registry is stored in the file .storage/core.entity_registry in the configuration directory. It lists all entities regardless of their type. Try installing the Spooky integration, which should help uncover any issues.

I've checked HA core sources and it looks the unique_id has to be unique only in the current domain and platform.

And see these lines copied from my core.entity_registry. Both have same unique_id because unique_id is really resolved only in the current domain and platform.

My understanding of the internal logic of Home Assistant doesn't allow me to determine whether the unique_id must be unique only within a platform or across platforms as well. I would leave the decision on which version of unique_id to use up to @rnovacek.

@kukulich
Copy link
Contributor

My understanding of the internal logic of Home Assistant doesn't allow me to determine whether the unique_id must be unique only within a platform or across platforms as well. I would leave the decision on which version of unique_id to use up to @rnovacek.

I would be happy if the previous version can be preserved and my installation is not broken ;-)

Copy link
Owner

@rnovacek rnovacek left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, this was on my list for too long.

Sorry for the late review, it's really bad with my free time lately.

While I agree that breaking compatibility is not perfect, it's most important not to break compatibility between release versions. I'm afraid we won't be able to make the main branch compatible between all commits.

@rnovacek rnovacek merged commit c06c007 into rnovacek:main Dec 2, 2024
2 checks passed
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.

3 participants