custom-components / zaptec

zaptec charger custom component for home assistant
MIT License
67 stars 20 forks source link

Future path of the zaptec integration #47

Closed sveinse closed 1 year ago

sveinse commented 1 year ago

What is the current plans of features, functions and bug fixes of the Zaptec integration?

I'm working heavily on it in my own fork https://github.com/sveinse/zaptec/tree/dev. How can I best contribute with new ideas? I certainly don't want to waste energy on something that will be rejected. Is it useful to generate of a number of PRs into this repo?

Hellowlol commented 1 year ago

Hi,

I have no plans for any features atm, but open to suggestions. I think it awesome if more people have time to contribute as I have limited time available.

At the top of my head of things that could be nice would be to add more relevant switches. like cable lock etc, deactive charger etc

sveinse commented 1 year ago

I've been working on a major refactor of the whole zaptec component. https://github.com/sveinse/zaptec. I know this comes unannounced, but I started working on something and ended up improving the whole thing.

My main motivation was to make the wanted zaptec data-points more easily available as separate "native" entities from the component, not having to rely on attrs, templates and services. The fork fixes all this and makes it very much easier to add new entities. On the technical side, it is updated to a more "HA-like" implementation, using a data update coordinator. By the use of a common zaptec entity class, setting up entities in the various platforms are now very easy with little coding.

Improvements:

User impact:

Granted its a considerable change. I've made LOTS of changes. Still, I hope this can be interesting. I myself think this improvement would benefit the user. Its not done yet, but if this is interesting, I will make a PR for it.

Hellowlol commented 1 year ago

Hi,

Lots of good changes. I did a quick review of the code, it looks very nice. There are a couple of things that need be changed regarding the API. Like almost the removal of commands and making "all" API methods private.

Nice work 👍

sveinse commented 1 year ago

Cool 👍

I do have a few comments about the API.

First of all, one requirement if this ever would become an official HA component is that the zaptec API (api.py) would have to be moved to a separate python package and uploaded to pypi. I assume that is not an ambition and thus not considered. Yet, I do believe it should set the tone between the API-part (api.py) and the HA-part of the component. E.g. only use public methods from HA.

My motivation for the methods _req_*() is that the API needed a clearer separation between the downwards facing methods (zaptec web API) and the upwards facing methods (which HA consumes). The consumer/user of the API should not have access to the low-level communication methods (and data validation) -- especially when the Installation/Circuit/Charger exists that proxy the API data.

On the removal of commands, I am somewhat conflicted about it. I don't love where there are a gazillion methods that just proxy into an internal command method. I was considering reading the actual command names from the constants file, and that required a central command() method. OTOH having dedicated methods for central commands are indeed very useful.

All in all if you're interested @Hellowlol, I can prepare a PR for all this and we can continue the detailed discussion there. There are still a few FIXMEs that needs resolving, and I need some time to prepare.

edit: Forgot one thing: Do you think I should partition my fork into several PRs? It will take more efforts for me to make the PRs, but it might be easier to process the change.

Hellowlol commented 1 year ago

My motivation for the methods req*() is that the API needed a clearer separation between the downwards facing methods (zaptec web API) and the upwards facing methods (which HA consumes). The consumer/user of the API should not have access to the low-level communication methods (and data validation) -- especially when the Installation/Circuit/Charger exists that proxy the API data.

I think you are thinking about this wrong, I see 0 reasons why they should be made private. In this case, the integration is the user. I have no goals of getting this merged into HA main branch, but I do agree that the API should be split into a separate package. If I have time ill do that one of the upcoming weeks and switch from aiohttp to httpx while I'm at it.

On the removal of commands, I am somewhat conflicted about it. I don't love where there are a gazillion methods that just proxy into an internal command method. I was considering reading the actual command names from the constants file, and that required a central command() method. OTOH having dedicated methods for central commands are indeed very useful.

Get that, but it does serve as documentation for the API. As a API user you know what you want to do, but you don't go around and remember that 200 the command for upgrade firmware.

small PR > large PR. I suggest you start by opening a PR and we can discuss the different options. Almost all of it can be merged in as is.

sveinse commented 1 year ago

Added in PR #56