core icon indicating copy to clipboard operation
core copied to clipboard

Add vartastorage integration

Open Vip0r opened this issue 3 years ago • 37 comments

Breaking change

Proposed change

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [X] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request: https://github.com/home-assistant/home-assistant.io/pull/21112

Checklist

  • [ X] The code change is tested and works locally.
  • [ X] Local tests pass. Your PR cannot be merged unless tests pass
  • [ X] There is no commented out code in this PR.
  • [ X] I have followed the development checklist
  • [ X] The code has been formatted using Black (black --fast homeassistant tests)
  • [ ?] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • [ X ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ X ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ X ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ X ] Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • [ ] No score or internal
  • [ X ] 🥈 Silver
  • [ ] 🥇 Gold
  • [ ] 🏆 Platinum

To help with the load of incoming pull requests:

  • [ ] I have reviewed two other [open pull requests][prs] in this repository. Not yet - but i will dive into it [prs]: https://github.com/home-assistant/core/pulls?q=is%3Aopen+is%3Apr+-author%3A%40me+-draft%3Atrue+-label%3Awaiting-for-upstream+sort%3Acreated-desc+review%3Anone+-status%3Afailure

Potential other improvements:

  • Make polling interval configurable by user.
  • Split charge cycle count sensor into two distinct sensor entities.
  • Use consistent naming of sensor entities within the integration.
  • Update the tests.

Vip0r avatar Jan 09 '22 13:01 Vip0r

Hi ezlo-picori, hi frenck,

thank you very much for your comments. I will update my code and commit this soon again. For two of this change requests i have follow up questions. Kindly ask you to provide me some feedback (see above)

Vip0r avatar Jan 10 '22 19:01 Vip0r

Hi ezlo-picori, hi frenck,

i modified all files according to your recommendations. Thank you very much for this feedback.

Could you please doublecheck and let me know how we can proceed?

Happy Weekend!

Vip0r avatar Jan 15 '22 13:01 Vip0r

Hi ezlo-picori, thanks once again for this valueable feedback! Really appreciate your change requests.

Please excuse that this PR was not already including all of this. I tried my best to make the comment compliant to alle the requirements in the docs.

Is there anything else what in your opinion need to be changed?

Vip0r avatar Jan 16 '22 13:01 Vip0r

Hi ezlo-picori, thanks once again for this valueable feedback! Really appreciate your change requests.

Please excuse that this PR was not already including all of this. I tried my best to make the comment compliant to alle the requirements in the docs.

Is there anything else what in your opinion need to be changed?

Thank you for quickly taking these comments into account (and sorry for the misguidance regarding the SignificantChangeChecker...) As far as I am concerned, I do not have more comments on the current version of this integration, but if someone else requests some improvements, I'll look to the updated version.

Have a great day

ezlo-picori avatar Jan 16 '22 13:01 ezlo-picori

Hello Frenck, is there anything additional what i can do or do i just have to wait for the next review?

Don't want to put pressure on it - just wan't to be sure that there is currently nothing missing from my side

Vip0r avatar Jan 25 '22 21:01 Vip0r

Thanks for you implementation, i tried it already as custom component.

But it looks like not every value is working for me :-( But not a problem with your implementation, my varta station didn't send the value :-( Really missing the total production power. Any idea?

But for the rest it works! Thanks a lot!!

Screenshot_2022-02-09-16-15-01-007_com android chrome

SChristophS avatar Feb 09 '22 17:02 SChristophS

Thanks for you implementation, i tried it already as custom component.

But it looks like not every value is working for me :-( But not a problem with your implementation, my varta station didn't send the value :-( Really missing the total production power. Any idea?

But for the rest it works! Thanks a lot!!

You can't imagine how excited i am. You are the first one using this integration. That feels marvelous :)

Regarding your question: While implementing the modbus client for the API i found two different docs describing the Modbus registers, formats and so one (just let me know and drop me a msg if you wan't me to share it with you) In the documentation i saw that some Modbus registers are only available for different device types. (This means some register are only available for VARTA element, some for VARTA pulse, some for VARTA link and so one) Sadly i also found the documentation is not fully correct. For example i found some some file types are in the reality on my device different from the documentation and so one. Long story short, there is unfortunately nothing you can do. I have on my side the same situation, the device is providing only the current production power of my panels and not the total one.

One thing you should check: When browsing on to the WebUI of your VARTA device, check if you configured your inverter connection in the settings (Energymanager => SunSpec-Manager)

Last but not least you will have the following option. After configuring your inverter-connection as described above you should see at least the current production power for your panels in HA, you can use the the core integration "Rieman sum integral" (https://www.home-assistant.io/integrations/integration/) to generate a total value (per day/per week/..) for this

Hope this helps you. Just let me know if you have any further questions

Vip0r avatar Feb 10 '22 18:02 Vip0r

Hi! Happy to see your integration! I have some trouble with calculating total charge/discharge energy with the riemann integration (if interested: [Riemann integration for discharge and loading behave different](https://community.home-assistant.io/t/riemann-integration-for-discharge-and-loading-behave-different/393003)

Because of this I would be interested how you get on this values. I see you have values for total discharge and charge. As far as I know there is only a modbus register for the total charge energy (1069 lower / 1070 higher word on my Varta Element).

mxwi avatar Feb 18 '22 07:02 mxwi

Because of this I would be interested how you get on this values. I see you have values for total discharge and charge. As far as I know there is only a modbus register for the total charge energy (1069 lower / 1070 higher word on my Varta Element).

Hey, as you mentioned total charged energy is: 1069 lower / 1070 higher word. For discharge the VARTA element only provide the current values:

  • active power in W (1066, SINT16)
  • apparent power in VA (1067, SINT16)

Vip0r avatar Feb 18 '22 18:02 Vip0r

@Vip0r It would be awesome if you could also include the totals in the Varta integration. That way we could use it with the HA Energy functionality without using the Riemann integration.

However, I could not figure out a way to get them via modbus. But they can easily be pulled using the CGI from the webinterface:

curl -X GET http://Varta-IP/cgi/energy.js                                
EGrid_AC_DC = 147378;
EGrid_DC_AC = 24301;
EWr_AC_DC = 151225;
EWr_DC_AC = 120430;
Chrg_LoadCycles = [9];

Where EGrid_AC_DC == Grid -> Building (Wh) EGrid_DC_AC == Home -> Grid (Wh) EWr_AC_DC == Inverter AC -> DC == Total Charged (Wh) EWr_DC_AC == Inverter DC -> AC == Total Discharged (Wh) Chrg_LoadCycles == Charge Cycle Counter

In addition there is also the "Service" Endpoint available here:

curl -X GET http://Varta-IP/cgi/user_serv.js
FilterZeit = 5839;
Fan = 0;
Main = 1;

Which return the time in hours until the next service / maintenance of the filter.

dr-waterstorm avatar Feb 22 '22 11:02 dr-waterstorm

@dr-waterstorm : Wow, thats just amazing. I also was not able to get this values via modbus. However i was not aware about this CGI. I will include this values of course as well!

@frenck : Short question - do you recommend to make this change now? Or should we wait, push this as a first version and integrate the additional sensors in a next version?

Vip0r avatar Feb 24 '22 20:02 Vip0r

@Vip0r Depends a bit... in the end, things like these are abstracted in the library. So I guess for Home Assistant end, there doesn't change much?

frenck avatar Apr 26 '22 23:04 frenck

Is there a way I can test this integration in my HA instance? I have a Varta Element 6.

Sesshoumaru-sama avatar Apr 28 '22 10:04 Sesshoumaru-sama

sorry if this is a stupid question but I just wanted to know when I can expect this to be merged? I just want to use this integration :)

louisdmueller avatar Jun 27 '22 18:06 louisdmueller

I don't know. There is a branch-conflict - someone from the dev team has to check it.

One more questions. What do you guys have in Chrg_LoadCycles = [XX]; For me it is 24, but I must have about 700 loadingcycles. Do I interpret the value wrong?

mxwi avatar Jul 11 '22 11:07 mxwi

I'd also love to see this merged. Hopefully this makes it into HA soon. For now I'm still using my "custom integration" which basically does the same. But it's not as neat, I have to admin.

@mxwi Mine states 96 cycles which seems to correspond to my estimate.

dr-waterstorm avatar Jul 11 '22 11:07 dr-waterstorm

Hey

@mxwi : Loadcycles show 170 on my side

@ezlo-picori , @frenck : Could you give me a short hint what would be my next steps to get this into any of the next releases?

Vip0r avatar Jul 15 '22 17:07 Vip0r

I wish I had any clue for you, but my own integration PR has also been stuck for months. The core dev team is quite busy and I guess our integrations are no priority for them, which I can understand though it is demotivating... Good luck to you for your integration to be merged!

ezlo-picori avatar Jul 15 '22 17:07 ezlo-picori

Hey @dr-waterstorm,

thanks for your great feedback once again. I made some changes and now also poll the CGI endpoints you mentioned in the used python library. This sensor integration has been updated as well. So you would be able to get the maintenance data and energy totals with this now. Of course you could use the integration within your custom_integration folder as long as it is not pushed to HA core.

by the way: Do you have any further informations what the values "main" and "fan" on the service CGI endpoints stands for?

Andy

Vip0r avatar Jul 15 '22 21:07 Vip0r

@Vip0r glad I could help and we now have the "absolute" values :) Sadly I do not know what "main" and "fan" does. I'd guess that "fan" states either if the fan is running or the speed of the fan. But no clue what "main" could be, maybe if the battery is "turned on" / "working"?

dr-waterstorm avatar Jul 18 '22 07:07 dr-waterstorm

I assume main does mean maintenance, but I do not if maintenance does mean changing of filter is required or if it does mean that service personnel has to be ordered.

KNXBroker avatar Jul 18 '22 08:07 KNXBroker

custom component

Is there an easy way to test this integration? I'm a beginner on home-assistant and would like to integrate my Varta Storage in my dashboard. Thanks

typozh avatar Aug 10 '22 14:08 typozh

@typozh 👋 Hi there, this is the review process for an integration, and we generally like to keep that on the topic of the actual review at hand, focussing on the code and the processes of getting it into Home Assistant itself.

If you have additional questions like you just wrote, you should try our Community Forum or join our Discord chat server.

You are more likely to get the answer you are looking for in those places.

Thanks! 👍

../Frenck

frenck avatar Aug 10 '22 15:08 frenck

@Vip0r are you still interested in moving forward with this integration? Would also be available to test with an Element 9. Thx

MSiegrist avatar Nov 01 '22 13:11 MSiegrist

Yes i am still interested in the Varta Element integration, and i can test it when available.

Yours: Peter


Från: Marc Siegrist @.> Skickat: den 1 november 2022 14:50 Till: home-assistant/core @.> Kopia: Peter Öman @.>; Manual @.> Ämne: Re: [home-assistant/core] Add vartastorage integration (PR #63736)

@Vip0rhttps://url10.mailanyone.net/scanner?m=1oprfY-0002Mw-5w&d=4%7Cmail%2F90%2F1667310600%2F1oprfY-0002Mw-5w%7Cin10g%7C57e1b682%7C24173980%7C12902222%7C6361244CAC88236A26B208BD631DD9E9&s=doedULDmkZgPLoe0goHTUS_2SRU&o=%2Fphti%3A%2FgtsmbthVco%2Fu.r0pi are you still interested in moving forward with this integration? Would also be available to test with an Element 9. Thx

— Reply to this email directly, view it on GitHubhttps://url10.mailanyone.net/scanner?m=1oprfY-0002Mw-5w&d=4%7Cmail%2F90%2F1667310600%2F1oprfY-0002Mw-5w%7Cin10g%7C57e1b682%7C24173980%7C12902222%7C6361244CAC88236A26B208BD631DD9E9&s=F57nWjsTROFXmehRfoLNLtcLIwk&o=%2Fphti%3A%2Fgtsmbthhco%2Fu.i-omtssseae%2Fanpor%2Ftc3%2Ful%23376l6muisecomse51nt0984-2468, or unsubscribehttps://url10.mailanyone.net/scanner?m=1oprfY-0002Mw-5w&d=4%7Cmail%2F90%2F1667310600%2F1oprfY-0002Mw-5w%7Cin10g%7C57e1b682%7C24173980%7C12902222%7C6361244CAC88236A26B208BD631DD9E9&s=SnK25W4TXg6091labaPw4q6a4FA&o=%2Fphti%3A%2Fgtsmbthnco%2Fu.tfotocaiiibunscsus%2Fnteri%2Fauhb-5LA2TTNUJD3DFYIZOMPL3WAFAENFTGLFNC3M5RNSAKX6. You are receiving this because you are subscribed to this thread.Message ID: @.***>

Wolfir77 avatar Nov 01 '22 14:11 Wolfir77

Should i open a new pull request if I want to pick up this integration (since i cannot contribute to this branch)?

MSiegrist avatar Nov 21 '22 14:11 MSiegrist

Hey all,

sorry for the long delay... had not that much time in the last weeks. Of course i am still interested in moving forwarding. Really appreciate the feedback so far.

Should i open a new pull request if I want to pick up this integration (since i cannot contribute to this branch)?

Would appreciate if you wish to contribute to the code. Can you guide me whats needed to grant you the required permissions? (sorry for the noob question :) )

I will try to investigate the feedback/recommendations given so far and update this request

Vip0r avatar Dec 06 '22 20:12 Vip0r

I've already worked on most of the feedback. See my linked PR (#82860). There are only a few things left (see Todos). You could merge my changes once you work on it again.

MSiegrist avatar Dec 08 '22 09:12 MSiegrist

Hello @frenck,

i adjusted the sections based on your recommendations. Kindly ask you to check if we can proceed. There some possible further improvements recommended by @MSiegrist which i documented in the initial post. Maybe we should avoid adding additional features in the initial pull request.

What do you think? Are we good to go?

Vip0r avatar Dec 10 '22 06:12 Vip0r

Documentation is also ready at https://github.com/home-assistant/home-assistant.io/compare/current...Vip0r:home-assistant.io:current

Vip0r avatar Dec 10 '22 07:12 Vip0r