Multiple bus devices and id conversion

dm82m / hass-Deltasol-KM2

Custom component for retrieving sensor information from Resol KM1/KM2, DL2/DL3, VBus/LAN, VBus/USB

MIT License

25 stars 6 forks source link

Multiple bus devices and id conversion #16

Closed chiefdeputy closed 2 years ago

chiefdeputy commented 2 years ago

Implements https://github.com/dm82m/hass-Deltasol-KM2/issues/10 with my suggestions.

Reverting filters of DLx devices In my opinion, we should keep the configuration as simple as possible and the code as clean as possible. The functionality can be achieved using Home Assistant GUI configuration, by renaming the now unique entities.
Revised API to use a named tuple to exchange information. So you don't have to rely on int indexes to get the right field. (readability of code)
Finally if the component finds entities with the old id, it is converted to the new one. If a new one already exists it is replaced by the old entity. (Should only be the case for @dm82m and me, as we tested the new ids before we had a conversion)

Already rebased on https://github.com/dm82m/hass-Deltasol-KM2/pull/15. So we should merge that first.

Happy to hear your opinions, especially about the drop of filtering...

dm82m commented 2 years ago

I merged the other changes, can you please rebase your commits and update this PR here?

Regarding the filter are my thoughts: in general I am with you: simplicity, clean code, ... On the other hand: if the DLX provides this feature and people are using it, so they invested time in renaming the sensors on their DLX - why wouldn't we allow them to use it here in our integration? It would just safe them their time. So in my opinion we should let this filtering feature as part of our integration.

rest of the code feedback will inline above!

ostat commented 2 years ago

Regarding the filter are my thoughts: in general I am with you: simplicity, clean code, ... On the other hand: if the DLX provides this feature and people are using it, so they invested time in renaming the sensors on their DLX - why wouldn't we allow them to use it here in our integration? It would just safe them their time. So in my opinion we should let this filtering feature as part of our integration.

I am keen to keep the ability to use the dlx filter option.

Otherwise the changes look good. Using the header["id"] + "__" + field["id"] for the unique ID is going to work well.

dm82m commented 2 years ago

Looks good so far, will test it tomorrow and release it if everything works out on my local instance! Thanks for all the work!

dm82m commented 2 years ago

-> fixed that on my own! PR is merged!

Logger: homeassistant.config Source: config.py:454 First occurred: 07:40:12 (1 occurrences) Last logged: 07:40:12

Invalid config for [sensor.deltasol]: not a string value: False for dictionary value @ data['api_key']. Got None. (See /config/configuration.yaml, line 233). Please check the docs at https://github.com/dm82m/hass-deltasol-KM2