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

Added option to limit AC charger power based on battery SoC #449

Closed
wants to merge 11 commits into from
Closed

Added option to limit AC charger power based on battery SoC #449

wants to merge 11 commits into from

Conversation

eu-gh
Copy link

@eu-gh eu-gh commented Sep 18, 2023

My personal setup includes the Huawei AC charger connected to a Pylontech battery, both of which are connected to the OpenDTU via CAN bus. I'm also using the option for automatic power control of the charger based on Power Meter readings, whichs works pretty good most of the time.

However, on a sunny day, the current implementation keeps charging the battery at the maximum set power until it is (almost) full, which is probably not really helping the cells long-term health. At least the Pylontech's BMS reports a maximum current of 10A at a SoC above 89% instead of the usual 25A.

So I added an option to the charger settings which lets the user enter a SoC threshold and also a second, reduced maximum power value if that SoC is reached. After I spend several hours on being able to actually see these new options on my machine (including the realization that the PlatformIO build does indeed not rebuild the WebApp and the journey to getting the whole NodeJS/NPM(Corepack/Yarn stuff to work), I was also successfully able to do a test run.

Looks like this:
grafik

@@ -14,6 +14,8 @@ dist-ssr
coverage
*.local
vite.user.ts
.yarn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has nothing to do with the feature described in this PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, but I didn't know any other way to avoid pushing the 400+ cached yarn packages into the repo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment below regarding the git staging area and selecting parts of your workdir to be included in the next commit.

@@ -777,12 +777,17 @@
"Configuration": "AC Ladegerät Konfiguration",
"EnableHuawei": "Huawei R4850G2 an CAN Bus Interface aktiv",
"EnableAutoPower": "Automatische Leistungssteuerung",
"EnableReducePowerOnBatterySoC": "Leistung abhängig vom Batterie-Ladezustand reduzieren",
"EnableReducePowerOnBatterySoCHint": "Wenn eine Batterie per CAN-Bus verbunden ist, kann die maximale Ausgangsleistung ab einem bestimmten SoC reduziert werden",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work for all kind of battery interfaces (Pylontech via CAN, JK BMS via serial, or Victron SmartShunt via Serial, or in the future also via MQTT). Please change the wording accordingly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do 👍

</div>

</div>
</CardElement>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The div in this CardElement is not properly indented. Why is it indented two or three levels to many?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly don't know, but I will change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm? Now the diff looks way to big... There are now changes to pats that are not concerned with this feature.

Copy link
Author

@eu-gh eu-gh Oct 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely cosmetic, I added another <div> layer so the input fields will disappear altogether if the charger is disabled.

webapp/yarn.lock Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not include changes to this file in your PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I do this without altering the included .gitignore file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not include changes to webapp_dist folder in your PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I do this without altering the included .gitignore file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not git add any file in webapp_dist even if it is marked as modified. Or in other words: When crafting a commit, do not include webapp_dist. You question seems to suggest that you don't know what the git staging area is and how to use it. You are enabled to select particular files and even particular changes within a file for the next commit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it now, the GitHub client was adding all changed files by itself per default. Thanks!

newPowerLimit = config.Huawei_Auto_Power_Reduced_Upper_Power_Limit;
}
}
// Limit power to maximum
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing an empty line before this comment, and comment was indented by accident. It should actually not change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

if (config.Battery_Enabled && config.Huawei_Auto_Power_Reduce_On_BatterySoC_Enabled){
uint8_t _batterySoC = Battery.getStats()->getSoC();
if (_batterySoC >= config.Huawei_Auto_Power_BatterySoC_Threshold && newPowerLimit > config.Huawei_Auto_Power_Reduced_Upper_Power_Limit){
MessageOutput.printf("[HuaweiCanClass::loop] Current battery SoC %i reached threshold %i, reducing output power to %f \r\n", _batterySoC, config.Huawei_Auto_Power_BatterySoC_Threshold, config.Huawei_Auto_Power_Reduced_Upper_Power_Limit);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How often will this appear once the threshold is reached? In every call to loop()? Or is this section of the function only reached with a fixed interval? I am asking to make sure that this does not flood the serial line or the web console.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does get called regularly when the battery SoC threshold is reached, but since all other outputs in this function are like this, it doesn't really make a huge difference.

Personally, I would like to add a verbose logging switch to this settings page and link most console outputs to it, but I didn't get around to it yet.

@MalteSchm
Copy link

MalteSchm commented Sep 23, 2023

Hi @eu-gh

Would it not be better to read the maximum current from the BMS, subtract the current that a Victron charger delivers and use this as maximum current for the Huawei charger?
This was on my mind but I have not yet had the time to implement this.

This makes this feature work without any user interface action and is a solution that just works without figuring out the SoC values manually (the correct max current will also depend on the battery temperature and cell imbalance so it is not gonna be correct if set manually)

BTW: I created a draft PR here that fixes the CAN communication bus issue: #454

@helgeerbe
Copy link
Collaborator

What is the state of this PR? I will convert it to draft, until someone informs me, that it is ready to merge.

@helgeerbe helgeerbe marked this pull request as draft September 28, 2023 11:12
@eu-gh
Copy link
Author

eu-gh commented Oct 1, 2023

@MalteSchm

Would it not be better to read the maximum current from the BMS, subtract the current that a Victron charger delivers and use this as maximum current for the Huawei charger? This was on my mind but I have not yet had the time to implement this.

This was my first intention too, but if I've seen this correctly, only the Pylontech BMS delivers this kind of data, which would make this feature exclude the two additional battery interfaces we gained in the meantime.

This makes this feature work without any user interface action and is a solution that just works without figuring out the SoC values manually (the correct max current will also depend on the battery temperature and cell imbalance so it is not gonna be correct if set manually)

Albeit these are both valid points, non of the stuff we do here is production-grade or even standardized by any means, so I believe that the possibility to set options manually is more of a pro than a con. Also, the use cases are varying heavily.

Two personal examples:

  • A stack of two US2000Cs wired in parallel configuration -> can take up to 50 amps, but (each) BMS reports only 25, so the charging current would be unnecessary limited
  • Distributing the available energy dynamically to multiple endpoints, e.g. a car charger -> this is much easier to do if the storage battery limits its charging power by itself

Also I would like to add a feature to stop charging at a certain SoC altogether, but that's for a different PR.

@helgeerbe
I've been using my personal build with this feature enabled for the last two weeks now and from my point of view, it's a really nice, simple addition without any real drawbacks. I don't see why this shouldn't be merged, but it is not on me to decide this.

@eu-gh
Copy link
Author

eu-gh commented Oct 4, 2023

Superseded by #480

@eu-gh eu-gh closed this Oct 4, 2023
Copy link

github-actions bot commented Apr 4, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants