setMaxCurrentShunt normalization optimized

tileiar commented 1 year ago

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 commented 1 year ago

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 commented 12 months ago

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

RobTillaart commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

RobTillaart commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

RobTillaart commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

Will be working on - https://github.com/RobTillaart/INA226/issues/31 today This won't affect the normalize code.

RobTillaart commented 11 months ago

FYI released 0.5.0

tileiar commented 11 months ago

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 commented 11 months ago

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

I've started a build

tileiar commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

RobTillaart commented 11 months ago

Only very minor comments, style things.

RobTillaart commented 11 months ago

looks good and I will merge it later this afternoon

RobTillaart commented 11 months ago

Created a PR to release this code as 0.5.1

Thanks again for finding the bug and develop an improved algorithm

tileiar commented 11 months ago

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

RobTillaart commented 11 months ago

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

RobTillaart / INA226

setMaxCurrentShunt normalization optimized #29