search

custom-components / zaptec

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

Major improvement refactor #56

Closed sveinse closed 1 year ago

sveinse commented 1 year ago

This is a considerable PR that contains a major refactor and contribution to the zaptec component. The intent is to improve the overall usability, feature availability and code structure of the integration. See below for a full list of features.

ℹ️ NOTE: This PR is available for beta-testing. Go to https://github.com/sveinse/zaptec for instructions on how to install with HACS.

:warning: WARNING: For users this change considerably change how zaptec entities are named and accessed, and any existing automation will break. The scheme of having zaptec-charger-xxxxx* is replaced by entities named using the name set in the Zaptec Portal, e.g. Sol Charger mode.

The following binary sensors (for being connected) includes all attributes if that's needed for any manual templates:

User changes

Technical features

API changes

Known issues

Hellowlol commented 1 year ago

@sveinse I have granted you write permission to the repo.

sveinse commented 1 year ago

@Hellowlol Thank you very much! Even with this privilege, I definitely would like your input on the design of this. One question I'd like to find out eventually is what entities and control the users of this really needs.

FYI I'm actively working on some improvements based on some early feedback that I think should be included into this PR. One of the changes is to revert back some of the more controversial changes with command() to create an easier API to use.

sveinse commented 1 year ago

@Hellowlol I have updated the PR, where I improved the API interfaces added some entities I was missing in my own home setup. Removed the _req_* interfaces you didn't fancy and more. Please feel free to comment.

I'm going to create a new issue where I can invite users to beta-test it. Since I don't have access to the server side of the Zaptec API, it is very hard to test the Zaptec component. We need input from others on how it works in their setup. I've released a zaptec-dev at https://github.com/sveinse/zaptec which is installable in HACS that can be used for testing.

edit Issues pertaining to this PR is tracked in https://github.com/sveinse/zaptec/issues

sveinse commented 1 year ago

@Hellowlol is it ok if we bump the version of zaptec when the beta testing of this PR is done and we merge it in? 0.0.6 is a very low number. I'd argue that we're closing up on something stable, so I'd like to suggest 0.9 or even 1.0?

Bugs4Bugs commented 1 year ago

Not sure if this is the right place to post, but how do I access the ZAPTEC sense information and show it in HA?

Hellowlol commented 1 year ago

@sveinse I think we are far from 1.0 until it has been appropriately tested against all the chargers. I suggest you merge the PR bump to 0.0.7, release it as a beta, and write whats breaking in the release notes. After a testing period of X weeks no real new issues have arisen. Bump the 0.0.7 to 1.0 as a normal release

sveinse commented 1 year ago

Thank you for the review and feedback!

@sveinse I think we are far from 1.0 until it has been appropriately tested against all the chargers. I suggest you merge the PR bump to 0.0.7, release it as a beta, and write whats breaking in the release notes. After a testing period of X weeks no real new issues have arisen. Bump the 0.0.7 to 1.0 as a normal release

@Hellowlol I agree that in the current state we're far from 1.0. I was thinking ahead. Personally I find the double zero 0.0.7 a bit like "I have no idea if this actually works". The integration at 0.0.6 were well into being fit for purpose and have been used a while in the field. I think it deserves a more "mature" number. Different people have different interpretations of these numbers, so I'm not going to push hard for 0.x. If 0.0.7 is what you want, this is what we'll do.

sveinse commented 1 year ago

@Hellowlol I'd like to talk about the architecture of api.py. Ref the _request(f"/installations/{self.id}") call, the reason that has gone back and forth with API methods in class Account is that I've been searching on how to make it most consistent. The existing design were a bit a little bit of everything.

api.py has two interfaces, one low-level interface towards the Zaptec cloud API and one high-level interface towards the user. User in this case is the rest of the zaptec component. The low-level interface is best represented by class Account while the class Interface and class Charger represents the high-level interfaces.

Shall we make it that class Account is the only low-level interface toward the cloud API and that every API function has a method in this class? Then all the high-level classes only use public class Account methods to get data from the cloud. No _request() direct call. I can make this change if this is what you'd prefer. It gives good separation between the layers. In hindsight this is probably what was intended to begin with.

PS! A notable downside to this is that the code below is bloated if there is only one caller of it -- the high-level classes. Since this is the low-level interface, the user should never need to interact with the low-level methods directly.

# Method in class Account
async def installation_info(self, inst_id: str):
    return await self._request(f"installation/{inst_id}")

edit Here is a crude implementation of how api.py can be if we chose to have all access to the API in class Account. How does this look? https://gist.github.com/sveinse/c98a39fba92dc328fc017da956cd8063

sveinse commented 1 year ago

@Hellowlol PR updated with black, fix your comments on copyright, fix diagnostics anonymization error. The only remaining issue is the comments on the API. Is it ok as it is in the PR, or should I refactor to change into the proposed structure presented in https://gist.github.com/sveinse/c98a39fba92dc328fc017da956cd8063 ?

What is the next steps? Should you approve this merge, or can I do it? How do we proceed? Please advice. I think there a a few users waiting on this making it into the official channels.

Hellowlol commented 1 year ago

The entire point is to have an object api. If i wanted to just use data method i might just use a swagger client. I dont care how it implementedbut there should be logic do so the data is connected to the correct object. Account example account get the live connection details but the rest of the logic should be in install class, charger class etc.

Nice work👍

Feel free to bump to whatever version you feel is appropriate for the state the repo is in, merge and release it as a prerelease

sveinse commented 1 year ago

@Hellowlol

The entire point is to have an object api. If i wanted to just use data method i might just use a swagger client. I dont care how it implementedbut there should be logic do so the data is connected to the correct object. Account example account get the live connection details but the rest of the logic should be in install class, charger class etc.

The object api is a high-level interface which sits on top of the Zaptec cloud API and returns python objects, e.g. Charger object and the Installation.set_limit_current() method.

The low-level interface communicates the data from the cloud as-is. E.g. say we have a Charger.status() which is a GET to charger/{id}/status and return the data as-is. This is the format of the Zaptec swagger API. I've never used swagger to generate python interfaces, but I can imagine that this is what it would use.

These two interfaces are very different and have different usages. It is very common in py apis to hide the low-level methods and only present the high-level methods. This was the path I was pursuing and probably the cause for these discussions. We can have both interfaces if that is what's wanted.

sveinse commented 1 year ago

Feel free to bump to whatever version you feel is appropriate for the state the repo is in, merge and release it as a prerelease

Thank you for the trust! 🥇

I suggest 0.7 (it's far more stable than the 0.0.x number alludes to). I will make it a pre release until it has had some exposure in the field and then release it.

PS! You execute merges as regular merges, no squash or rebase, right?

Hellowlol commented 1 year ago

I have no fixed rule for how i do merges, it depends in the situasion. Generally i prefer merge

svenakela commented 1 year ago

So close but no cigar. Come on guys, you can do it! (i.e. I want to load balance my charger)

sveinse commented 1 year ago

@svenakela close indeed. Testing is what remains. Have you tested the zaptec-dev implementation?

svenakela commented 1 year ago

@svenakela close indeed. Testing is what remains. Have you tested the zaptec-dev implementation?

I am on 0.7.0 now. Go charger, if anything can be tested let me know.

sveinse commented 1 year ago

@svenakela sure. Run it, test it, use it. Getting some real-life experience with it. The more experience we get with 0.7.0 the faster it can be released to a production version.

svenakela commented 1 year ago

@svenakela sure. Run it, test it, use it. Getting some real-life experience with it. The more experience we get with 0.7.0 the faster it can be released to a production version.

Charge limiting and balancing as we speak.