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

Jbd bms support #1218

Merged
merged 4 commits into from
Oct 30, 2024
Merged

Jbd bms support #1218

merged 4 commits into from
Oct 30, 2024

Conversation

MoleBre
Copy link

@MoleBre MoleBre commented Sep 1, 2024

First of all: also my thank to all the developers. You are doing a great job. This project is awesome.
Please be patient with my skills. Its my first time contributing to a project on github and it took me some time to understand the code more or less :-).

I copied the the jk-bms code and adapted it to the jbd-bms uart communication protocol as specified by the manufacturer.

The code is working with my setup via the rs485 interface of my jbd bms:

This pull request is related to issue #467.

BatterySettings
LifeView-LifeData
LifeView-LifeData-JBD-BMS

@MoleBre MoleBre marked this pull request as draft September 7, 2024 12:07
@schlimmchen schlimmchen linked an issue Sep 7, 2024 that may be closed by this pull request
@schlimmchen
Copy link
Member

Thanks for this contribution!

I looked at this some time ago and noticed that it needs some polishing. Minor stuff (commented code should be cleaned up, spaces/indention).

The other thing that is a little bit offputting is this:

I copied the the jk-bms code

However, I am not sure if I am willing to put in the time and effort to generalize and share code. It would be the right thing to do, though. I can also see that there are significant differences, so clearly, there will be a lot of new code. We could generalize the DataPointContainer using a template (the DataPoint's std::variant go into the template parameters, and the DataPointContainer needs at least the label as template parameter...). I think that would be a good idea.

You marked this as draft a couple of hours ago. Why is that? because of the conflicts in webapp_dist? Just clean webapp_dist and you are good, at least regarding the conflicts.

@MoleBre MoleBre marked this pull request as ready for review September 8, 2024 10:37
@MoleBre
Copy link
Author

MoleBre commented Sep 8, 2024

Hi Schlimmchen. Ich antworte mal kurz auf Deutsch.

Vielen Dank erstmal für deine Rückmeldung!

Den PR-Status "Draft" und "Open" hatte ich falsch verstanden. Ich habe es jetzt wieder auf "Open" zurückgeändert.

Ja, da hast du recht. Den Code aufzudoppeln ist nicht schön. Anders hätte ich deinen Jk-Bms Code jedoch anpassen müssen, damit ich ihn für das Jbd-Bms wiederverwenden kann. Das habe ich mich bei meinem ersten PR nicht getraut. Ich habe hier kein Jk-Bms zum testen und ich wollte keine Bugs in euren bestehenden Code einbauen. Das Risiko war mir zu hoch.

Ich habe die Test-Kommentare gelöscht und den Commit mit den Webapp-Files zurückgenommen. Die falschen Einrückungen konnte ich aber bei mir in VS Code nicht finden.

Gebe mir gerne Bescheid, wenn ich noch etwas anpassen soll.

BG.

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.

I'll tackle the required changes.

src/JbdBmsController.cpp Outdated Show resolved Hide resolved
src/JbdBmsController.cpp Outdated Show resolved Hide resolved
webapp/src/locales/de.json Outdated Show resolved Hide resolved
webapp/src/locales/de.json Outdated Show resolved Hide resolved
webapp/src/views/BatteryAdminView.vue Outdated Show resolved Hide resolved
src/BatteryStats.cpp Outdated Show resolved Hide resolved
src/JbdBmsDataPoints.cpp Outdated Show resolved Hide resolved
include/JbdBmsDataPoints.h Outdated Show resolved Hide resolved
@schlimmchen
Copy link
Member

Rebased onto current development and de-deplicated DataPointsContainer.

@schlimmchen
Copy link
Member

@MoleBre Ich wollte mergen, als mir aufgefallen ist, dass die HASS autodiscovery fehlt. Kannst du da bitte zumindest die wichtigsten Werte nacharbeiten? Siehe MqttHandleBatteryHass.cpp.

@schlimmchen
Copy link
Member

schlimmchen commented Sep 14, 2024

Zwei Drei Dinge sind hier noch zu tun:

@schlimmchen schlimmchen force-pushed the development branch 2 times, most recently from 91cc2fc to 8ff94e7 Compare September 16, 2024 14:10
@schlimmchen schlimmchen self-assigned this Oct 8, 2024
@schlimmchen
Copy link
Member

Tested successfully against a SP22S003AL22S40A with firmware version 6.7

Issues:

  • BatteryOneTemperature is actually the board's temperature. This might be specific to my model, so I am not changing it.
  • There should be a third temperature sensor value, but it seems it is not communicated over the serial connection.
  • Currents below ~800mA are not detected by the BMS at all (not an OpenDTU-OnBattery problem, just an observation).
  • Could not test all alarms, but some, and also multiple at once.
  • Did not check all HASS values, but some, and I assume the rest are good (maybe there is a typo somewhere...)

@schlimmchen schlimmchen merged commit c55ff7d into hoylabs:development Oct 30, 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 30, 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.

[Request] Support for JBD BMS
2 participants