sensor: ina230: fix single-shot and shutdown modes #90898
Conversation
github-actions
bot
added
area: Sensors
github-actions
bot
requested review from
asemjonovs,
avisconti,
jeppenodgaard,
MaureenHelm,
teburd,
tristan-google,
ubieda,
vaishnavachath,
vanti and
yperess
JordanYates
force-pushed
the
250531_ina230
branch
from
7f1d445 to
e54caa8
Compare
Use the common sensor struct conversion functions instead of re-implementing the functionality. Signed-off-by: Jordan Yates <jordan@embeint.com>
Remove the property deprecated in f0f7f8b. Signed-off-by: Jordan Yates <jordan@embeint.com>
Reorganise the config and data structs by size of variables to reduce padding. Signed-off-by: Jordan Yates <jordan@embeint.com>
When configured into single-shot modes, samples need to be triggered by writing to the configuration register again. Once triggered, wait for the expected sampling duration and then start polling the status bit. Signed-off-by: Jordan Yates <jordan@embeint.com>
Fix sampling from the ADC shutdown mode by writing a single-shot mode before starting conversion, and returning to the shutdown mode after the conversion has completed. Signed-off-by: Jordan Yates <jordan@embeint.com>
Add a devicetree property to invert the direction of reported power and current flow, if needed to match the convention of some subsystem. Signed-off-by: Jordan Yates <jordan@embeint.com>
JordanYates
force-pushed
the
250531_ina230
branch
from
26c75c4 to
2bde84f
Compare
|
| while (1) { | ||
| ret = ina23x_reg_read_16(&config->bus, INA230_REG_MASK, ®); | ||
| if ((ret != 0) || (reg & INA230_REG_MASK_CNVR)) { | ||
| break; | ||
| } | ||
| k_sleep(K_MSEC(1)); | ||
| } |
There was a problem hiding this comment.
This can loop infinitely, use:
WAIT_FOR(
ina23x_reg_read_16(&config->bus, INA230_REG_MASK, ®) != 0 || (reg & INA230_REG_MASK_CNVR),
K_MSEC(5), /* Use whatever reasonable timeout you think is right here */
k_msleep(1));| if ((chan == SENSOR_CHAN_ALL) || (chan == SENSOR_CHAN_VOLTAGE)) { | ||
| ret = ina23x_reg_read_16(&config->bus, INA230_REG_BUS_VOLT, &data->bus_voltage); | ||
| if (ret < 0) { | ||
| LOG_ERR("Failed to read bus voltage"); | ||
| LOG_ERR("Failed to read %s", "bus voltage"); |
There was a problem hiding this comment.
Why use formatting here? This is unnecessarily expensive
There was a problem hiding this comment.
Using the same format string and only changing the argument allows the format string to reused for all log instances, saving ROM. It is a somewhat common pattern in Zephyr.
There was a problem hiding this comment.
Interesting, this might be something we bring up with the TSC, we use tokenized logging so each string regardless of length only costs us 4 bytes, using string formats adds runtime costs and ROM. @keith-zephyr what do you think?
There was a problem hiding this comment.
Last time I looked at that feature #42840 hadn't been implemented, which sort of defeated the point.
In other PR's I have been requested to use the form I used here, but I see why its unhelpful for dictionary logging.
There was a problem hiding this comment.
Yeah, dictionary logging is kinda bad because it's per build. We use https://pigweed.dev/pw_log_tokenized/#module-pw-log-tokenized which creates a hash of all the strings and allows the dictionary to work across builds.
There was a problem hiding this comment.
This change seems breaking, maybe add something to the release notes?
There was a problem hiding this comment.
Which change are you referring to? config has been deprecated for almost 2 years, and the enum that was renamed literally never worked.
There was a problem hiding this comment.
Removing the config attribute and renaming the Shutdown continuous enum. Even if it's deprecated, I don't see any harm in updating the release notes to specify that it was removed and that the enum was changed.
When not running in one of the continuous modes, sampling needs to be explicitly triggered by a write to the configuration register. Extra steps need to be taken when idling in the shutdown mode, since writing the constant configuration value will just return to shutdown without sampling.
Includes other minor updates to the driver including common logging strings, using common conversion functions, more efficient struct packing and optional inversion of reported current and power flow.