incorrect args passed to OverkillSolarBms::write() in bms.cpp

[jbatx]

In debugging bms.cpp for esp32, I noticed that all calls to OverkillSolarBms::write() are passing the length and data args in the reverse order from the function definition on line 528. I assume this is a bug. However, I haven't done any c++ since about 1998 and admit that could totally be missing something.

Call example from 313:

write(true, BMS_REG_NAME, length, data);

function definition from 528:

OverkillSolarBms::write(bool read, uint8_t command_code, uint8_t* data, uint8_t length) {

[jbatx]

Update... Along with above fixes, the calls to min(.....) need to be changed to min<int>(.....). After that, lib will compile for esp32 in the Arduino IDE v1 and v2. I am not able to create a branch and PR

[FurTrader]

Sorry, the guy that wrote the library isn't available and I personally don't know how to use github correctly.  Can you give me a ELI5 of what you've been doing, and how I can help? -SteveOverkill Solar LLC On Thursday, September 23, 2021, 12:09:13 PM EDT, Jeremy Brooks ***@***.***> wrote: Update... Along with above fixes, the calls to min() need to be changed to min(). After that, lib will compile for esp32 in the Arduino IDE v1 and v2. I am not able to create a branch and PR — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

[jbatx]

Hey Steve, No worries at all. ..I had to look up ELI5. :-). The esp32 is a chip made by Espressiff and also a family of boards that are compatible with the Arduino IDE. They're dual core and have a realtime OS on board. They also have three UARTs. Very cool stuff, IMO. I'm not a cpp programmer - I'm a hack at best with it. Fixing this lib was the first time I've messed with cpp since maybe '99 in school. Also, this is my first arduino project. I've been a software dev/hacker for 20 years though. For the software part, here's the situation - I think this is now fixed in my local version. 1. The lib needed a code improvement - the min<int>() thing. I think this snuck by because the tested Arduino board's compiler options in the Arduino IDE allows for some sloppiness whereas the more modern/advanced boards like esp32's options do not. 2. The lib had a bug where the data and length parameters were totally reversed. That is, the data was being passed as length and length as the data. I can't imagine that the lib ever could have worked like this... maybe, I don't know 3. If I have permission to create a branch on the repo, I can submit the new code for review (Pull Request). *For the hardware part....* I'm working with an Overkill 16s 48v - the one you all just shipped to me a couple weeks ago. This is my second one. I cannot yet get the esp32 and bms to talk. However, I think I know the solution. You may be able to validate this. *Hypothesis*: fact: The esp32 UART is 3.3v tx and rx. fact: The bms is 5v rx and tx. I believe that the esp32 signals are not registering with the bms because the bms's rx Vih is higher than the Voh of the esp32. So, the bms just doesn't do anything with the request for data sent by the esp32. btw, I'm not using the 13Vraw from the bms unless the BT module is plugged in - which it's not, in this project. *Possible Solution:* Install a logic level converter between the esp32 uart and the bms uart to convert the 3.3v signals to 5v and the 5v to 3.3v. I think my question for you is, does this hypothesis make sense to you and is it what you would do to fix the issue? P.S. Here is a much more detailed post about my project if you want to know more. https://diysolarforum.com/threads/anyone-working-with-the-overkill-solar-arduino-lib.27811/ Thanks! Jeremy Brooks Austin, TX On Thu, Sep 23, 2021 at 12:31 PM Fur Trader ***@***.***> wrote:

…

Sorry, the guy that wrote the library isn't available and I personally don't know how to use github correctly. Can you give me a ELI5 of what you've been doing, and how I can help? -SteveOverkill Solar LLC On Thursday, September 23, 2021, 12:09:13 PM EDT, Jeremy Brooks ***@***.***> wrote: Update... Along with above fixes, the calls to min() need to be changed to min(). After that, lib will compile for esp32 in the Arduino IDE v1 and v2. I am not able to create a branch and PR — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFCR2AQULEBB53VZVC24XULUDNP65ANCNFSM5EK4EOAA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[airy77]

I agree that the data and length fields appear to be swapped, but it turns out it doesn't matter if both the data and length fields equal 0 (swapping a 0 for 0 still yields zero).

Take a look at 3 of the calls within bms.cpp to the function "OverkillSolarBms::write(bool read, uint8_t command_code, uint8_t* data, uint8_t length)":

write(true, BMS_REG_NAME, length, data);                  // length=0, data=NULL
write(true, BMS_REG_BASIC_SYSTEM_INFO, length, data);     // length=0, data=NULL
write(true, BMS_REG_CELL_VOLTAGES, length, data);         // length=0, data=NULL

and notice that all three calls have 0 for the length and data so coincidentally, everything works out and the correct data is received from the BMS.

Where things go bad is the only other call to "write" and that is in the function to control the charge and discharge MOSFETS, which doesn't work for me when calling bms.set_mosfet_control from my sketch. Here is the line in bms.cpp:

write(true, BMS_REG_CTL_MOSFET, length, data);

Again, it looks like the length and data fields are swapped but even more puzzling, why isn't the function called with the first argument assigned to "false" (indicating a BMS write), after all, I'd think we need to write to the BMS to control the FETs, not read from the BMS (a BMS read is indicated by the first bool parameter within the function call - true:read, false:write).

So I changed the call to:

write(false, BMS_REG_CTL_MOSFET, data, length);

but I still can't control the FETs. Anybody else figured out how to control the FETs?