Extended code, bug-fixes and adjusted default values in setShunt()

[Dennis-van-Gils]

Code extended, see list below. The only code break is that the default values in method setShunt() now follow the spec sheet, i.e. shunt_res = 0.015 and max_current = 10. That is the last commit in this list, which I could remove from the PR if needed. The rest of the requested commits are not breaking. Tested.

• Added missing modes in enum INA228_MeasurementMode
• Added methods set/getTemperatureConversionTime()
• Added optional adc_range parameter in method setShunt()
• Added method getADCRange()
• Default parameter values in setShunt() follow spec sheet now

Keep up the good work :)

Followed up:

• Fixed wrong bits retrieved by getMode()
• Added method reset_accumulators()
• Fixed doxygen report by adding missing docstrings

[Dennis-van-Gils]

Is there something I can do or help with to expedite this merge? Best regards, Dennis.

[caternuson]

Added missing modes in enum INA228_MeasurementMode

Looks good. Can you add doc strings for the new ones. The terse verbiage from the datasheet is OK.

Added methods set/getTemperatureConversionTime()

Looks good.

Added optional adc_range parameter in method setShunt()

Instead, use setADCRange(), see next comment.

Added method getADCRange()

It'd be good to also add a setter setADCRange(). That's how
one would set ADC range instead of a parameter to setShunt().
Also, the _adc_range member does not seem necessary, instead
can call getADCRange() directly, ex:

  if (getADCRange()) {
    scale = 4;
  }

Default parameter values in setShunt() follow spec sheet now

Leave these as unchanged in this PR. There may be some reason
they are what they are. If not, can update those in a separate PR.

Fixed wrong bits retrieved by getMode()

thanks!

Added method reset_accumulators()

Rename this resetAccumulators just to be camel case style consistent.

Fixed doxygen report by adding missing docstrings

thanks!

[Dennis-van-Gils]

@caternuson All requested changes are met, except for your note on setting the ADC range. It is not as straight forward as you write. Here is the case:

Notice lines 139-145: shunt_cal needs to be written to the INA228 device register INA228_REG_SHUNTCAL for the correct bus voltage to get reported back by the INA228. The shunt_cal value depends on shunt_res, max_current and adc_range. Changing any of these three requires updating register INA228_REG_SHUNTCAL.

Scenario 1 (current PR):
Passing shunt_res, max_current and adc_range as parameters to setShunt() is the most efficient, because shunt_cal is calculated once and written to the register once. Also, the original brief of setShunt() reads 'Sets the shunt calibration by resistor', which makes me believe it was originally intended to perform scenario 1, but was forgotten to get adc_range passed in as parameter.

Scenario 2:
Splitting the parameters over methods setShunt(shunt_res, max_current) and setADCRange(adc_range) requires extra private members _shunt_res, _current_lsb and _adc_range to be defined. These private members can then be used inside new method _updateShuntCalRegister():

void _updateShuntCalRegister(void) {
  float scale = 1;
  if (_adc_range) {
    scale = 4;
  }

  float shunt_cal = 13107.2 * 1000000.0 * _shunt_res * _current_lsb * scale;
  
  Adafruit_I2CRegister shunt =
      Adafruit_I2CRegister(i2c_dev, INA228_REG_SHUNTCAL, 2, MSBFIRST);
  shunt.write(shunt_cal);
}

Subsequently, _updateShuntCalRegister() will then have to be called inside setShunt() and setADCRange(). It makes for convoluted code in my opinion. I have the code for scenario 2 available if preferred.

[caternuson]

The shunt_cal value depends on shunt_res, max_current and adc_range.

It depends on scale, which is set by adc_range. And adc_range is queried via a register read, instead of via a passed in parameter.

There's no need for any additional member variables. The only change is refactoring the manual query of ADC range:

  bool adcrange = (Config->read() >> 4) & 1;

into a getter function that is then called:

  if (getADCRange()) {

(this also gets rid of the adcrange bool which is not really needed - just make the call in the conditional)

And then also adding a setter for ADC range, setADCRange().

However - a new member variable to hold the current ADC range could be added if desired. It would get set in setADCRange():

void setADCRange(uint8_t adc_range) {
  /*
   * add code here to actually set ADC RANGE bit in register
   */

  // and save locally
  _adc_range = adc_range

and then used in setShunt() instead of making a query:

if (_adc_range) {

Setting ADC range via a call to setShunt() seems awkward.

[Dennis-van-Gils]

Alright, I follow scenario 2 with your suggestions incorporated. But you should realize there is no escaping adding at least one extra private member. True, _adc_range is not needed and can instead be requested from the register when needed. That's the current PR now. But the issue lies here: Calling either setShunt() or setADCRange() with new values requires the recalculation of the 'shunt calibration' and subsequently updating this value in the INA228 register. This gets now done via method _updateShuntCalRegister(), which gets called at the end of setShunt() and setADCRange(). Newly added member _shunt_res is needed for recalculating the shunt calibration.

[caternuson]

Can setADCRange() just call setShunt()? There's no need for setShunt() to call setADCRange(). So factoring out the shunt value register write to _updateShuntCalRegister() and adding _shut_res doesn't seem necessary?

It's also OK to add a new member _adc_range if that makes sense. Seems like that just helps by not requiring that register to be read in setShunt()?

[caternuson]

So factoring out the shunt value register write to _updateShuntCalRegister() and adding _shut_res doesn't seem necessary?

Oh, wait, nevermind. I see why you're doing this now. Otherwise the shunt value would somehow have to be specified again in the call to setShunt() from setADCRange().

[caternuson]

OK, suggest moving _updateShuntCalRegister() to private in the header. Then I think this PR is looking good.

Also - have you tested this code?

[Dennis-van-Gils]

So factoring out the shunt value register write to _updateShuntCalRegister() and adding _shut_res doesn't seem necessary?

Oh, wait, nevermind. I see why you're doing this now. Otherwise the shunt value would somehow have to be specified again in the call to setShunt() from setADCRange().

I am glad you see the issue now :)

[Dennis-van-Gils]

Also - have you tested this code?

Yes, all changes of this PR are tested and stable. It is used in a project for a university practicum.
Issue #6 "getMode() gets the wrong bits" can be closed if this PR is merged.
Glad I could contribute.

[caternuson]

Thanks! LGTM. Going to merge.