Follow-up to "arm64: make cwfg(4) report battery information to apm(4)".
This driver continues to report stale hw.sensors values when reading
them fails, which can easily be observed on a Pinebook Pro after
plugging in the AC cable.
Running on battery looks like this (note sensors and apm are in sync):
$ sysctl hw.sensors
hw.sensors.cwfg0.volt0=3.68 VDC (battery voltage)
hw.sensors.cwfg0.raw0=104 (battery remaining minutes)
hw.sensors.cwfg0.percent0=27.00% (battery percent)
$ apm
Battery state: high, 27% remaining, 104 minutes life estimate
A/C adapter state: not known
Performance adjustment mode: auto (408 MHz)
When I plug in the AC cable, `raw0' jumps around considerable one or two
times before stalling on the last value (note how `percent0' stayed the
same while `raw0' trippled):
$ sysctl hw.sensors
hw.sensors.cwfg0.volt0=3.98 VDC (battery voltage)
hw.sensors.cwfg0.raw0=359 (battery remaining minutes)
hw.sensors.cwfg0.percent0=27.00% (battery percent)
$ apm
Battery state: high, 27% remaining, unknown life estimate
A/C adapter state: not known
Performance adjustment mode: auto (408 MHz)
`raw0' aka. `CWFG_SENSOR_RTT' stops yielding valid values as long as AC
is plugged in (no idea if by design or a bug).
Hence hw.sensors.cwf0 values become incoherent and drift away from apm's
output which --due to the reset logic discussed in the aforementioned
tech@ thread-- properly picks up the stalled value as "unknown".
I see two approaches to fix this:
1. simple but less user-friendly: flag sensors invalid upfront in apm's
fashion and mark them OK iff they yield valid values; this is what
other drivers such as rktemp(4) do, but the consequence/intention of
`SENSOR_FINVALID' is sysctl(8) and systat(8) skipping such sensors,
i.e. above sysctl output would omit the `raw0' line if AC is plugged
in (arguably better than printing outdated/misleading values).
2. elaborate but informative: set sensor value/status to 0/unknown like
acpibat(4) does for example; the advantage is that sensors no longer
come and go but could look like this:
hw.sensors.cwfg0.raw0=0 (battery remaining minutes), UNKNOWN
I'd do prefer this but am not yet sure if that's how the sensor
framework is supposed to be used and/or I'd need to tinker with the
code (on multiple notebooks/sensors) to see if it works out.
Either way, diff below implements the first approach such that we avoid
bogus sysctl/systat lines and hw.sensors gets in sync with apm.
One could still switch to the second approach afterwards.
Feedback? Objections? OK?
Index: cwfg.c
===================================================================
RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
retrieving revision 1.3
diff -u -p -r1.3 cwfg.c
--- cwfg.c 25 Mar 2021 12:18:27 -0000 1.3
+++ cwfg.c 25 Mar 2021 12:25:31 -0000
@@ -348,9 +348,12 @@ cwfg_update_sensors(void *arg)
uint8_t val;
int error, n;
-#if NAPM > 0
- /* reset previous reads so apm(4) never gets stale values
+ /* invalidate all previous reads to avoid stale/incoherent values
* in case of transient cwfg_read() failures below */
+ sc->sc_sensor[CWFG_SENSOR_VCELL].flags |= SENSOR_FINVALID;
+ sc->sc_sensor[CWFG_SENSOR_SOC].flags |= SENSOR_FINVALID;
+ sc->sc_sensor[CWFG_SENSOR_RTT].flags |= SENSOR_FINVALID;
+#if NAPM > 0
cwfg_power.battery_state = APM_BATT_UNKNOWN;
cwfg_power.ac_state = APM_AC_UNKNOWN;
cwfg_power.battery_life = 0;
