setMaxCurrentShunt normalization optimized

[tileiar]

Hello Rob, thank you very much for your INA226 LIB! It is essential for my little DIY project and a great help! Thank you for sharing it!

I think there is a bug in setMaxCurrentShunt at the normalization code. After normalization _current_LSB is less than before.

setMaxCurrentShunt(8, 0.01, true )
normalize: true
initial current_LSB: 0.00024414 uA / bit
Prescale current_LSB: 0.00024416 uA / bit
factor: 10000
Final current_LSB: 0.00010000 uA / bit
Calibration: 5120
Max current: 3.28 A
Shunt: 0.01000000 ohm
ShuntV: 0.0800 Volt

setMaxCurrentShunt(20, 0.002, true )
normalize: true
initial current_LSB: 0.00061035 uA / bit
Prescale current_LSB: 0.00061040 uA / bit
factor: 10000
Final current_LSB: 0.00010000 uA / bit
Calibration: 25600
Max current: 3.28 A
Shunt: 0.00200000 ohm
ShuntV: 0.0400 Volt

There would be an easy fix by changing '_current_LSB = 1.0 / factor;" to '_current_LSB = 10.0 / factor;" at line 235.

Never the less I thought, the normalization could be less greedy and more flexible. I hope you like it!

Best regards,
Armin

[RobTillaart]

Thank you for this PR.
I will look into it later this or coming week (busy).
Have to reread datasheet to check the details again

[RobTillaart]

Sorry for the inconvenience,
Other priorities take my time so several things are delayed including this PR.

[RobTillaart]

Found some time to have a look.

---

Note: the unit in the debug print is incorrect. uA / bit should be A / bit

setMaxCurrentShunt(8, 0.01, true )
normalize: true
initial current_LSB: 0.00024414 uA / bit ===> 0.00024414 * 32768 == 7.99997 A
Prescale current_LSB: 0.00024416 uA / bit ===> 0.00024416 * 32768 == 8.00063 A
factor: 10000
Final current_LSB: 0.00010000 uA / bit ===> 0.00010000 * 32768 == 3.2768 A
Calibration: 5120
Max current: 3.28 A**
Shunt: 0.01000000 ohm
ShuntV: 0.0800 Volt

---

setMaxCurrentShunt(8, 0.01, true )
Max current: 3.28 A

So the normalized Max Current is below the requested max current.
That is not OK.

So I consider the problem confirmed.
Now look at the code how to fix (your fix is straightforward but does not explain why there is a factor 10)

[RobTillaart]

I have this (temporart) variation

int INA226::setMaxCurrentShunt(float maxCurrent, float shunt, bool normalize)
{
  #define printdebug true

  //  fix #16 - datasheet 6.5 Electrical Characteristics
  //            rounded value to 80 mV
  float shuntVoltage = abs(maxCurrent * shunt);
  if (shuntVoltage > 0.080)         return INA226_ERR_SHUNTVOLTAGE_HIGH;
  if (maxCurrent < 0.001)           return INA226_ERR_MAXCURRENT_LOW;
  if (shunt < INA226_MINIMAL_SHUNT) return INA226_ERR_SHUNT_LOW;

  _current_LSB = maxCurrent * 3.0517578125e-5;      //  maxCurrent / 32768;

  #ifdef printdebug
    Serial.println();
    Serial.print("normalize:\t");
    Serial.println(normalize ? " true":" false");
    Serial.print("initial current_LSB:\t");
    Serial.print(_current_LSB, 8);
    Serial.println(" A / bit");
  #endif

  uint32_t calib  = 0;
  uint32_t factor = 1;

  //  normalize the LSB to a round number
  //  LSB will increase
  if (normalize)
  {
    calib = round(0.00512 / (_current_LSB * shunt));
    _current_LSB = 0.00512 / (calib * shunt);

    #ifdef printdebug
      Serial.print("Prescale current_LSB:\t");
      Serial.print(_current_LSB, 8);
      Serial.println(" A / bit");
    #endif

    //  new maximum current must be able to handle the requested maxCurrent.
    float newMaxCurrent = 32768;       //  # steps of unit 1 A (* 10^-n)
    while (newMaxCurrent > maxCurrent)
    {
      newMaxCurrent *= 0.1;
      factor *= 10;
    }
    if (newMaxCurrent < maxCurrent)
    {
      // we must correct the last iteration
      factor /= 10;
    }
    // else if (newMaxCurrent == maxCurrent)  //  no correction needed.

    //  scale current_LSB
    _current_LSB = 1.0 / factor;
    
    //  factor = 1;
    //  while (_current_LSB < 1)
    //  {
    //    _current_LSB *= 10;
    //    // Serial.println(_current_LSB, 8);
    //    factor *= 10;
    //  }
    //  _current_LSB = 1.0 / factor;
  }

  //  auto scale calibration
  calib = round(0.00512 / (_current_LSB * shunt));
  while (calib > 65535)
  {
    #ifdef printdebug
      Serial.print("Adjust calib:\t");
      Serial.println(calib);
    #endif
    _current_LSB *= 10;
    calib /= 10;
  }
  _writeRegister(INA226_CALIBRATION, calib);

  _maxCurrent = _current_LSB * 32768;
  _shunt = shunt;

  #ifdef printdebug
    Serial.print("factor:\t");
    Serial.println(factor);
    Serial.print("Final current_LSB:\t");
    Serial.print(_current_LSB, 8);
    Serial.println(" A / bit");
    Serial.print("Calibration:\t");
    Serial.println(calib);
    Serial.print("Max current:\t");
    Serial.print(_maxCurrent);
    Serial.println(" A");
    Serial.print("Shunt:\t");
    Serial.print(_shunt, 8);
    Serial.println(" ohm");
    Serial.print("ShuntV:\t");
    Serial.print(shuntVoltage, 4);
    Serial.println(" Volt");
  #endif

  return INA226_ERR_NONE;
}

[RobTillaart]

This code gives similar results as yours, but it explains the how a bit more.

Got this output

INA.setMaxCurrentShunt(8, 0.01, true);

normalize: true
initial current_LSB: 0.00024414 A / bit
Prescale current_LSB: 0.00024416 A / bit
factor: 1000
Final current_LSB: 0.00100000 A / bit
Calibration: 512
Max current: 32.77 A
Shunt: 0.01000000 ohm
ShuntV: 0.0800 Volt

The requested maxCurrent is a factor 4 above the requested maxCurrent.

I'm going to think how I can round the _current_LSB in normalized form to 1, 2.5 or 5 times some power of 10.

In this case
Final current_LSB: 0.00025000 A / bit
Calibration: 2048
Max current: 8.19 A

It would provide a higher resolution.
Opinion?

[tileiar]

Unsurprisingly I don’t think my code is hard to understand ;-)

If you agree to use 2.5uA * factor (1 ,10 or 100) I could make the code simpler:

uint32_t calib  = 0;
uint8_t factor = 1;
float currentLSB = 0;

//  normalize the LSB to a round number
//  LSB will increase
if (normalize)
  {
    if (_current_LSB <= 10e-6 ) {
      // _current_LSB is <= 10uA
      // start normalization with 2.5uA and a step of 2.5uA      
      currentLSB = 2.5e-6		
      factor = 1;
    }else if (_current_LSB <= 100e-6 ) {
      // _current_LSB is > 10uA and <= 100uA
      // start normalization with 25uA and a step of 25uA 
      currentLSB = 25e-6
      factor = 10;
    }else {
      // _current_LSB is > 100uA
      // start normalization with 250uA and a step of 250uA
      currentLSB = 250e-6
      factor = 100; 
    }
     
    while( currentLSB < _current_LSB ) {
        currentLSB = currentLSB + 2.5e-6 * factor;    
    }
    _current_LSB = currentLSB;
  }

[RobTillaart]

Unsurprisingly I don’t think my code is hard to understand ;-)

It can be very well understood however think that it was my code part (determining the factor) that could be improved upon.
By refactoring that part that lead to the new value currentLSB explained better why it had to be corrected.
At least it became more clear to me the main maintainer 😎

Calculating the factor in a loop allows a larger range of usage then only the three levels you propose.
E.g. INA.setMaxCurrentShunt(0.5, 0.01, true)

If you agree to use 2.5uA * factor (1 ,10 or 100) I could make the code simpler:

I strongly prefer the three step 1, 2, 5 scheme.
There are two reasons for it

• these three have the same amount of digits and would equally fit on e.g. a 4 digit display.
• their relative size is almost equal ( factor 2 or 2.5 for the next step)

[tileiar]

Ok, so in which range are we working on?

• Let’s say we start with 1uA (going below doesn't much sense to me). This means your minimum shunt resistor should be 2.5 Ohm (ADC resolution is 2.5uV). Current range would be from 1uA to 32mA.
• On the other side we have a min shunt resistor of 1 mOhm. This means a current range from 2.5mA to 80A.

How should the current_LSB steps should look like?

• In my implementation it is 2.5uA, 5uA, 7.5uA, 10uA, 25uA, 50uA, 75uA, 100uA, 250uA, 500uA, 750uA, 1000uA, 1250uA, ... , 2500uA, ...
• Do I understand you right, that the steps should be 1uA, 2uA, 5uA, 10uA, 20uA, 50uA, 100uA, 200uA, 500uA, 1mA, 2mA, 5mA, 10mA?

[tileiar]

Note: 1, 2, 4, 6, 8, 10, 20, 40, 60, 80, ... would be easier to implement

[RobTillaart]

How should the current_LSB steps should look like?

The second one was my thoughts,

Currently it is 1,10,100, 1000 which is "screaming" for improvement of accuracy.
Your scale works quite well, and after I had some test runs with your code it works quite well.

There is one scenario in which I encountered a "problem".
If the calibration factor goes beyond 65535, it is scaled back with a factor 10.
Your scheme of the 2.5 steps breaks a bit as possibly you only want scale back in the right steps.
(it would break any scheme other than 1,10,100...)

Compare these calls.

INA.setMaxCurrentShunt(0.5, 0.01, true);
INA.setMaxCurrentShunt(0.5, 0.001, true);

  //  auto scale calibration
  calib = round(0.00512 / (_current_LSB * shunt));
  while (calib > 65535)
  {
    _current_LSB *= 10;     <<<<<<<<<<
    calib /= 10;
    Serial.println(calib);
  }

[RobTillaart]

Note: 1, 2, 4, 6, 8, 10, 20, 40, 60, 80, ... would be easier to implement

Please give it a try, I can have a look tomorrow (other appointments)

[tileiar]

Ok, I see.

1. normalization
   Useing bad combinations of max current and shunt resistor may result in uneven values for current_lsb. I'm not sure, if this is a problem?!

To solve this, the current_lsb calculation has to be shunt resistor centric:

Assumptions / limits:

• shunt resistor >= 250m Ohm (coverd by NA226_ERR_SHUNT_LOW)
• shunt resistor <= 2.5 Ohm (does not run into an error, but hard limit to 1uA resolution)
• current lsb >= 1u and <= 10mA
• max current >= 32.768 mA (does not run into an error, but hard limit to 1uA resolution)
• max current <= 327.68 A (indirectly checked by INA226_ERR_SHUNTVOLTAGE_HIGH)

uint32_t calib  = 0;
// uint8_t factor = 1;

if (normalize)
  {
    // adc lsb is 2.5uV; the lowest current we can messure based on given shunt resistor is ...
    _current_LSB = 2.5e-6 / shunt;

    // start to normalize with a multiple of 10 (next lower) 
    if (_current_LSB > 1000e-6 ) {
      _current_LSB = 1000e-6;  // 1mA
    }else if (_current_LSB > 100e-6 ) {
      _current_LSB = 100e-6;  //100uA
    }else if (_current_LSB > 10e-6 ) {
      _current_LSB = 10e-6;  // 10uA
    }else {
      _current_LSB = 1e-6; // 1uA
    }

    // check against max current and normalize current_lsb to * 1, 2, 5 or 10
    if (_current_LSB * 32768 >=  maxCurrent ) {
      _current_LSB *=1;
    }else if (_current_LSB * 2 * 32768 >=  maxCurrent ) {
      _current_LSB *=2;
    }else if (_current_LSB * 5 * 32768 >=  maxCurrent ) {
      _current_LSB *=5;
    }else {
      _current_LSB *=10;
    }

  }

Note: the code is NOT tested; it is just for discusson; if you like it, I will test it of corse!

2.) auto scale
May a scale of 2 would be a good option:

//  auto scale calibration
  calib = round(0.00512 / (_current_LSB * shunt));
  while (calib > 65535)
  {
    _current_LSB *= 2;
    calib /= 2;
  }

Note: This part should not be necessary für normalized _current_LSB; but it is more safe to double check it anyway

[tileiar]

sorry for closing this PR by accident 😟; I don't know how to undo, so I reopend it.

[RobTillaart]

sorry for closing this PR by accident 😟; I don't know how to undo, so I reopend it.

Is a GUI "bug", the close with comment button should be much further away from the comment one, e.g. on the left,

[RobTillaart]

Note: the code is NOT tested; it is just for discussion; if you like it, I will test it of course!

The proof is in the pudding test, so yes please test it if you have time.

[RobTillaart]

Will be working on - #31 today
This won't affect the normalize code.

[RobTillaart]

FYI released 0.5.0

[tileiar]

Hello Rob, I've rewritten the code. I think its working now very well. Unfortunaley I'm not very familiar with github. Do you see the new code? Is this the right procedure or should I do a new pull request? Thx.

[RobTillaart]

I'm quite busy and will try to look into it this evening.

I've started a build

[tileiar]

btw: I have a version with integer rather than float also. But it needs lower and upper limits to work. eg. shunt resistor from 0.001 to 1 Ohm. I don't know if this would be accaptable for you?

[RobTillaart]

btw: I have a version with integer rather than float also.
But it needs lower and upper limits to work. eg. shunt resistor from 0.001 to 1 Ohm. I don't know if this would be accaptable for you?

Think it would not please all the users of the library - which I don't know, so that is an assumption.
Does the math do more rounding, or do you use microVolts and micrAmps as unit?

What would be the added value for users?

---

Another thought (thinking out loud).
now the current is fetched from the current register.
However if I read the shuntVoltage register and as I know the shunt resistance, I can do the math in the lib.
(assuming single measurements)

The disadvantage is that for calculating the power I need 2 calls to the device unless the user uses the last two values.

////////////////   UNCHANGED
float INA226::getShuntVoltage()
{
  int16_t val = _readRegister(INA226_SHUNT_VOLTAGE);
  return val * 2.5e-6;   //  fixed 2.50 uV
}

float INA226::getBusVoltage()
{
  uint16_t val = _readRegister(INA226_BUS_VOLTAGE);
  return val * 1.25e-3;  //  fixed 1.25 mV
}

////////////////   CHANGED
float INA226::getCurrent()
{
  return getShuntVoltage() / _shunt;    //  can be optimized by storing/using  1.0 / shunt;
}

float INA226::getPower()
{
  return getBusVoltage() * getCurrent();
}

Could be the basis for a tiny_INA226 or mini_ina226 library?
max stripped, minimal footprint for 4 core functions, no calibration averaging etc.

[tileiar]

Hi, sorry, one more thing. The new code uses about 78 Bytes more Flash (0 RAM) than your temporary fix. I have an alternative version, which is less elegant* but uses 116 Bytes less Flash and 24 Bytes more (!) RAM. What do you think?

*) a simple array of values and a loop to check:

    const uint16_t norm_val[] = { 1, 2, 5, 10, 20, 50, 100, 200, 500, 1000, 2000, 5000 }; // 1uA ... 5mA
    uint16_t currentLSB_uA = float(_current_LSB * 1e+6);
    currentLSB_uA++; // ceil would be more precise, but uses 176 bytes of flash
    byte i;

    for(i=0; i < sizeof(norm_val)/sizeof(uint16_t); i++){
      if(norm_val[i] >= currentLSB_uA){
        _current_LSB = norm_val[i] * 1e-6;
        i=0xFF;
        break;
      }
    }
    if(i!=0xFF) {
      return INA226_ERR_NORMALIZE_FAILED;
    }

Thx, Armin

[tileiar]

Could be the basis for a tiny_INA226 or mini_ina226 library?
max stripped, minimal footprint for 4 core functions, no calibration averaging etc.

Would be nice. On the other hand for my projects the footprint of this version of the lib is totally fine. Maybe a "setMaxCurrentShuntRaw" would be an easy improvement to save some bytes of FLASH and RAM.

[RobTillaart]

Hi, just had a small test run and it works pretty well

a simple array of values and a loop to check:

Think that is a good idea,
if you turn that into a matrix testing 1,2,5 and powers separately might save some RAM (as only the upper 4 need 16 bit)
(I care more about RAM as about FLASH)

uint16_t currentLSB_uA = float(_current_LSB * 1e+6); // float casting is not needed as 1e6 is float.

byte i; ==> I prefer uint8_t as byte can be signed on some platforms.

so it would become

    uint8_t norm_val[] = { 1, 2, 5 };
    uint16_t factor = 1;
    uint16_t currentLSB_uA = float(_current_LSB * 1e+6);
    currentLSB_uA++;  //  ceil would be more precise, but uses 176 bytes of flash


    bool failed = true;
    for ( uint16_t factor = 1; factor < 10000; factor *= 10)
    {
      for(uint8_t i = 0; i < sizeof(norm_val)/sizeof(uint16_t); i++) 
      {
        uint16_t norm = norm_val[i] * factor;
        if (norm >= currentLSB_uA) 
        {
          _current_LSB = norm * 1e-6;
          failed = false;;
          break;
        }
      }
    }
    if (failed) {
      return INA226_ERR_NORMALIZE_FAILED;
    }

[tileiar]

Hi, your are right! I played around with your suggestion to see whats the impact on Flash/RAM. It is strange. If you don't catch the break in the outer loop, the code is working, but it is 86 byte longer. Thus I suggest a do-while loop. New size is +4 bytes on RAM and 0 Bytes on Flash compared to your temporary fix.

// normalize _current_LSB
    const uint8_t norm_val[] = { 1, 2, 5 };
    uint16_t factor = 1;
    uint16_t currentLSB_uA = float(_current_LSB * 1e+6);
    currentLSB_uA++; // ceil would be more precise, but uses 176 bytes of flash
    bool failed = true;

    do{
      for(uint8_t i=0; i < sizeof(norm_val)/sizeof(uint8_t); i++){
        if( norm_val[i] * factor >= currentLSB_uA){
          _current_LSB = norm_val[i] * factor * 1e-6;
          failed = false;
          break;
        }
      }
      factor *= 10;
    }while(factor < 100000 and failed);

    if(failed) {
      _current_LSB = 0;
      return INA226_ERR_NORMALIZE_FAILED;
    }

But thinking in this direction, there would be an easier solution:

// normalize _current_LSB
    uint16_t factor = 1;
    uint16_t currentLSB_uA = float(_current_LSB * 1e+6);
    currentLSB_uA++; // ceil would be more precise, but uses 176 bytes of flash
    bool failed = true;

    do{
      if( 1*factor >= currentLSB_uA){
        _current_LSB = 1*factor * 1e-6;
        failed = false;
      }else if( 2*factor >= currentLSB_uA){
        _current_LSB = 2*factor * 1e-6;
        failed = false;
      }else if( 5*factor >= currentLSB_uA){
        _current_LSB = 5*factor * 1e-6;
        failed = false;
      }else {
        factor *= 10;
      }
    }while(factor < 100000 and failed);

    if(failed) {
      _current_LSB = 0;
      return INA226_ERR_NORMALIZE_FAILED;
    }

Which is 90 bytes of Flash and 4 bytes of RAM less.

[tileiar]

I made a new version based on my last input. I think this could be the final version. That do you think?!

[RobTillaart]

Only very minor comments, style things.

[RobTillaart]

looks good and I will merge it later this afternoon

[RobTillaart]

Created a PR to release this code as 0.5.1

Thanks again for finding the bug and develop an improved algorithm

[tileiar]

You are wellcome! Thank you for your work and your patience with me!

[RobTillaart]

Now we have to wait if it triggers new issues or not. Think the code is quite robust so I wont expect them soon.