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 <[email protected]>
Remove the property deprecated in f0f7f8b. Signed-off-by: Jordan Yates <[email protected]>
Reorganise the config and data structs by size of variables to reduce padding. Signed-off-by: Jordan Yates <[email protected]>
JordanYates
force-pushed
the
250531_ina230
branch
from
26c75c4 to
2bde84f
Compare
drivers/sensor/ti/ina23x/ina230.c
Outdated
| 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));There was a problem hiding this comment.
Cool macro but it doesn't work for this use case.
Read failures will just break the loop with no error code, and you don't know whether the bit is set or not without testing it again outside the loop.
| 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.
dts/bindings/sensor/ti,ina230.yaml
Outdated
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 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Note the removable of the property and enum value that never worked. Signed-off-by: Jordan Yates <[email protected]>
JordanYates
force-pushed
the
250531_ina230
branch
from
2bde84f to
c687f79
Compare
|
|
@JordanYates RC1 is out so we're on bug fixes only. Given this looks like a bug fix, can you please file a GitHub issue and attach so we can merge it in for RC2? @MaureenHelm @ubieda @teburd @yperess any of you able to review this and approve such that we can merge for RC2? |
@dkalowsk I would recommend not necessarily being too strict about asking to file GitHub issues for bug fixes going into RC2 as there will be a lot of them to tackle this week, and as long as they're clearly described in the PR description and use the "bug" label it should be fine. Post RC2 is definitely a different story and the extra paperwork makes a lot more sense there. |
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.