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

Add SoC threshold for battery current limit. #1293

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

ranma
Copy link

@ranma ranma commented Sep 28, 2024

I'd like to only limit the discharge current below a configurable state of charge (in my case currently "limit to base consumption below 70% SoC").

In principle the current limit could also be extended to support multiple SoC/voltage thresholds for different current limits, which might be useful for users of "dumb" batteries to approximate the multiple steps of discharge current reduction as the voltage or SoC approaches "empty". However currently this only adds the SoC threshold for the single current limit.

@ranma
Copy link
Author

ranma commented Sep 28, 2024

image

@ranma ranma force-pushed the discharge-current-soc-limit branch from 53070c8 to 242613d Compare September 28, 2024 16:36
@spcqike
Copy link

spcqike commented Sep 28, 2024

Do you thinking it’s possible to respect the „ignore SoC“ switch? (Does the switch still exist? I thought so but haven’t found it without a battery (: )
So to either hide this SoC threshold here or have a voltage threshold, when SoC should be ignored.

@ranma ranma force-pushed the discharge-current-soc-limit branch from 242613d to 08dbfea Compare September 28, 2024 19:42
@ranma
Copy link
Author

ranma commented Sep 28, 2024

Do you thinking it’s possible to respect the „ignore SoC“ switch?

Added a check for "ignore SoC" and also a voltage threshold setting.

Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

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

Looks good to me, however, I want #1294 merged first, then I will squash and rebase this onto development. Also, I will then change the web UI (1) to use wide input field labels and (2) to hide elements which do not apply until "enable interface" is checked.

@schlimmchen schlimmchen self-assigned this Oct 8, 2024
@AndreasBoehm
Copy link
Member

I merged development into this branch so you can go ahead with your adjustments @schlimmchen without worrying about merge conflicts.

@AndreasBoehm AndreasBoehm force-pushed the discharge-current-soc-limit branch from 1f06871 to 25f1902 Compare October 23, 2024 17:42
@AndreasBoehm
Copy link
Member

Looks good to me, however, I want #1294 merged first, then I will squash and rebase this onto development. Also, I will then change the web UI (1) to use wide input field labels and (2) to hide elements which do not apply until "enable interface" is checked.

Is this what you had in mind @schlimmchen ?

Screenshot 2024-10-23 at 19 54 40
Screenshot 2024-10-23 at 19 54 33

@schlimmchen
Copy link
Member

Ah, those are two different screenshots... In that case, yes, I guess, except that the hint is also not applicable if the battery interface is disabled. I'll tell you when you push the respective changes to the webapp 😉 It's probably correct.

@AndreasBoehm AndreasBoehm force-pushed the discharge-current-soc-limit branch from b5875ab to 8bb5ac5 Compare October 23, 2024 19:02
@AndreasBoehm
Copy link
Member

Changes pushed :)

@schlimmchen
Copy link
Member

  • Just wide, not wide="true".
  • All labels (of a page) should be wide, not only those in the discharge current limit card.
  • "Use Battery-Reported lmit" is only supported by PYTES and Pylontech, afaik. In that case we should only show this option when either of those two data providers are selected.

@AndreasBoehm
Copy link
Member

  • "Use Battery-Reported lmit" is only supported by PYTES and Pylontech, afaik. In that case we should only show this option when either of those two data providers are selected.

Good catch, but the MQTT Battery Provider should be allowed as well to enable the topic input and I adjusted the SBS Battery provider to provide the discharge current limit in the correct way and added it to the list of allowed providers as well.

@schlimmchen
Copy link
Member

image

What "Ignore Battery SoC" setting -- will a bunch of people ask. This cross-dependency is... unfortunate. Well, we are not going to solve this overnight. Let's clarify in the tooltip that a DPL setting is meant here. In the long run, all battery-related settings (DPL thresholds and the "ignore SoC" setting in particular shall move to the battery. Other people suggested this as well already.

image

I suspect the condition for the hint is not correct. I switched to MQTT as the provider, enabled "use battery limits", then switched to JK BMS. Add a template-tag (or a div) around the "use battery limit" switch and the hint, then make that container dependent on the provider. Ah, wait. The input switch for "use battery limit" should move to the template below it, and that template should have the check for the provider setting. Do you know what I mean?

@AndreasBoehm
Copy link
Member

I adjusted the texts and fixed displaying the alert for wrong providers.

AndreasBoehm and others added 3 commits October 24, 2024 14:52
SBS CAN receiver implementation was not using the correct way to provide
discharge current limit.
This changes the custom current limit so the custom limit is only
applied when any of:

- SoC is valid and not ignored and SoC < threshold
- Voltage is valid and Voltage < threshold
- Voltage is invalid

Independently, if "Use Battery-Reported limit" is enabled and valid, it
is applied (unless a lower custom limit already was applied).
* use wide labels for all battery settings
* dynamically show and hide valid battery discharge limit settings
@schlimmchen schlimmchen force-pushed the discharge-current-soc-limit branch from 6537d9d to 77283ca Compare October 24, 2024 12:54
@schlimmchen schlimmchen merged commit cfb5c3f into hoylabs:development Oct 24, 2024
9 checks passed
Copy link

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 Nov 24, 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