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

Support for DalyBMS #672

Draft
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

xpertsavenue
Copy link

@xpertsavenue xpertsavenue commented Feb 13, 2024

Hello,

First of all, thank you for the great project!

My battery is equipped with a DalyBMS, for which the serial messages are also documented and yet there are already some projects that implement this function for the ESP8266. Long story short, I don't want to use 2 separate ESP µCs to monitor the battery and host this project. So I mixed the Daly2Mqtt project with this OpenDTU-OnBattery and was able to get data from the DalyBMS - even with my limited programming skills.

2024-02-13 13_05_05- 2024-02-13 13_05_34-OpenDTU-onBattery 2024-02-13 13_07_45-MQTT Explorer

Working:

  • Get serveral Data from the DalyBMS with a interval of 5s
  • Values send to MQTT
  • Values are displayed in the UI

Not working:

  • I also tried to change the UI to add this but unfornatly I could only run the project with "yarn dev". "Yarn build" is also working but the compiled website is unforunatly not included when I build and upload it to the ESP32. How to pack those files to the the .bin?

Improvments:

  • Switching the Charge and Discharge MOS is possible but I have no idea how to make this possible from the UI
  • Waking up the BMS requires just an optocoupler and a free microcontroller pin --> can be easily added but is not done yet

This is the first time I worked with C in such a big project, so I guess not everything is efficent and matches the style of like programming should be done. Feel free to change and add things if you are an expert!

@schlimmchen
Copy link
Member

This looks very promising! It's not clean and I found issues from scrolling the changes, but in general it looks very promising!

As a first step, could you double-check that functions/methods which are not used/required are removed from the code base?

And you have to revert the line-endings change to the french locale. That used to use CRLF, and you should not change it, as it becomes a nightmare to merge upstream changes.

How to pack those files to the the .bin?

Build the web application using "npm run build", then re-compile the firmware, which will pick up the respective files. Do NOT add any webapp files to this changeset.

I don't agree that the cell voltages belong into the live view. Publish them to MQTT, that's fine. You added labels for up to 24 cell voltages, but will not include any in the live view if there are more than 8 cells?!

@xpertsavenue
Copy link
Author

This looks very promising! It's not clean and I found issues from scrolling the changes, but in general it looks very promising!

As a first step, could you double-check that functions/methods which are not used/required are removed from the code base?

I did - how can I push the new code to this pull request? Or is it automatically in this one? I could also close this one and made a new one but I think there is a smart way do add this also to this existing one. The latest code is in my repository.

And you have to revert the line-endings change to the french locale. That used to use CRLF, and you should not change it, as it becomes a nightmare to merge upstream changes.

I have no idea how this happened, but I reverted the file back.

Build the web application using "npm run build", then re-compile the firmware, which will pick up the respective files. Do NOT add any webapp files to this changeset.

Thanks, this has worked for one time and now its again not working. "npm run build" shows no errors, the "Build" / "Upload" task also not.

I don't agree that the cell voltages belong into the live view. Publish them to MQTT, that's fine. You added labels for up to 24 cell voltages, but will not include any in the live view if there are more than 8 cells?!

I agree, its nice to see that during debugging but for the daily view on the dashboard its not necessary. The reason why I have limited it to 8 cells was that the whole battery-info-box disappeared when there where to much sensors listed.

@schlimmchen
Copy link
Member

I did - how can I push the new code to this pull request?

Just push them into your remote branch "xpertsavenue:development". You did so already, you pushed b4fc542 and it showed up here.

The reason why I have limited it to 8 cells was that the whole battery-info-box disappeared when there where to much sensors listed.

That's because the JSON buffer overflowed in the firmware when preparing the data for the webview. I think the buffer just runs out of space and the resulting JSON is not valid JSON, so the web application discards all of it.

@helgeerbe helgeerbe marked this pull request as draft February 19, 2024 12:40
@helgeerbe
Copy link
Collaborator

What is the state of this PR?

@schlimmchen
Copy link
Member

@helgeerbe In case you mind my opinion: It is not ready. There are issues that need to be addressed before this can make its way into the repo. At least the french locale line endings need to be fixed. The change to webapp/vite.config.ts must not be part of the changeset. The updated web app must not be part of the changeset.

I have ordered a very small and cheap Daly BMS to test and possibly help polish this changeset.

@helgeerbe
Copy link
Collaborator

@schlimmchen Thanks for your feedback. I already marked this PR as draft.

@schlimmchen
Copy link
Member

Well, the BMS came with DHL today and I jumped on it. I hooked it up to my benchtop battery pack and was quite disappointed by the Daly BMS app. I then soldered an ADUM1201 board to a respective connector and hooked it up to a spare ESP32 board. I rebased this branch onto the current helgeerbe/development branch, which gave me some minor troubles, mainly because of my own #679. I installed a pin_mapping.json and was pleasently surprised that data arrived immediately.

Kudos to @xpertsavenue!

image
The data age is not reset for some reason. I guess this might be my own fault when resolving the merge conflicts.

The data is not refreshed until the browser is refreshed. That confuses me. This must be investigated and fixed.

"Charging possible" and "Discharging possible" shows numbers rather than text.

As I suspected, there are unused functions in the code. They should be removed. When we want to implement setting BMS state, we can always copy again from the respective project.

Hm... Receiving data happens synchronously after sending the request and the whole loop() method will stall (and with it the main loop) until timeout if the connection breaks down. That's a major issue and needs a change in the design of DalyBms.cpp.

deinit() is a stub, but it at least needs to call HwSerial.end().

Reading any alarm bit is commented out?

The whole get structure is only there to cache data from one function to another... We really should write directly to the respective DalyBatteryStats class and get rid of the get structure altogether.

So, this definitely is a good starting point, but the feature definitely needs some more love. @xpertsavenue Are you up to address some of the issues yourself?

@xpertsavenue
Copy link
Author

Great to hear that it is not only working for my BMS!
I working currently working on a few things that I think i'm able to fix. But I'm sure I can not fix all things as I have limited programming skills.
I will commit an update this weekend.

@xpertsavenue
Copy link
Author

Hey guys - today something unplanned happened and during a test I destroyed my DalyBMS. Stupid thing, the BMS don't like a short circuit on the UART port between 3.3V and GND.
However, I still thinking about changing to a JK BMS as this is already good supported but I made no final decision yet. But no matter of my decision I can not continue to work on it.
I already changed a few of the things @schlimmchen mentioned:

  • "Charging possible" and "Discharging possible" shows numbers rather than text.

Should be fixed and renamed

  • As I suspected, there are unused functions in the code. They should be removed. When we want to implement setting BMS state, we can always copy again from the respective project.

I removed some more code lines -except a few that I would say should be somehow implemented, e.g. switching on the Charge/Discharge Mosfets

deinit() is a stub, but it at least needs to call HwSerial.end().

I added the HwSerial.end()

The whole get structure is only there to cache data from one function to another... We really should write directly to the respective DalyBatteryStats class and get rid of the get structure altogether.

Done.

So all of the changes are not finally tested as I destroyed the BMS. Also I think the async stuff and also the failure codes are exceeds my C++ skills.

So please feel free to take the code, test and extend the code.

@schlimmchen
Copy link
Member

However, I still thinking about changing to a JK BMS as this is already good supported

After fiddling with the Daly, I encourage you to do so, especially since your Daly is now fried. I also watched a few videos and was happy that I made a good call going with the JK BMS.

So please feel free to take the code, test and extend the code.

I would be happy to pick it up from here. Despite your fruitful efforts, there is still work to do. The synchronous receive logic needs to be refactored for sure.

Who knows whether or not this type of BMS is actually popular among OpenDTU-OnBattery users? @spcqike? @helgeerbe?

@der-peterh
Copy link

Well I'm using a Daly BMS and would really appreciate a single project in order to keep the amount of µControllers to manage my installation rather small.

@flofresser
Copy link

flofresser commented Feb 29, 2024

I also use a Daly BMS. The integration in OpenDTU on Battery would be awesome

@spcqike
Copy link

spcqike commented Feb 29, 2024

Who knows whether or not this type of BMS is actually popular among OpenDTU-OnBattery users?

sry for my late answer, i havn't noticed the direct question.

well, i don't know. afaik the JK BMS is more common (at least i read more questions and stuff about this), but thats probably because its integrated allready.

as i don't have a battery yet and if i'm going to buy one, it most likely will be a prebuild pylontech, i haven't put to much focus on the BMS hardware discussions :) maybe we create a poll and just ask the community?

as i see, there are at least 2 people using it 😄 so there are happy early access testers ;)

@schlimmchen
Copy link
Member

as i see, there are at least 2 people using it 😄 so there are happy early access testers ;)

Yes, and two writing means there are 20 who would be using it. That motivates me. Unfortunately, I put other issues on my list that I find are more important. xpertsavenue did a great job, but the feature is just not ready in this state.

@Feldsalat
Copy link

Habe ebenfalls Daly-BMS in Benutzung, wäre eine schöne Sache es hier integriert zu haben.

@schlimmchen schlimmchen force-pushed the development branch 2 times, most recently from 91cc2fc to 8ff94e7 Compare September 16, 2024 14:10
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.

7 participants