Draft: Better Support for Solarflow Batteries without Cloud

[vaterlangen]

This MR adds a new battery provider (index 7) to support Zendure Solarflow batteries using local MQTT broker.

Preconditions

In order to use this integration, the Solarflow system has to be "disconnected from cloud" (aka. configured to use the local MQTT broker instead of the pre configured vendor cloud). Please refer https://github.com/reinhard-brandstaedter/solarflow-bt-manager for details. If one can adjust answers on DNS queries (e.g. using OpenWRT as router), Solarflow can also be "disconnected from cloud" by returning the local broker IP for requests to mqtteu.zen-iot.com. The broker should be configured without authentication - or one has to gather the username passoword used by Solarflow.

Features

• Query battery status from Solarflow
• Display stats on LiveView
• Publish stats via MQTT
• HASS auto-discovery
• Auto detection of AB1000 and AB2000 packs (currently only tested with my AB2000)
• Time-sync for Solarflow
• Sending settings to Solarflow
• Set Zendure output limit fixed or via schedule (relative to sunrise/fall)
• Configurable full charge intervals for battery (or better: BMS) conditioning

Main Page with battery data read from Solarflow

Configuration of new battery provider

Testing

Tested with my local setup consisting of

• 1x HUB1200 / HUB2000
• 2x AB2000
• 1x HMT-2250
• Mosquitto as local MQTT broker

[vaterlangen]

Removed accidentally pushed stuff from branch

[AndreasBoehm]

I am wondering why we need this PR if there is already support for Zendure Batteries after this PR (#1127) has been merged? Or is that a different way to setup the battery?

[vaterlangen]

With this PR, all battery data is read and displayed (as shown in the screenshot) - not only voltage and soc as implemented by #1127.It iis intended for Solarflow instances, that are not connected to the vendor cloud (but using local MQTT broker, as this requires sending some commands to the device, too) and not using the vendor App. On the settings page, one can already adjust some parameters, as maximum output power and soc limits.

As I discovered a better way to read data from the device, I'm currently reworking the code.

[vaterlangen]

Initial post completely updated and ready for review

[schlimmchen]

Let's get this ready, shall we?

I took the liberty to clean this PR up. Since we let it stew for so long, @vaterlangen had to merge hoylabs/development several times. Rebasing is now impossible, but merging is not an option.

git diff hoylabs/development vaterlangen/feature/zendure_solarflow | git apply -

That command is what I used and I have very high confidence in git that it did exactly what I wanted.

However, I am unable to force-push to vaterlangen/feature/zendure_solarflow, even though maintainers are supposed to be allowed to edit this PR and even though I was able to edit the branch before, around 3 months ago. @vaterlangen Did you do something to prevent me from changing this branch again?

I then started a more in-depth review. There are some oddities:

• std::optional<std::shared_ptr<>> does not make any sense to me. Use nullptr if no object can be returned.
• You seem to be eager to use move semantics to avoid copying stuff, to a point that makes we wonder "does this even improve anything?", but then there are lines that obviously can be improved regarding the same thing.
• Please don't add dead code, i.e., commented-out code. Ah, I see you want to make values auto-discoverable for all packs, but packs are a volatile thing... Well... Let's sleep one more night over this. Definitily getPackDataList() must return a const&, not a copy.
• You often (always?) are missing a space between a closing bracket and an opening curly brace... This is bugging me hard^^
• You use structured binding, something new to me. Nice to learn about this, I have not had the opportunity to catch up on C++17 stuff, let alone C++23. However, then you proceed to use only the index to retrieve the element you already have as entry, which by the way is a copy. The problem with finding stuff like this is the following: You used a concept that makes sense in that context, but you obviously have not fully understood them, but applied them anyways. That in turn makes me doubt the rest of the code... I see why you did this: you want to modify entry, which did not work, because your structured binding expression yields a copy.
• The readability of your code can be significantly improved by un-indenting your code

I started refactoring/updating the code to bring it to a level that I am happy to merge. I did not push that, yet, and will continue shortly.

[vaterlangen]

@schlimmchen Thanks for driving this forward!

No, I did not change anything on the repo settings. I just disabled and re-enabled maintainer edits. Please try again.

Thanks for your comments:

• oh, yes. That's right - optional is just generating overhead...
• that was the same I thought...but not coming with a solution
• Sorry for bugging you, that was not intended
• The loop was originally designed using both, index and object - as it is now using the object only, this can be done by a range-based loop
• And again, you're right. "Some" spots could be optimized this way

Thanks for cleaning my mess up...

I also addressed some points an pushed them to feature/zendure_solarflow_upd - just in case I can reduce your work by this

[vaterlangen]

@schlimmchen Updated to latest dev - the old branch is still available as feature/zendure_solarflow_backup
If you focus on #1451 right now, please let me know if I should start migrating this branch into the new structure

[schlimmchen]

We would have merged that already if it wouldn't conflict with your work. So we talked about it again this week and decided to merge the restructure first, as we then can review/fine-tune your work when migrating it to the new structure, in one go. If you could do that yourself, that would be great!

Please understand that it is hard to prioritize reviewing other people's work over working on one's own ideas. That is the primary reason this PR (and Niko's in particular) are... neglected.

If you would rebase on top of the refactor I would merge your PR without too much scrutiny.

[vaterlangen]

I'm absolutely fine with it. I'll point this MR to 1451 and set it to draft till I've migrated everything to the new structure. Are there further updates on 1451 expected, I should wait for before starting?

[schlimmchen]

Nope, please go ahead. We were waiting to merge it as I wanted to let it settle in that this is what we want and and how we want it. Also, I did not want it to become part of the latest releases yet. I think now is a good time to just do it. We already have the solar charger integration restructured based in the same scheme.

[schlimmchen]

Oh, sorry! I forgot that you changed the target branch and that merging that target branch now closed this PR...

Well... I will try to fix that from my mobile, or I will do it tomorrow.

Is your changeset ready for review?

[vaterlangen]

Ok, I also tried from my mobile - but did not get it done.
Unfortunately, I still have an ugly WDOG restart by async_tcp task open to fix. ATM I can only spend time at the evening and battery is empty most of that time....so testing is difficult...

[schlimmchen]

Nope, this PR is closed for good. No worries. Please open a new one: Rebase your branch onto the current development branch, then open a new PR. If you struggle with that, I am happy to help.

Unfortunately, I still have an ugly WDOG restart by async_tcp task open to fix.

Is this reproducible? If yes, I may be able to help if you described what is needed to trigger the watchdog.

[0lini]

I would be open to help with testing. My setup consists of a HM-800 with a Solarflow Hub 1200 connected to an AB1000. I redirect requests to a local MQTT server.

[vaterlangen]

I would be open to help with testing. My setup consists of a HM-800 with a Solarflow Hub 1200 connected to an AB1000. I redirect requests to a local MQTT server.

Great! Just install the updated FW and check if your setup with AB1000 is working without FW reboots
I just rebased to development and opened the new MR #1668

[vaterlangen]

Nope, this PR is closed for good. No worries. Please open a new one: Rebase your branch onto the current development branch, then open a new PR. If you struggle with that, I am happy to help.

Done and MR opened as #1668

Is this reproducible? If yes, I may be able to help if you described what is needed to trigger the watchdog.

It always happens when OpenDTU was restarted and the Website was opened (at least when using Wifi - not sure if it would also happen with LAN). For some magical reasons, this was not happening any more when I tried to find the reason. These damn bugs, they are always hiding when you are near...