cdpuk / givenergy-local

Home Assistant integration for local access to GivEnergy inverter and battery systems
MIT License
50 stars 14 forks source link

Fix for Duplicate unique ID's with multiple batteries #34

Closed SciFi-Bob closed 1 year ago

SciFi-Bob commented 1 year ago

Hey,

Firstly I am no developer, just dabble a bit and this fixes an issue with 3 of the battery sensors getting duplicate unique_id values. I have no idea if it's the right or best way to do it as this is the first HA code I have done :-)

Hope it helps!

The error I was seeing was as follows. The erroring sensors were missing from all but the first battery.

2023-03-02 23:59:07.900 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_soc already exists - ignoring sensor.battery_charge 2023-03-02 23:59:07.906 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_num_cycles already exists - ignoring sensor.battery_cycles 2023-03-02 23:59:07.907 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_v_battery_out already exists - ignoring sensor.battery_output_voltage 2023-03-02 23:59:07.933 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_soc already exists - ignoring sensor.battery_charge 2023-03-02 23:59:07.934 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_num_cycles already exists - ignoring sensor.battery_cycles 2023-03-02 23:59:07.934 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_v_battery_out already exists - ignoring sensor.battery_output_voltage

cdpuk commented 1 year ago

Thanks for the PR. I'm glad to hear this worked for you but I'm still trying to work out exactly why. Since I've not got a 2nd battery to work with, I'm going to try and write some tests that demonstrate the change in behaviour and make this easier to maintain in the future.

SciFi-Bob commented 1 year ago

Hey. Well I now have a dev environment setup and have three batteries so would be more than willing to help you out! Can even get on a zoom if you like.

cdpuk commented 1 year ago

Would you be able to give this branch a go? https://github.com/cdpuk/givenergy-local/tree/bugfix/multiple-battery-identification I think I've worked out the cause. If it works for you I'll polish it up with the new tests I've created and get it published for others.

SciFi-Bob commented 1 year ago

Would you be able to give this branch a go? https://github.com/cdpuk/givenergy-local/tree/bugfix/multiple-battery-identification I think I've worked out the cause. If it works for you I'll polish it up with the new tests I've created and get it published for others.

No change to the behaviour, log below:-

2023-03-06 12:45:23.608 INFO (MainThread) [homeassistant.components.sensor] Setting up sensor.givenergy_local 2023-03-06 12:45:23.611 INFO (SyncWorker_2) [homeassistant.loader] Loaded binary_sensor from homeassistant.components.binary_sensor 2023-03-06 12:45:23.614 INFO (SyncWorker_0) [homeassistant.loader] Loaded number from homeassistant.components.number 2023-03-06 12:45:23.649 INFO (SyncWorker_2) [homeassistant.loader] Loaded switch from homeassistant.components.switch 2023-03-06 12:45:23.724 INFO (MainThread) [homeassistant.setup] Setting up binary_sensor 2023-03-06 12:45:23.725 INFO (MainThread) [homeassistant.setup] Setup of domain binary_sensor took 0.0 seconds 2023-03-06 12:45:23.727 INFO (MainThread) [homeassistant.setup] Setting up number 2023-03-06 12:45:23.730 INFO (MainThread) [homeassistant.setup] Setup of domain number took 0.0 seconds 2023-03-06 12:45:23.739 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.pv_energy_total 2023-03-06 12:45:23.756 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.pv_power_string_1 2023-03-06 12:45:23.768 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.pv_power_string_2 2023-03-06 12:45:23.781 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_import_today 2023-03-06 12:45:23.793 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_import_total 2023-03-06 12:45:23.806 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_export_today 2023-03-06 12:45:23.819 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_export_total 2023-03-06 12:45:23.831 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.inverter_output_today 2023-03-06 12:45:23.842 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.inverter_output_total 2023-03-06 12:45:23.859 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_charge_today 2023-03-06 12:45:23.872 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_discharge_today 2023-03-06 12:45:23.884 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_throughput_total 2023-03-06 12:45:23.898 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.consumption_power 2023-03-06 12:45:23.911 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_export_power 2023-03-06 12:45:23.923 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_voltage 2023-03-06 12:45:23.937 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_power 2023-03-06 12:45:23.949 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.inverter_eps_backup_power 2023-03-06 12:45:23.963 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_percent 2023-03-06 12:45:23.975 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_temperature 2023-03-06 12:45:23.986 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_voltage 2023-03-06 12:45:24.001 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.grid_frequency 2023-03-06 12:45:24.014 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.inverter_heatsink_temperature 2023-03-06 12:45:24.025 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.inverter_charger_temperature 2023-03-06 12:45:24.040 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.pv_energy_today 2023-03-06 12:45:24.055 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.pv_power 2023-03-06 12:45:24.068 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.consumption_today 2023-03-06 12:45:24.090 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.consumption_total 2023-03-06 12:45:24.108 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_mode 2023-03-06 12:45:24.123 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_charge_power_limit 2023-03-06 12:45:24.135 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_discharge_power_limit 2023-03-06 12:45:24.152 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_charge 2023-03-06 12:45:24.166 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_cycles 2023-03-06 12:45:24.179 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_output_voltage 2023-03-06 12:45:24.193 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_remaining_capacity 2023-03-06 12:45:24.208 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_cells_voltage 2023-03-06 12:45:24.216 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_soc already exists - ignoring sensor.battery_charge 2023-03-06 12:45:24.219 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_num_cycles already exists - ignoring sensor.battery_cycles 2023-03-06 12:45:24.220 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_v_battery_out already exists - ignoring sensor.battery_output_voltage 2023-03-06 12:45:24.230 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_remaining_capacity_2 2023-03-06 12:45:24.243 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_cells_voltage_2 2023-03-06 12:45:24.252 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_soc already exists - ignoring sensor.battery_charge 2023-03-06 12:45:24.254 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_battery_num_cycles already exists - ignoring sensor.battery_cycles 2023-03-06 12:45:24.256 ERROR (MainThread) [homeassistant.components.sensor] Platform givenergy_local does not generate unique IDs. ID BI2221G078_v_battery_out already exists - ignoring sensor.battery_output_voltage 2023-03-06 12:45:24.262 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_remaining_capacity_3 2023-03-06 12:45:24.277 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new sensor.givenergy_local entity: sensor.battery_cells_voltage_3 2023-03-06 12:45:24.303 INFO (MainThread) [homeassistant.components.binary_sensor] Setting up binary_sensor.givenergy_local 2023-03-06 12:45:24.309 INFO (MainThread) [homeassistant.components.number] Setting up number.givenergy_local 2023-03-06 12:45:24.340 INFO (MainThread) [homeassistant.setup] Setting up switch 2023-03-06 12:45:24.342 INFO (MainThread) [homeassistant.setup] Setup of domain switch took 0.0 seconds 2023-03-06 12:45:24.353 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new binary_sensor.givenergy_local entity: binary_sensor.battery_charge_slot_1 2023-03-06 12:45:24.376 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new binary_sensor.givenergy_local entity: binary_sensor.battery_charge_slot_2 2023-03-06 12:45:24.400 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new binary_sensor.givenergy_local entity: binary_sensor.battery_discharge_slot_1 2023-03-06 12:45:24.434 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new binary_sensor.givenergy_local entity: binary_sensor.battery_discharge_slot_2 2023-03-06 12:45:24.457 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new number.givenergy_local entity: number.battery_ac_charge_limit 2023-03-06 12:45:24.473 INFO (MainThread) [homeassistant.components.switch] Setting up switch.givenergy_local 2023-03-06 12:45:24.488 INFO (MainThread) [homeassistant.helpers.entity_registry] Registered new switch.givenergy_local entity: switch.battery_ac_charging

cdpuk commented 1 year ago

That's surprising, as I definitely saw a change for the better in the test. I've made another commit on that branch that tries an alternative approach. From looking at other integrations I noticed they try to make just one call to async_add_entities. I've adjutsed the sensor.py code to do that, so perhaps we'll see an improvement. Again much appreciated if you can give it a try.

SciFi-Bob commented 1 year ago

That's worked! Nice one :-)

By the way, what is meant to be happening with the individual cell voltages, you seem to collect them all but unless I am missing something they are not available to HA anywhere...

return self.data.dict(include={f"v_battery_cell_{i:02d}" for i in range(1, 16)}) # type: ignore[no-any-return]

Personally I would like to see these as it would allow for diagnosing an issue with a battery before it becomes a failure...

cdpuk commented 1 year ago

Great, I've opened another PR for the revised approach, but thanks for the hint in the right direction in your original proposed change. I'll close this in favour of #35.

The cell voltages show as attributes under the battery voltage sensor. It was done this way to avoid having loads of individual sensors. They can be extracted to sensors using templates if necessary.