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;