Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Better Support for Solarflow Batteries without Cloud #1221

Draft
wants to merge 1 commit into
base: schlimmchen/battery-restructuring
Choose a base branch
from

Conversation

vaterlangen
Copy link

@vaterlangen vaterlangen commented Sep 3, 2024

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

grafik

Configuration of new battery provider

grafik

Testing

Tested with my local setup consisting of

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

@vaterlangen vaterlangen force-pushed the feature/zendure_solarflow branch 2 times, most recently from 51137eb to 05c8b89 Compare September 3, 2024 17:25
@vaterlangen
Copy link
Author

vaterlangen commented Sep 3, 2024

Removed accidentally pushed stuff from branch

@vaterlangen vaterlangen marked this pull request as draft September 8, 2024 22:25
@AndreasBoehm
Copy link
Member

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
Copy link
Author

vaterlangen commented Sep 11, 2024

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 vaterlangen changed the title Support for Solarflow Batteries Better Support for Solarflow Batteries without Cloud Sep 11, 2024
@vaterlangen
Copy link
Author

vaterlangen commented Sep 14, 2024

Initial post completely updated and ready for review

@vaterlangen vaterlangen marked this pull request as ready for review September 14, 2024 21:28
@schlimmchen schlimmchen force-pushed the development branch 2 times, most recently from 91cc2fc to 8ff94e7 Compare September 16, 2024 14:10
@schlimmchen
Copy link
Member

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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

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?

@vaterlangen vaterlangen changed the title Better Support for Solarflow Batteries without Cloud Draft: Better Support for Solarflow Batteries without Cloud Jan 25, 2025
@vaterlangen vaterlangen marked this pull request as draft January 25, 2025 13:04
@schlimmchen
Copy link
Member

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.

@vaterlangen vaterlangen changed the base branch from development to schlimmchen/battery-restructuring January 25, 2025 21:02
@vaterlangen vaterlangen force-pushed the feature/zendure_solarflow branch from 11f726f to 6d1cc39 Compare January 25, 2025 21:17
@vaterlangen vaterlangen force-pushed the feature/zendure_solarflow branch from 6d1cc39 to ad43b6e Compare January 25, 2025 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants