marq24 / ha-evcc

Home Assistant integration for evcc☀️🚘- optimized charging of electric vehicles, connecting your EV charger with your PV system. The integration use local polling (interval configurable) of the evcc API. Please note, that this integration is not official and not supported by the evcc developers. This project is not affiliated with evcc in any way.
Apache License 2.0
67 stars 3 forks source link

Minor comments after first use #2

Closed ThomDietrich closed 5 months ago

ThomDietrich commented 5 months ago

Hy @marq24, thanks for providing this integration. As mentioned in my community thread on the MQTT integration, I am very fond to finally see this being developed :)

May I post two-three smaller things that came to my mind upon first use of your integration?

  1. Integration name: You've named this integration "evcc Bridge (unofficial)". That's not quite in line with typical naming conventions. 99% of integrations are unofficial and your integration technically isn't a bridge, it is quite literally an integration :) In line with other integrations I would suggest "evcc" or "evcc - Solar Charging"
  2. HACS description has a typo: "HACs Home Assistant Integration for a local running evcc backend - control evcc from HA - communicate via the HTTP API" and you could make it more relevant to full text search: "Home Assistant integration for evcc - optimized charging of electric vehicles, connecting your EV charger with your PV system" (taken from their webpage)
  3. Entity friendly names: All entities have an additional "[evcc]" prefix and even though I understand why this might seem useful, it's far from the general guidelines / what you see in other integrations. All entities have a logical link to their integration and the addition in the friendly name isn't needed
  4. Entity IDs are often in the format of sensor.evcc_batterycapacity or sensor.evcc_chargepower_1_my_garage. I believe that 1. words should be separated (e.g. sensor.evcc_battery_capacity) and 2. the hierarchy/topology could be stricter: sensor.evcc_my_garage_charge_power

I hope you found this useful. I don't claim to know any of these points better than you do but would certainly be interested in your opinion about them. Best and thanks!

marq24 commented 5 months ago

Hi Thomas,

thanks for your valuable input (I just edited your post to make it a numbered list - so it's easier for me to refer to the different points)

re 2) Thanks - update is Done

re 4) This will become an 'breaking-change' for existing users - but I really like the idea to pull the loadpoint id in front of the entity id

re 3) I think I will make this configurable in the settings of the integration [default will be off - but I "like" it so I want to keep this for me personal usage]

re 1) Here I want to start some sort of discussion - and I am totally open minded - I just want to explain a bit more in detail why I have "selected" the current name. Adjusting the name is really a piece of cake and done in less then 2 minutes...

Why 'bridge'?

The integration does not provide "any functionality" - it does not even calculate something special - it's a mapping of entities to API endpoints. So when users going to see unplausibel data, then the root cause will be in 99.98% the API delivered this unplausibel data... So using the word brigde was my attempt to express "this piece of software is some sort of stupid"...

If in your experience "every" HA integration is exactly that (and indeed my other HA integrations are basically follow the same theory of operation) I just will drop it...

Why 'unofficial'?

This might be personal thing - IMHO it's important to indicate that this integration is not connected to the authors of evcc - I agree that my SENEC.Home or my Watterkotte integration are also not connected with these companies - but since evcc is GitHub opensource project I don't want to give the users (of this integration) the impression, that the it's supported by the authors/owners of evcc - not even that I have informed anybody. Using the logo was already a step that was not an easy step [mental wise].

ThomDietrich commented 5 months ago

Hey!

re 2) Great!

re 4) My thought exactly, hence why I wanted to mention it early. I expect a lot of users jumping over now that the community thread notifies most existing MQTT users about the integration... You could implement the breaking change early enough :)

re 3) There might be an easy workaround. Many integrations use the device name as the first element in their entities. In ESPHome this was even a breaking change not too long ago. You could use he user defined device name as the first part of the friendly name as well as for the entity_id. With the implicit entity_id cleaning you would probably be able to use "[evcc]" here.

re 1) I understand your arguments! And while I am generally in agreement the reality is different. Common integrations in my personal setup are among others "AstroWeather", "ESPHome", "Huawei Solar", "Kostal Plenticore Solar Inverter", "Solcast PV Forecast", "Samsung Smart TV", "Octoprint". Most of these have a very similar nature to yours. Regarding "unofficial" - You should definitely keep the unofficially in your README, arguably not needed in the title. After all, especially in open source it is quite customary and wanted that independent projects contribute towards each other. One more thought: Definitely inform them. They will be happy to hear that HA, one of the most important related software systems out there, now has well implemented support for EVCC. Thinking about it I am surprised that the official webpage does not list such software systems.

marq24 commented 5 months ago

re3) this is already present (some sort of) - the Title you specify during initial setup is first part of the entity_id... For the name prefix I have decided to use the configuration/option approach

The new release with all changes is online

Nochmals Vielen Dank Thomas für Dein Feedback

ThomDietrich commented 5 months ago

Installing now. Btw the installation destination is /config/custom_components/evcc_intg. Is that intended? I don't think it's bad, just wanted to highlight.

Natürlich, ich bin ein großer Fan vom open surce Gedanken ;)

ThomDietrich commented 5 months ago

Looking great overall. I have a bonus problem for you. The entity_id now spells sensor.evcc_neue_garage_charge_power, which is great. But why is the friendly name simply "Ladeleistung"? Shouldn't it be something like "Neue Garage: Ladeleistung" or "Ladeleistung (Neue Garage)"?

marq24 commented 5 months ago

Btw the installation destination is /config/custom_components/evcc_intg. Is that intended?

yes that's intended - after I made the mistake with my first two integrations (using an already existing name) I wanted to make sure that this evcc_intg is unique - and there will not be a evcc_prod or a evcc_dev stage ;-) - [at least not from me]...

I have a bonus problem for you...

Configure a second Loadpoint in evcc and the "bonus" is solved... The integration adds the Loadpoint Name to the Sensor only if there are more then one... [aka 'Highlander']

ThomDietrich commented 5 months ago

Re evcc_intg: All good.

Re bonus: I see! Not sure if I would prefer to see it even without a second loadpoint but it's fine. Sounds like a good strategy.

Thanks! I think I am through here :)