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 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.
> 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?
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:
