Major improvement refactor by sveinse · Pull Request #56 · custom-components/zaptec

sveinse · 2023-08-10T18:26:25Z

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: 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:

<name> Installation
<name> Circuit
<name> Charger

User changes

Entity naming is completely changed. Entity names are read from Zaptec Portal.
Many new entities/sensors/buttons instead of relying on attrs and templates
Services available as buttons which doesn't require charger_id
Entities are grouped into devices and devices reflect the Zaptec structure in portal
Add services "authorize_charging" and "deauthorize_charging"
Remove obsolete services "start_charging" and "stop_charging"
Added support for "download diagnostics"
Add unique id on device

Technical features

Refactor to use the common HA practices for entities
Make it much simpler to add new entities
Add ZaptecBaseEntity for heavy lifting
Added ZaptecUpdateCoordinator to centrally manage API updates
Added new protocols: binary sensor, button, number, update in addition to sensor and switch
General: Added type hints

API changes

Made ZaptecBase an ABC base class
Cleaned up ZaptecBase.set_attributes()
Fixed infinite authentication failed loop
Cleaned up stream message reception
Fixed error handling in stream reception
Added MC-NBFX data decoder for stream messages
Cleanup exception handling
Structurize build() and state() for init and updating
Added type mapping on attributes, ensuring a consistent type from Zaptec

Known issues

* Refactor to use the common HA approach for entities * Make it much simpler to make new entities * Add ZaptecBaseEntity for heavy lifting * Added ZaptecUpdateCoordinator to centrally manage API updates * Added new protocols: binary sensor, button, number in addition to sensor and switch * Added support for "download diagnostics" * Add unique id on component * General: Added type hints User changes: * Entity naming is completely changed. Entity names are read from Zaptec Portal. * Entities are grouped into devices and devices reflect structure * Remove services "start_charging" and "stop_charging" (not available in HW) * Add services "authorize_charging" and "deauthorize_charging" * Many new entities/sensors/buttons instead of relying on attrs and templates * Services also available as buttons which doesn't require `charger_id` API changes: * Made ZaptecBase an ABC base class * Cleaned up ZaptecBase.set_attributes() * Fixed infinite authentication failed loop * Cleaned up stream message reception * Fixed error handling in stream reception * Added MC-NBFX data decoder for stream messages * Set all Zaptec API facing methods as private methods * Provide proper exception on fails * Structurize build() and state() for init and updating * Added properties (e.g. `Charger.operating_mode`) that translates Zaptec API internal numbers to strings

Hellowlol · 2023-09-11T19:32:49Z

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

sveinse · 2023-09-11T20:04:50Z

@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.

* Improved API and interface. Added data validation. Added type conversion. * Added several new HA entities * Fix bugs and smaller improvements

sveinse · 2023-09-14T14:25:01Z

@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 · 2023-09-16T12:04:35Z

@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 · 2023-09-16T14:18:32Z

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

* Fix failure to start if zaptec user have no extra privileges, sveinse/zaptec#10 * Ensure entities are marked as "Unavailable" if not available in Zaptec * Fix text of binary sensor "charger.is_authorization_required" * Fix more user friendly error messages

* Fix sveinse/zaptec#2, sveinse/zaptec#10, sveinse/zaptec#15 * Handle starting zaptec with non-privileged user * Fix "unavailable" state on entities * Fix non-working buttons and commands * Fix error reporting and logging * Fix unity bug in "total_charge_power" * Added "total_charge_power_session" * Update API data validators

Hellowlol · 2023-09-17T18:47:25Z

@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 · 2023-09-17T18:50:00Z

 MIT License

 Copyright (c) 2021 Hellowlol and contributers
+Copyright (c) 2023 Svein Seldal, @sveinse


Lets just have one copywrite line:
.... 2023 @Hellowlol , @sveinse and contributers

Hellowlol · 2023-09-17T18:53:18Z

-                    # Add names.
-                    obs.update({value: key for key, value in v.items()})
+class AuthorizationError(ZaptecApiError):
+    '''Authenatication failed'''


Run black and fixup so ' -> "

I haven't run black on anything. I didn't know if you did. So I'll run everything through black and update the PR.

Hellowlol · 2023-09-17T18:53:45Z

-                    self.__class__.__name__,
-                )
+    def set_attributes(self, data: TDict) -> bool:
+        """Set the class attributes from the given data"""""


Hellowlol · 2023-09-17T18:56:06Z

-        """Set a limit now how many amps the installation can use
+        # Get the hierarchy of circurits and chargers
+        try:
+            hierarchy = await self._account._request(f"installation/{self.id}/hierarchy")


Why isnt hirarchy used directly?

It is. If you look in line 172.

Hellowlol · 2023-09-17T18:58:08Z

+        '''Raw request for installation data'''
+
+        # Get the installation data
+        data = await self._account._request(f"installation/{self.id}")


Same for this one? Why not use the api?

There is currently no method in class Account for this as the only consumer of this request is in the class Interface.

sveinse · 2023-09-17T22:04:40Z

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 · 2023-09-17T23:28:58Z

@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 · 2023-09-22T21:17:08Z

@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.

* Fix timeout issues, sveinse/zaptec#19 * Fix sensor stateclass, making it possible to use kwh from zaptec in HA energy, sveinse/zaptec#16

Hellowlol · 2023-09-26T22:14:41Z

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

Hellowlol · 2023-09-26T22:18:23Z

-            _LOGGER.error("Could not get info from %s: %s", full_url, err)
+                        # Validate the incoming json data
+                        # FIXME: This raise pydantic.ValidationError if the json is unexpected. How to handle this?
+                        validate(json_result, url=url)


Why validate?

Because we cannot assume that the data from the Zaptec API is formatted correctly, one should always validate incoming data. What if Zaptec changes something?

We rely on certain data being present, e.g. for data in installations["Data"]: assumes Data is there. If it isn't then the resulting error can be very hard to link to the incoming data being malformed. Validation ensures that one gets a clear "I don't understand the data I just got"-type errors, which is a lot easier to deal with.

There is normally another dimension to validation and that is deserialization into named objects (see validation.py). But given the way this API is constructed with the special attribute handling, this feature isn't used.

Hellowlol · 2023-09-26T22:19:51Z

-        _LOGGER.debug("Calling %s", cmd)
-        return await self._account._request(cmd, method="post")
-
-    async def update(self, data):


Comment out, don’t remove this method.

I can add it back if you want @Hellowlol. I'm sorry, I deleted the function because it doesn't do anything, and it is messy to have have useless methods included. What is the intent of the function?

Honestly, I can’t remember. I just remember that I needed it for something. I’ll get back to it when I have time

I propose that we leave it out for now (since it does nothing) and add it later when you have time to look at it.

sveinse · 2023-09-27T09:35:30Z

@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 · 2023-09-27T19:11:17Z

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 · 2023-10-02T18:54:14Z

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

* Add support for selecting which chargers to add, sveinse/zaptec#5 * Add support for adding optional prefix, sveinse/zaptec#4 * Adding API timeout to 60 secs, sveinse/zaptec#19 * Updated UI messages * Fixup general error handling * Change API to allow for supporting selecting which chargers to use

Based on the latest version of https://github.com/ludeeus/integration_blueprint

* Add TOTAL_INCREASING to totl_charger_power_session, sveinse/zaptec#23 * Fix regression of missing services, sveinse/zaptec#24 * Refactor services to make them easier to control from UI

* API: Make requests more robust by introducing a retry mechanism, sveinse/zaptec#19 * API: Fix set_limit_current bug, sveinse/zaptec#27 * Fix coordinator update when calling services

svenakela · 2023-11-09T22:13:51Z

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

sveinse · 2023-11-11T11:21:09Z

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

svenakela · 2023-11-22T14:31:59Z

@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 · 2023-11-22T22:09:31Z

@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 · 2023-11-23T07:56:39Z

@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.

sveinse mentioned this pull request Aug 10, 2023

Future path of the zaptec integration #47

Closed

Improvements

8562495

* Improved API and interface. Added data validation. Added type conversion. * Added several new HA entities * Fix bugs and smaller improvements

This was referenced Sep 14, 2023

Add support for reading device type schemas #42

Closed

fix reg #51

Closed

sveinse mentioned this pull request Sep 16, 2023

Zaptec Go: Authorising a charge service #55

Closed

sveinse added 2 commits September 17, 2023 01:02

Fix issues

1ca1859

* Fix failure to start if zaptec user have no extra privileges, sveinse/zaptec#10 * Ensure entities are marked as "Unavailable" if not available in Zaptec * Fix text of binary sensor "charger.is_authorization_required" * Fix more user friendly error messages

Hellowlol reviewed Sep 17, 2023

View reviewed changes

sveinse mentioned this pull request Sep 22, 2023

Fix set charge current #48

Merged

sveinse added 2 commits September 22, 2023 22:58

Formatted project with black

c79fca8

Fixed OCMF decoding on attributes and improve diagnostics redaction

95f9186

Fixed issues

4974217

* Fix timeout issues, sveinse/zaptec#19 * Fix sensor stateclass, making it possible to use kwh from zaptec in HA energy, sveinse/zaptec#16

Hellowlol reviewed Sep 26, 2023

View reviewed changes

sveinse mentioned this pull request Oct 9, 2023

chore: Update to use latest devcontainer sveinse/zaptec#21

Merged

sveinse marked this pull request as draft October 11, 2023 09:34

sveinse and others added 7 commits October 16, 2023 02:00

chore: Update to use latest devcontainer

8e91327

Based on the latest version of https://github.com/ludeeus/integration_blueprint

Updated devcontainer

8fe9879

Minor cleanups and simplifications

c2de9e2

Rename CONF_NAME to CONF_PREFIX

14573b7

Fixes

aebc427

* Add TOTAL_INCREASING to totl_charger_power_session, sveinse/zaptec#23 * Fix regression of missing services, sveinse/zaptec#24 * Refactor services to make them easier to control from UI

Fixes API retry robustness and set_limit_current

842b825

* API: Make requests more robust by introducing a retry mechanism, sveinse/zaptec#19 * API: Fix set_limit_current bug, sveinse/zaptec#27 * Fix coordinator update when calling services

sveinse added 3 commits November 11, 2023 12:31

Adding cloud flag, sveinse/zaptec#30

b02efea

Update documentation and manifest

7b0087f

Bump revision to 0.7.0

167847d

sveinse marked this pull request as ready for review November 11, 2023 13:59

sveinse merged commit a20ef04 into custom-components:master Nov 11, 2023

sveinse deleted the feature-refactor branch December 29, 2023 12:01

Uh oh!

Conversation

sveinse commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User changes

Technical features

API changes

Known issues

Uh oh!

Hellowlol commented Sep 11, 2023

Uh oh!

sveinse commented Sep 11, 2023

Uh oh!

sveinse commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sveinse commented Sep 16, 2023

Uh oh!

Bugs4Bugs commented Sep 16, 2023

Uh oh!

Hellowlol commented Sep 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sveinse commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sveinse commented Sep 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sveinse commented Sep 22, 2023

Uh oh!

Hellowlol commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sveinse Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sveinse commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sveinse commented Sep 27, 2023

Uh oh!

Hellowlol commented Oct 2, 2023

Uh oh!

svenakela commented Nov 9, 2023

Uh oh!

sveinse commented Nov 11, 2023

Uh oh!

svenakela commented Nov 22, 2023

Uh oh!

sveinse commented Nov 22, 2023

Uh oh!

sveinse commented Aug 10, 2023 •

edited

Loading

sveinse commented Sep 14, 2023 •

edited

Loading

sveinse commented Sep 17, 2023 •

edited

Loading

sveinse commented Sep 17, 2023 •

edited

Loading

Hellowlol commented Sep 26, 2023 •

edited

Loading

sveinse Sep 27, 2023 •

edited

Loading

sveinse commented Sep 27, 2023 •

edited

Loading