Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INA237 Sensor Bug Fixes and Enhancements (with unit tests) #60717

Merged
merged 8 commits into from
Aug 29, 2023
Merged

INA237 Sensor Bug Fixes and Enhancements (with unit tests) #60717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
864cb9a
dt-bindings: sensor: ina237: add configuration properties
EricNRS Jul 23, 2023
e2eaa61
test: sensor: ina237: add emulator unit test
EricNRS Jul 21, 2023
db7f60a
drivers: sensor: ina237: fix current sign issue
EricNRS Jul 27, 2023
4a34ccd
drivers: sensor: ina237: add high-precision mode
EricNRS Jul 23, 2023
f39fca1
drivers: sensor: ina237: add shunt voltage support
EricNRS Jul 20, 2023
50ed5a3
dt-bindings: sensor: ina230: add configuration properties
EricNRS Aug 16, 2023
35524b0
test: sensor: ina230: add emulator unit test
EricNRS Aug 16, 2023
d5201e1
drivers: sensor: ina230: fix current sign issue
EricNRS Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions drivers/sensor/ina23x/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@ config INA237
help
Enable driver for INA237.

config INA237_VSHUNT
bool "INA237 VShunt Measurement Enable"
depends on DT_HAS_TI_INA237_ENABLED
help
Enable shunt voltage measurement for INA237.

This is the actual shunt voltage measured which is scaled within the
INA237 based upon the SHUNT_CAL register. This value is useful for
looking at measurement noise or debugging the SHUNT_CAL value.

Note that enabling this option requires an extra I2C read when
SENSOR_CHAN_ALL is selected, so only enable if the shunt voltage
measurement is required.

config INA230_TRIGGER
bool "INA230 trigger mode"
depends on INA230
Expand Down
25 changes: 10 additions & 15 deletions drivers/sensor/ina23x/ina230.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ static int ina230_channel_get(const struct device *dev, enum sensor_channel chan
{
struct ina230_data *data = dev->data;
const struct ina230_config *const config = dev->config;
uint32_t bus_uv, current_ua, power_uw;
int32_t sign;
uint32_t bus_uv, power_uw;
int32_t current_ua;

switch (chan) {
case SENSOR_CHAN_VOLTAGE:
Expand All @@ -42,21 +42,12 @@ static int ina230_channel_get(const struct device *dev, enum sensor_channel chan
break;

case SENSOR_CHAN_CURRENT:
if (data->current & INA23X_CURRENT_SIGN_BIT) {
current_ua = ~data->current + 1U;
sign = -1;
} else {
current_ua = data->current;
sign = 1;
}

/* see datasheet "Programming" section for reference */
current_ua = current_ua * config->current_lsb;
current_ua = data->current * config->current_lsb;

/* convert to fractional amperes */
val->val1 = sign * (int32_t)(current_ua / 1000000U);
val->val2 = sign * (int32_t)(current_ua % 1000000U);

val->val1 = current_ua / 1000000L;
val->val2 = current_ua % 1000000L;
break;

case SENSOR_CHAN_POWER:
Expand Down Expand Up @@ -261,7 +252,11 @@ static const struct sensor_driver_api ina230_driver_api = {
static struct ina230_data drv_data_##inst; \
static const struct ina230_config drv_config_##inst = { \
.bus = I2C_DT_SPEC_INST_GET(inst), \
.config = DT_INST_PROP(inst, config), \
.config = DT_INST_PROP(inst, config) | \
(DT_INST_ENUM_IDX(inst, avg_count) << 9) | \
(DT_INST_ENUM_IDX(inst, vbus_conversion_time_us) << 6) | \
(DT_INST_ENUM_IDX(inst, vshunt_conversion_time_us) << 3) | \
DT_INST_ENUM_IDX(inst, adc_mode), \
.current_lsb = DT_INST_PROP(inst, current_lsb_microamps), \
.cal = (uint16_t)((INA230_CAL_SCALING * 10000000ULL) / \
((uint64_t)DT_INST_PROP(inst, current_lsb_microamps) * \
Expand Down
2 changes: 1 addition & 1 deletion drivers/sensor/ina23x/ina230.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

struct ina230_data {
const struct device *dev;
uint16_t current;
int16_t current;
uint16_t bus_voltage;
uint16_t power;
#ifdef CONFIG_INA230_TRIGGER
Expand Down
110 changes: 78 additions & 32 deletions drivers/sensor/ina23x/ina237.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,51 +20,65 @@ LOG_MODULE_REGISTER(INA237, CONFIG_SENSOR_LOG_LEVEL);
#define INA237_CAL_SCALING 8192ULL

/** @brief The LSB value for the bus voltage register, in microvolts/LSB. */
#define INA237_BUS_VOLTAGE_UV_LSB 3125
#define INA237_BUS_VOLTAGE_TO_uV(x) ((x) * 3125U)

/** @brief Power scaling (scaled by 10) */
#define INA237_POWER_SCALING 2
/** @brief Power scaling (need factor of 0.2) */
#define INA237_POWER_TO_uW(x) ((x) / 5ULL)

/**
* @brief Scale die temperture from 0.125 degC/bit to micro-degrees C
* Note that the bottom 4 bits are reserved and are always zero.
*/
#define INA237_DIETEMP_TO_uDegC(x) (((x) >> 4) * 125000)

static void micro_s32_to_sensor_value(struct sensor_value *val, int32_t value_microX)
{
val->val1 = value_microX / 1000000L;
val->val2 = value_microX % 1000000L;
}

static void micro_u64_to_sensor_value(struct sensor_value *val, uint64_t value_microX)
{
val->val1 = value_microX / 1000000U;
val->val2 = value_microX % 1000000U;
}

static int ina237_channel_get(const struct device *dev, enum sensor_channel chan,
struct sensor_value *val)
{
struct ina237_data *data = dev->data;
const struct ina237_data *data = dev->data;
const struct ina237_config *config = dev->config;
uint32_t bus_uv, current_ua, power_uw;
int32_t sign;

switch (chan) {
case SENSOR_CHAN_VOLTAGE:
bus_uv = data->bus_voltage * INA237_BUS_VOLTAGE_UV_LSB;

val->val1 = bus_uv / 1000000U;
val->val2 = bus_uv % 1000000U;
micro_s32_to_sensor_value(val, INA237_BUS_VOLTAGE_TO_uV(data->bus_voltage));
break;

case SENSOR_CHAN_CURRENT:
if (data->current & INA23X_CURRENT_SIGN_BIT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't ina230 have this same problem? INA23X_CURRENT_SIGN_BIT looks to be unused after this change.

The existing code is broken not just on ESP32-S3, but on all processors. The flaw is not understanding the integer promotions concept in the C standard. While data->current is 16-bit, the expression ~data->current does NOT mean to invert bits of the 16-bit value! The first step is promote data->current from unsigned 16-bit to a signed 32-bit integer. Then this 32-bit integer (the upper 16-bits are of course zero) will be inverted. That's not what was desired. What would produce the current result would be to invert the 16-bit value, add 1 to the still 16-bit value, and then take the 16-bit result and promote it to 32-bits at the end. But 16->32 conversion happens first, not last.

Anyway, this code is somewhat inefficient, actually performing the multiply to invert the sign. Better code to take absolute value:

    current_ua = data->current < 0 ? -data->current : data->current;
// or, producing the same code
    current_ua = abs(data->current);

You can see the results of different methods: original, your method, and the above examples
https://godbolt.org/z/qz3vsG4M6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the Compiler Explorer link. Probably just go with abs() in the negative case given the results.

Copy link
Contributor Author

@EricNRS EricNRS Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't ina230 have this same problem?

Yes, it does have the same issue code issue, but I do not have INA230 hardware to test, so I didn't want to change the code and release it untested. Seems like a relatively safe thing to change and verify by inspection and/or find a volunteer to test it.

Update: I have modified the INA230 code as well and requested testing on Discord, but no takers so far

Copy link
Contributor Author

@EricNRS EricNRS Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I have added a unit test for the INA230 and tested the change, so should be fixed.

Changes have been included in the latest push to the PR. PTAL when you have a chance.

current_ua = ~data->current + 1U;
sign = -1;
} else {
current_ua = data->current;
sign = 1;
}

/* see datasheet "Current and Power calculations" section */
current_ua = current_ua * config->current_lsb;

/* convert to fractional amperes */
val->val1 = sign * (int32_t)(current_ua / 1000000U);
val->val2 = sign * (int32_t)(current_ua % 1000000U);
micro_s32_to_sensor_value(val, data->current * config->current_lsb);
break;

case SENSOR_CHAN_POWER:
/* see datasheet "Current and Power calculations" section */
power_uw = (data->power * INA237_POWER_SCALING * config->current_lsb) / 10000U;
/* power in uW is power_reg * current_lsb * 0.2 */
micro_u64_to_sensor_value(val,
INA237_POWER_TO_uW((uint64_t)data->power * config->current_lsb));
break;

#ifdef CONFIG_INA237_VSHUNT
case SENSOR_CHAN_VSHUNT:
if (config->config & INA237_CFG_HIGH_PRECISION) {
/* high-resolution mode - 1.25 uV/bit, sensor value is in mV */
micro_s32_to_sensor_value(val, data->shunt_voltage * 1250);
} else {
/* standard-resolution - 5 uV/bit -> nano-volts */
micro_s32_to_sensor_value(val, data->shunt_voltage * 5000);
}
break;
#endif /* CONFIG_INA237_VSHUNT */

/* convert to fractional watts */
val->val1 = (int32_t)(power_uw / 1000000U);
val->val2 = (int32_t)(power_uw % 1000000U);
case SENSOR_CHAN_DIE_TEMP:
micro_s32_to_sensor_value(val, INA237_DIETEMP_TO_uDegC(data->die_temp));
break;

default:
Expand Down Expand Up @@ -155,6 +169,24 @@ static int ina237_read_data(const struct device *dev)
}
}

if ((data->chan == SENSOR_CHAN_ALL) || (data->chan == SENSOR_CHAN_DIE_TEMP)) {
ret = ina23x_reg_read_16(&config->bus, INA237_REG_DIETEMP, &data->die_temp);
if (ret < 0) {
LOG_ERR("Failed to read temperature");
return ret;
}
}

#ifdef CONFIG_INA237_VSHUNT
if ((data->chan == SENSOR_CHAN_ALL) || (data->chan == SENSOR_CHAN_VSHUNT)) {
ret = ina23x_reg_read_16(&config->bus, INA237_REG_SHUNT_VOLT, &data->shunt_voltage);
if (ret < 0) {
LOG_ERR("Failed to read shunt voltage");
return ret;
}
}
#endif /* CONFIG_INA237_VSHUNT */

return 0;
}

Expand All @@ -169,7 +201,11 @@ static int ina237_sample_fetch(const struct device *dev, enum sensor_channel cha
struct ina237_data *data = dev->data;

if (chan != SENSOR_CHAN_ALL && chan != SENSOR_CHAN_VOLTAGE && chan != SENSOR_CHAN_CURRENT &&
chan != SENSOR_CHAN_POWER) {
chan != SENSOR_CHAN_POWER &&
#ifdef CONFIG_INA237_VSHUNT
chan != SENSOR_CHAN_VSHUNT &&
#endif /* CONFIG_INA237_VSHUNT */
chan != SENSOR_CHAN_DIE_TEMP) {
return -ENOTSUP;
}

Expand Down Expand Up @@ -360,15 +396,25 @@ static const struct sensor_driver_api ina237_driver_api = {
.channel_get = ina237_channel_get,
};

/* Shunt calibration must be muliplied by 4 if high-prevision mode is selected */
#define CAL_PRECISION_MULTIPLIER(config) \
(((config & INA237_CFG_HIGH_PRECISION) >> 4) * 3 + 1)

#define INA237_DRIVER_INIT(inst) \
static struct ina237_data ina237_data_##inst; \
static const struct ina237_config ina237_config_##inst = { \
.bus = I2C_DT_SPEC_INST_GET(inst), \
.config = DT_INST_PROP(inst, config), \
.adc_config = DT_INST_PROP(inst, adc_config), \
.adc_config = DT_INST_PROP(inst, adc_config) | \
(DT_INST_ENUM_IDX(inst, adc_mode) << 12) | \
(DT_INST_ENUM_IDX(inst, vbus_conversion_time_us) << 9) | \
(DT_INST_ENUM_IDX(inst, vshunt_conversion_time_us) << 6) | \
(DT_INST_ENUM_IDX(inst, temp_conversion_time_us) << 3) | \
DT_INST_ENUM_IDX(inst, avg_count), \
.current_lsb = DT_INST_PROP(inst, current_lsb_microamps), \
.cal = INA237_CAL_SCALING * DT_INST_PROP(inst, current_lsb_microamps) * \
DT_INST_PROP(inst, rshunt_micro_ohms) / 10000000000ULL, \
.cal = CAL_PRECISION_MULTIPLIER(DT_INST_PROP(inst, config)) * \
INA237_CAL_SCALING * DT_INST_PROP(inst, current_lsb_microamps) * \
DT_INST_PROP(inst, rshunt_micro_ohms) / 10000000ULL, \
.alert_config = DT_INST_PROP_OR(inst, alert_config, 0x01), \
.alert_gpio = GPIO_DT_SPEC_INST_GET_OR(inst, alert_gpios, {0}), \
}; \
Expand Down
10 changes: 8 additions & 2 deletions drivers/sensor/ina23x/ina237.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
#include <zephyr/drivers/sensor.h>

#define INA237_REG_CONFIG 0x00
#define INA237_CFG_HIGH_PRECISION BIT(4)

#define INA237_REG_ADC_CONFIG 0x01
#define INA237_REG_CALIB 0x02
#define INA237_REG_SHUNT_VOLT 0x04
#define INA237_REG_BUS_VOLT 0x05
#define INA237_REG_MASK 0x06
#define INA237_REG_DIETEMP 0x06
#define INA237_REG_CURRENT 0x07
#define INA237_REG_POWER 0x08
#define INA237_REG_ALERT 0x0B
Expand All @@ -37,9 +39,13 @@

struct ina237_data {
const struct device *dev;
uint16_t current;
int16_t current;
uint16_t bus_voltage;
uint32_t power;
int16_t die_temp;
#ifdef CONFIG_INA237_VSHUNT
int16_t shunt_voltage;
Copy link
Contributor

@xyzzy42 xyzzy42 Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be ifdef protected, so it doesn't take space in the struct unless enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done and pushed.

#endif
enum sensor_channel chan;
struct ina23x_trigger trigger;
};
Expand Down
5 changes: 0 additions & 5 deletions drivers/sensor/ina23x/ina23x_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@
#include <zephyr/drivers/i2c.h>
#include <zephyr/sys/util_macro.h>

/**
* @brief Macro used to test if the current's sign bit is set
*/
#define INA23X_CURRENT_SIGN_BIT BIT(15)

/**
* @brief Macro used to check if the current's LSB is 1mA
*/
Expand Down
53 changes: 53 additions & 0 deletions dts/bindings/sensor/ti,ina230.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,59 @@ compatible: "ti,ina230"
include: ti,ina23x-common.yaml

properties:
config:
type: int
deprecated: true
default: 0x0000
description: |
Value of the configuration register
e.g shunt voltage and bus voltage ADC conversion
times, ADC and averaging, and ADC operating mode.

alert-config:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add nl before alert-config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the latest changes that I've pushed.

type: int
description: Diag alert register, default matches the power-on reset value

adc-mode:
type: string
description: |
ADC channel conversion configuration.
Default is the power-on reset value.
default: "Bus and shunt voltage continuous"
enum:
- "Shutdown single shot"
- "Shunt Voltage single shot"
- "Bus Voltage single shot"
- "Bus and Shunt Voltage single shot"
- "Shutdown continuous"
- "Shunt voltage continuous"
- "Bus voltage continuous"
- "Bus and shunt voltage continuous"

vbus-conversion-time-us:
type: int
description: |
Vbus conversion time in microseconds.
Default is the power-on reset value.
default: 1100
enum: [140, 204, 332, 588, 1100, 2116, 4156, 8244]

vshunt-conversion-time-us:
type: int
description: |
Vshunt conversion time in microseconds.
Default is the power-on reset value.
default: 1100
enum: [140, 204, 332, 588, 1100, 2116, 4156, 8244]

avg-count:
type: int
description: |
Number of samples to average (applies to all inputs).
Default is the power-on reset value.
default: 1
enum: [1, 4, 16, 64, 128, 256, 512, 1024]

mask:
type: int
default: 0
Expand Down
Loading