Closed ahmed-tkhan closed 6 months ago
Thanks for the issue, I will try to look into it asap however that might take several days.
A quick read of the issue sound serious so label it as bug for now.
So Another bug I saw was with how you set and get power valid limits:
////////////////////////////////////////////////////////
//
// POWER LIMIT
//
int INA3221::setPowerUpperLimit(int16_t milliVolt)
{
if (milliVolt > 16376) return -10;
int16_t value = milliVolt & 0xFFF8; // mask reserved bits
return _writeRegister(INA3221_POWER_VALID_UPPER, value);
}
int16_t INA3221::getPowerUpperLimit()
{
int16_t value = _readRegister(INA3221_POWER_VALID_UPPER);
return value;
}
int INA3221::setPowerLowerLimit(int16_t milliVolt)
{
if (milliVolt > 16376) return -10;
int16_t value = (milliVolt << 1) & 0xFFF8;
return _writeRegister(INA3221_POWER_VALID_LOWER, value);
}
int16_t INA3221::getPowerLowerLimit()
{
int16_t value = _readRegister(INA3221_POWER_VALID_LOWER);
return value >> 1;
}
(updated syntax highlighting)
Updated your posts for readability of the code, If time permits I will have a first in depth look today.
Need to order new INA3221's this week, so I have some hardware to test.
int INA3221::setCriticalAlert(uint8_t channel, uint16_t microVolt) { if (channel > 2) return -1; if (microVolt > 8191*40) return -2; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<< uint16_t value = (microVolt / 40) << 3; // LSB 40uV shift 3 return _writeRegister(INA3221_CRITICAL_ALERT(channel), value); }
Can't be a solution as 8191*40 == 320.000++ is beyond the max uint16 (65535) parameter microVolt. So the parameter might need to be uint32_t at least.
TODO: dive into datasheet again ;)
The full scale voltage = 163.8 mV == 163800 uV So it makes little sense to set a value above that value for the alert function.
The reset value of this register is 0x7FF8 => which matches 4095 * 40 = 163800
The function should guard for this value as it makes no sense to set an alert above the max value the device can measure. @ahmed-tkhan Does this makes sense to you?
So the code becomes:
int INA3221::setCriticalAlert(uint8_t channel, uint32_t microVolt)
{
if (channel > 2) return -1;
// Check for the full scale voltage = 163.8 mV == 163800 uV
if (microVolt > (4095 * 40)) return -2;
uint16_t value = (microVolt / 40) << 3; // LSB 40uV shift 3
return _writeRegister(INA3221_CRITICAL_ALERT(channel), value);
}
It would be logical to enforce the warning alert to be smaller than the critical alert. To check this constantly would generate extra I2C calls or one need to cache all 6 alert levels (* 4 bytes => 24 bytes) and of course the extra code to check it. So I will leave that as a responsibility for the user of the library, in fact the user can decide to only use the critical alert and doesn't care about the warning alert (or vice versa).
The getter also need to change to uint32_t to be able to hold the value 163800.
uint32_t INA3221::getCriticalAlert(uint8_t channel)
{
if (channel > 2) return -1;
uint32_t value = _readRegister(INA3221_CRITICAL_ALERT(channel));
return (value >> 3) * 40;
}
The mA wrappers are a factor 1000 off as the parameter is MilliAmpere ==> yet another BUG.
// mA wrappers
int INA3221::setCriticalCurrect(uint8_t channel, uint16_t milliAmpere)
{
return setCriticalAlert(channel, milliAmpere * _shunt[channel]);
}
Or define these in microAmpere? or Float milliAmpere? [latter sounds good imho.]
Thus
// mA wrappers
int INA3221::setCriticalCurrect(uint8_t channel, float milliAmpere)
{
return setCriticalAlert(channel, 1000 * milliAmpere * _shunt[channel]);
}
float INA3221::getCriticalCurrent(uint8_t channel)
{
return getCriticalAlert(channel) * 0.001 / _shunt[channel];
}
int INA3221::setWarningCurrent(uint8_t channel, float milliAmpere)
{
return setWarningAlert(channel, 1000 * milliAmpere * _shunt[channel]);
}
float INA3221::getWarningCurrent(uint8_t channel)
{
return getWarningAlert(channel) * 0.001 / _shunt[channel];
}
@ahmed-tkhan Today I will investigate and fix the PowerLimit code.
Power limit code revisited
////////////////////////////////////////////////////////
//
// POWER LIMIT
//
// LSB 8mV shift 3
//
int INA3221::setPowerUpperLimit(int16_t milliVolt)
{
// int16_t is always within the range (after masking) 32760
// if (milliVolt > 4095 * 8) return -10; // LSB 8mV shift 3
int16_t value = milliVolt & 0xFFF8; // mask reserved bits
return _writeRegister(INA3221_POWER_VALID_UPPER, value);
}
int16_t INA3221::getPowerUpperLimit()
{
int16_t value = _readRegister(INA3221_POWER_VALID_UPPER);
// (value >> 3) * 8mV; shift 3 compensates 8 mV
return value;
}
int INA3221::setPowerLowerLimit(int16_t milliVolt)
{
// int16_t is always within the range (after masking) 32760
// if (milliVolt > 4095 * 8) return -10; // LSB 8mV shift 3
int16_t value = milliVolt & 0xFFF8;
return _writeRegister(INA3221_POWER_VALID_LOWER, value);
}
int16_t INA3221::getPowerLowerLimit()
{
int16_t value = _readRegister(INA3221_POWER_VALID_LOWER);
// (value >> 3) * 8mV; shift 3 compensates 8 mV
return value;
}
Next to investigate are
// SHUNT VOLTAGE SUM
int16_t getShuntVoltageSum(); // returns microVolt
int setShuntVoltageSumLimit(int16_t microVolt);
int16_t getShuntVoltageSumLimit(); // returns microVolt
as these might have similar problems.
@ahmed-tkhan
Think I fixed all the issues you mentioned and some more. Can you verify the develop branch?
I added a sort of unit test example that checks reading/writing several settings.
Furthermore I created a PR from the develop branch to create a 0.2.0 version.
There are still things to investigate / improve but this is a major step. Again thanks for your report about the problems.
@ahmed-tkhan Closed the issue as all reported issues are solved.
Again thanks for reporting the issues.
you set a limit on the input microvolt value of 2^14-1 (16383) but I think there may have been some issues with this. one is that there are 13 bits in the documentation not 14 (CL0-CL12). so the highest representable number is 8191 (2^13-1) not16383. Also the limit should be: 40uV x highest representable value with given bits I.E. 40 x 8191 I am pretty sure that this is the same with the warning alert too.
SOLUTION (maybe):
(updated syntax highlighting)