Accessing an array out of bound on bms_apply_dis_scp at bms_bq769x0.c:257

[RianWardana]

Hello, thanks for making this repository open-source as it is very beneficial for renewable energy enthusiasts community.

I found some issues when trying to set SCD_THRESH and other setting bits on PROTECT1, 2, 3 registers. The firmware is able to write to those registers, but upon closer look, there is a mistake in what the firmware write to those registers.

My investigation leads to something peculiar in how SCD_THRESH is chosen over SCD_threshold_setting in bq769x0_registers.h.

In bms_apply_dis_scp() at bms_bq769x0.c:

    for (int i = sizeof(SCD_threshold_setting)-1; i > 0; i--) {
        if (conf->dis_sc_limit * conf->shunt_res_mOhm >= SCD_threshold_setting[i]) {
            protect1.bits.SCD_THRESH = i;
            break;
        }
    }

In above snippet, sizeof(SCD_threshold_setting) is expected to give the array length. But in reality, sizeof() returns the array size in bytes. And because @SCD_threshold_setting is composed of uint16_t elements, sizeof(SCD_threshold_setting) returns 2 bytes * array length.

Is this a mistake or there is actually a reason for using sizeof(SCD_threshold_setting) instead of sizeof(SCD_threshold_setting) / sizeof(SCD_threshold_setting[0]) ?

Of course I am not an expert in C programming as I have never taken formal education on this matter, so pardon me if I was wrong.

Reference https://stackoverflow.com/questions/671703/array-index-out-of-bound-in-c

[martinjaeger]

Oh yes, you are right. That's a mistake.

As you might have seen, I started with several updates/improvements in the code base, also to support the ISL94202 chip. The part of the code for the ISL was directly implemented together with unit tests. The older parts for BQ chips still lack unit-tests and will need a review and some more testing. Thanks for finding this bug already :)

[RianWardana]

Okay, no problem. If that's the case, I will submit pull request regarding this matter.

While reviewing the code, I also have some inquiry about how this firmware decides when to apply balancing. But I think it's better to open another thread for this.

[martinjaeger]

Ok, cool.

And yes, please open another issue for the other topic.

[martinjaeger]

Found this error in multiple locations and fixed all of them now.