On Wed, Mar 24 2021, Klemens Nanni <k...@openbsd.org> wrote:
> On Wed, Mar 24, 2021 at 12:49:51AM +0100, Jeremie Courreges-Anglas wrote:
>
>> >> > @@ -202,6 +217,12 @@ cwfg_attach(struct device *parent, struc
>> >> >
>> >> > sensor_task_register(sc, cwfg_update_sensors, 5);
>> >> >
>> >> > +#if NAPM > 0
>> >> > + /* make sure we have the apm state initialized before apm
>> >> > attaches */
>> >> > + cwfg_update_sensors(sc);
>> >> > + apm_setinfohook(cwfg_apminfo);
>> >> > +#endif
>> >> > +
>> >> > printf("\n");
>> >> > }
>> >> >
>> >> > @@ -324,6 +345,7 @@ cwfg_update_sensors(void *arg)
>> >> > uint32_t vcell, rtt, tmp;
>> >> > uint8_t val;
>> >> > int error, n;
>> >> > + u_char bat_percent;
>> >> >
>> >> > if ((error = cwfg_lock(sc)) != 0)
>> >> > return;
>> >> > @@ -348,6 +370,7 @@ cwfg_update_sensors(void *arg)
>> >> > if ((error = cwfg_read(sc, SOC_HI_REG, &val)) != 0)
>> >> > goto done;
>> >> > if (val != 0xff) {
>> >> > + bat_percent = val;
>> >> > sc->sc_sensor[CWFG_SENSOR_SOC].value = val * 1000;
>> >> > sc->sc_sensor[CWFG_SENSOR_SOC].flags &=
>> >> > ~SENSOR_FINVALID;
>> >> > }
>> >>
>> >> If val == 0xff bat_percent is unitialized. Note that in this error
>> >> condition the sensors code leaves sc->sc_sensor[CWFG_SENSOR_SOC].value
>> >> alone.
>> > Oops. Both `val' and `rtt' can be "useless" and we could end up with
>> > a partially updated `cwfg_power'.
>>
>> With the current code, rtt is always meaningful. I was really talking
>> only about bat_percent being possibly uninitialized.
> Yes, `rtt' always means something but the code does not always populate
> the struct member with it from which I read unconditionally - this is
> what I meant.
>
> Hence, instead of always reading from it and possibly getting a stale
> value, the second diff makes sure that each update cycle resets every
> value and only updates those that come from a fresh reading.
>
>> > If we always reset all values up front and then update whatever is
>> > possible, we can avoid the intermediate variable completely and end up
>> > with `cwfg_power' providing consistent readings.
>>
>> Except if reading one register fails and we take the goto done shortcut.
>> In that case sensors and apm get out of sync, which is bad IMHO.
> My intention was that I'd rather provide an all reset struct with a
> single value updated that is known to be good rather than lazily
> updating whatever is possible.
>
>> >> So to keep apm and sensors in sync I think it would be better to reuse
>> >> the cached sc->sc_sensor[CWFG_SENSOR_SOC].value.
>> > I did `sc->sc_sensor[CWFG_SENSOR_SOC].value / 1000' first but ended up
>> > with bogus values, hence the `bat_percent' buffer to avoid arithmetics.
>> >
>> >
>> >> I don't know whether the error condition mentioned above happens
>> >> frequently with this driver. Reporting APM_BATT_ABSENT (and similar for
>> >> sensors) would be useful, and could be done in a subsequent step.
>> > But that isn't the case? From apm(4):
>> >
>> > APM_BATTERY_ABSENT
>> > No battery installed.
>> >
>> > The driver just can't tell us enough, but the battery is still there
>> > (we get good percentage/liftime readings), so
>> >
>> > APM_BATT_UNKNOWN
>> > Cannot read the current battery state.
>> >
>> > is only appropiate here imho.
>> >
>> >
>> >> What's the underlying hardware, does it involve a pluggable battery?
>> > Nope, Pinebook Pro with one internal battery and AC.
>>
>> Since the battery can't be removed in your pinebook, then obviously
>> APM_BATTERY_ABSENT isn't very meaningful. But maybe the CellWise
>> hardware supports pluggable batteries. Anyway...
>>
>> The diff below is mostly your diff, except:
>> - statically initialize cwfg_power with proper values. That way there's
>> a reason less for initializing it early by forcefully calling
>> cwfg_update_sensors().
> I like that, thanks.
>
>> - only update cwfg_power when we already fetched new values for sensors.
> As outlined above, I'd like to avoid this approach.
>
> Let's assume the sensor works fine and all values are read correctly,
> then apm shows something like this:
>
> Battery state: high, 78% remaining, 323 minutes life estimate
>
> Now if for whatever reason in cwfg_update_sensors() only readings for
> RTT (battery remaining minutes) start failing, your diff would keep
> updating SOC (battery percent) without touching RTT, so apm would trend
> like so
>
> Battery state: high, 70% remaining, 323 minutes life estimate
> Battery state: high, 50% remaining, 323 minutes life estimate
>
> Or not? To deal with this I reset everything to invalid/unknown, such
> that in the above scenario apm would trend like so
>
> Battery state: high, 78% remaining, 323 minutes life estimate
> Battery state: high, 70% remaining, unknown life estimate
> Battery state: high, 50% remaining, unknown life estimate
>
> Which reads arguably better to me.
>
> I see how this makes hw.sensors and apm diverge, i.e. the former stales
> while the latter does reflect a change, but I'd argue to also fix
> hw.sensors instead of keeping apm in sync at the cost of misleading
> information for the user.
>
> Does that make sense?
I was arguing for keeping them in sync mostly so that someone (probably
you) would feel the need to improve both at the same time. Looks like
we agree on the end goal, just differing on the approach for the first
step, so I'll stop nitpicking.
>> I hope that this approach will make changing the cwfg_update_sensors()
>> code easier.
> A follow-up diff to also make hw.sensors recognise failed readings as
> such, i.e. bring it in line with my apm logic, could be
>
> /* SOC */
> if (val != 0xff) {
> ...
> -}
> +} else
> + sc->sc_sensor[CWFG_SENSOR_SOC].flags |= SENSOR_FINVALID;
>
> I have yet to test that idea but it seems SENSOR_FINVALID is only ever
> set during attach and then only ever unset in case of good readings,
> so it seems hw.sensors values currently never go back to invalid state.
It seems fair to invalidate both sensors and apm state if a read fails
or we get an "invalid" value.
>> If it works for you and you agree with this tweaked proposal, ok jca@
> Not quite (yet).
>
> Here's the third round, still following my idea, but of course with your
> idea of static initialisation merged to save a call to
> cwfg_update_sensors().
>
> More feedback? Objections? OK?
No objection from me, ok jca@
> Index: dev/fdt/cwfg.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 cwfg.c
> --- dev/fdt/cwfg.c 22 Mar 2021 18:37:26 -0000 1.2
> +++ dev/fdt/cwfg.c 24 Mar 2021 00:57:00 -0000
> @@ -32,12 +32,15 @@
> #include <sys/malloc.h>
> #include <sys/sensors.h>
>
> +#include <machine/apmvar.h>
> #include <machine/fdt.h>
>
> #include <dev/ofw/openfirm.h>
>
> #include <dev/i2c/i2cvar.h>
>
> +#include "apm.h"
> +
> #define VERSION_REG 0x00
> #define VCELL_HI_REG 0x02
> #define VCELL_HI_MASK 0x3f
> @@ -119,6 +122,22 @@ struct cfdriver cwfg_cd = {
> NULL, "cwfg", DV_DULL
> };
>
> +#if NAPM > 0
> +struct apm_power_info cwfg_power = {
> + .battery_state = APM_BATT_UNKNOWN,
> + .ac_state = APM_AC_UNKNOWN,
> + .battery_life = 0,
> + .minutes_left = -1,
> +};
> +
> +int
> +cwfg_apminfo(struct apm_power_info *info)
> +{
> + memcpy(info, &cwfg_power, sizeof(*info));
> + return 0;
> +}
> +#endif
> +
> int
> cwfg_match(struct device *parent, void *match, void *aux)
> {
> @@ -202,6 +221,10 @@ cwfg_attach(struct device *parent, struc
>
> sensor_task_register(sc, cwfg_update_sensors, 5);
>
> +#if NAPM > 0
> + apm_setinfohook(cwfg_apminfo);
> +#endif
> +
> printf("\n");
> }
>
> @@ -325,6 +348,15 @@ cwfg_update_sensors(void *arg)
> uint8_t val;
> int error, n;
>
> +#if NAPM > 0
> + /* reset previous reads so apm(4) never gets stale values
> + * in case of transient cwfg_read() failures below */
> + cwfg_power.battery_state = APM_BATT_UNKNOWN;
> + cwfg_power.ac_state = APM_AC_UNKNOWN;
> + cwfg_power.battery_life = 0;
> + cwfg_power.minutes_left = -1;
> +#endif
> +
> if ((error = cwfg_lock(sc)) != 0)
> return;
>
> @@ -350,6 +382,11 @@ cwfg_update_sensors(void *arg)
> if (val != 0xff) {
> sc->sc_sensor[CWFG_SENSOR_SOC].value = val * 1000;
> sc->sc_sensor[CWFG_SENSOR_SOC].flags &= ~SENSOR_FINVALID;
> +#if NAPM > 0
> + cwfg_power.battery_state = val > sc->sc_alert_level ?
> + APM_BATT_HIGH : APM_BATT_LOW;
> + cwfg_power.battery_life = val;
> +#endif
> }
>
> /* RTT */
> @@ -362,6 +399,9 @@ cwfg_update_sensors(void *arg)
> if (rtt != 0x1fff) {
> sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
> sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
> +#if NAPM > 0
> + cwfg_power.minutes_left = rtt;
> +#endif
> }
>
> done:
>
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
