Shunt calibration register 15 Bit instead of 16?

[josef929]

Hi Rob Tillaart,
I am a physicist at the Universität Heidelberg. In our experiment, we are using the INA 236 chip. It is similar to the INA 226 but has two selectable ranges (~ 80 mV/20 mV). By implementing the range selection in your code, I encountered a possible mistake in the code.

In the function "setMaxCurrentShunt," during normalisation, the minimal current_lsb is calculated (lines 244-247). This value is based on the shunt_register size and some internal factors. You assumed the register to be 16 Bits, but according to the datasheet, only 15 Bits are writable.
In Line 294, at the auto scale calibration, the shunt calibration value is compared to 16 bits rather than 15 Bits.
This has only consequences if the optimal current_lsb is larger than 15Bit.

Thank you for the effort you put into this project.
Best regards
Josef

[RobTillaart]

Thanks for this issue. Need to investigate this asap probably coming week.

[RobTillaart]

@josef929

Bug confirmed!

[RobTillaart]

@josef929

Did you patch the library to support the INA236 (80/20 mV) ?

It might easy to implement a derived class.

[RobTillaart]

@josef929
Created develop branch for 0.6.0 version which should fix this.
Changed the max calibration value from 65535 => 32767.

If you have time, can you confirm the develop branch works for you?

[josef929]

Yes, I implemented the selectable range. I rewrote the code for the INA 236 (changed the names and checked the registers), but most of the code is basically the same. I can sent you the code for the INA 236 class if you want.

[josef929]

@josef929 Created develop branch for 0.6.0 version which should fix this. Changed the max calibration value from 65535 => 32767.

If you have time, can you confirm the develop branch works for you?

Yes I can do that.

[RobTillaart]

I can sent you the code for the INA 236 class if you want.

Please do, so I can make it available for the larger audience if you agree
(either as derived class or as separate repository)

[josef929]

documents-export-2024-5-27.zip
Thats the version I am using right now. I did some calibration and everything worked for me.

[RobTillaart]

Quick look shows a few differences, two functions a number of constants.
to be continued

[RobTillaart]

@josef929
small error in your code Line 361-366

  //  auto scale calibration if needed.
  uint32_t calib = round(0.00512 / (_current_LSB * shunt*pow(4,adcrange)));
  while (calib > 32768)  <<<<<<<< should be >  32767
  {
    _current_LSB *= 2;
    calib >>= 1;
  }
  _writeRegister(INA236_CALIBRATION, calib);

[RobTillaart]

@josef929
Estimate is that 200-250 of the 700 lines of non-equal functions exist in the .cpp file, mostly setMaxCurrentShunt();
With current insight making a separate library for the INA236 will be easiest way to publish.
(in terms of maintenance it is hardly more work as the library is pretty stable).
Furthermore I think it will be the most clear to the users of the library.

So green light for a separate repository?

[josef929]

Yes, thats fine with me.

[RobTillaart]

OK, I will set up a new repo, add your name - credits where credits are due.

[josef929]

@josef929 Created develop branch for 0.6.0 version which should fix this. Changed the max calibration value from 65535 => 32767.

If you have time, can you confirm the develop branch works for you?

I tested the 0.6.0 version. The returned current values were all as expected, especially for small maximal currents, where the 15 Bit shunt calibration register limits the resolution. I checked both true and false option for the normalization of the calibration.

[RobTillaart]

Thanks for testing!

Created a develop branch + PR - https://github.com/RobTillaart/INA236
build is running
documentation is work in progress.

[RobTillaart]

@josef929

found another minor point, can you confirm that it makes sense?

When calling setADCRange(..), the value of _voltage_LSB must change as the next reading will be done with a different voltageLSB.

[josef929]

@josef929

found another minor point, can you confirm that it makes sense?

When calling setADCRange(..), the value of _voltage_LSB must change as the next reading will be done with a different voltageLSB.

Yes and No.
It is true that the variable voltage LSB has to change when setADCRange is called. However, I implemented the range so that the best range (depends only on the max current input) is selected based on the max current in the setMaxCurrentShunt function. In that function, the voltage_lsb is then updated to the chosen range. Changing the range manually would require a new calibration. I saw no benefit or situation where this would be useful.

[RobTillaart]

Exactly,
However if the user decides to call setRange() - for whatever reason - and changes the value in the device, the next measurement will be affected. So to guarantee the correctness of that new measurement, the voltage_LSB must be changed too.

And yes, if that setting is not optimal or even out of range (setting 20 mV while actual voltage is higher) is the risk of the user.
Maybe a warning must be added in the documentation.

[RobTillaart]

BTW, your decision to set the best range in setMaxCurrentShunt is 100% right (imho).

[josef929]

Exactly, However if the user decides to call setRange() - for whatever reason - and changes the value in the device, the next measurement will be affected. So to guarantee the correctness of that new measurement, the voltage_LSB must be changed too.

And yes, if that setting is not optimal or even out of range (setting 20 mV while actual voltage is higher) is the risk of the user. Maybe a warning must be added in the documentation.

Understood. In such scenarios, the value of the shunt register also needs to be updated (either by a factor of 4 or 1/4, depending on the previous Range ).

[RobTillaart]

The build doesn't cooperate 100%, it fails on I2C somehow.
To be continued tomorrow.

[RobTillaart]

@josef929
Thanks for your code and testing so far.
As this issue is solved I have closed it,
If new problems occur please open an issue under the appropriate repo (INA226/236) ,

(still investigating the build CI which works for the 226 and fails for the 236 although they are nearly identical)

[RobTillaart]

@josef929
FYI, INA236 initial version released - https://github.com/RobTillaart/INA236