Re: cwfg: flag sensor as invalid on bogus reads
Am Sat, Mar 27, 2021 at 12:00:32AM +0100 schrieb Klemens Nanni: > On Fri, Mar 26, 2021 at 11:22:32PM +0100, Patrick Wildt wrote: > > It's pretty normal for voltage to go up once AC is connected. In the > > end, afaik batteries are charged by applying voltage. Additionally > > if an external supply provides power, there's a smaller voltage drop. > I thought as much, thanks. > > > The "remaining battery time" to become invalid makes sense as well, > > I mean, with an AC it's gonna be endless and there's no way to measure > > the battery change. Well, the only thing it could maybe try and > > estimate is time until charged. > That's stuff for another diff (perhaps; I've heard guessing estimates > is hard). > > > What happens to battery percentage? Does it change while it's charging? > > > > As mentioned, connecting the charger will make the voltage go up, but > > the battery charge will not have changed, hence I expect the percentage > > to stay the same value, even though the voltage changes. But with time, > > percentage should go up. > the `percent0' values does increase steadily over time while AC is > plugged in. Cool, that's what I'd hope it does. So voltage goes up as expected, remaining battery is invalid (because it's on AC), and the percentage goes up. Sounds good to me. :) > > > @@ -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; > > > > I'd probably put a newline here, but that's just personal nitpicking. > Sure, committed with it. > > > I think it makes sense that outdated information should be marked > > invalid. Doing that upfront makes sense. Doing it for VCELL is > > not strictly necessary, but makes sense for consistency. > I did this for consistency, yes. >
Re: cwfg: flag sensor as invalid on bogus reads
On Fri, Mar 26, 2021 at 11:22:32PM +0100, Patrick Wildt wrote: > It's pretty normal for voltage to go up once AC is connected. In the > end, afaik batteries are charged by applying voltage. Additionally > if an external supply provides power, there's a smaller voltage drop. I thought as much, thanks. > The "remaining battery time" to become invalid makes sense as well, > I mean, with an AC it's gonna be endless and there's no way to measure > the battery change. Well, the only thing it could maybe try and > estimate is time until charged. That's stuff for another diff (perhaps; I've heard guessing estimates is hard). > What happens to battery percentage? Does it change while it's charging? > > As mentioned, connecting the charger will make the voltage go up, but > the battery charge will not have changed, hence I expect the percentage > to stay the same value, even though the voltage changes. But with time, > percentage should go up. the `percent0' values does increase steadily over time while AC is plugged in. > > @@ -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; > > I'd probably put a newline here, but that's just personal nitpicking. Sure, committed with it. > I think it makes sense that outdated information should be marked > invalid. Doing that upfront makes sense. Doing it for VCELL is > not strictly necessary, but makes sense for consistency. I did this for consistency, yes.
Re: cwfg: flag sensor as invalid on bogus reads
Am Fri, Mar 26, 2021 at 12:26:51AM +0100 schrieb Klemens Nanni: > 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? > It's pretty normal for voltage to go up once AC is connected. In the end, afaik batteries are charged by applying voltage. Additionally if an external supply provides power, there's a smaller voltage drop. The "remaining battery time" to become invalid makes sense as well, I mean, with an AC it's gonna be endless and there's no way to measure the battery change. Well, the only thing it could maybe try and estimate is time until charged. What happens to battery percentage? Does it change while it's charging? As mentioned, connecting the charger will make the voltage go up, but the battery charge will not have changed, hence I expect the percentage to stay the same value, even though the voltage changes. But with time, percentage should go up. > > 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.c25 Mar 2021 12:18:27 - 1.3 > +++ cwfg.c25 Mar 2021 12:25:31 - > @@ -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; I'd probably put a newline here, but that's just personal nitpicking. I think it makes sense that outdated information should be marked invalid. Doing that upfront makes sense. Doing it for VCELL is not strictly necessary, but makes sense for consistency. ok patrick@ > +#if NAPM > 0 > cwfg_power.battery_state = APM_BATT_UNKNOWN; > cwfg_power.ac_state = APM_AC_UNKNOWN; > cwfg_power.battery_life = 0; >
cwfg: flag sensor as invalid on bogus reads
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 - 1.3 +++ cwfg.c 25 Mar 2021 12:25:31 - @@ -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;
