Re: arm64: make cwfg(4) report battery information to apm(4)
On Wed, Mar 24 2021, Klemens Nanni 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 lif
Re: arm64: make cwfg(4) report battery information to apm(4)
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 ar
Re: arm64: make cwfg(4) report battery information to apm(4)
On Mon, Mar 22 2021, Klemens Nanni wrote:
> Better diff at the end thanks to jca's eyeballing, see comments inline.
>
> kettenis: I see room for improvement in our subsystems and their
> interactions, but I don't think the current situation is bad enough to
> leave those bits out for now.
>
> Feedback? OK?
>
[...]
>> > @@ -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.
> 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.
[...]
>> 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().
- only update cwfg_power when we already fetched new values for sensors.
I hope that this approach will make changing the cwfg_update_sensors()
code easier.
If it works for you and you agree with this tweaked proposal, ok jca@
Index: cwfg.c
===
RCS file: /d/cvs/src/sys/dev/fdt/cwfg.c,v
retrieving revision 1.2
diff -u -p -p -u -r1.2 cwfg.c
--- cwfg.c 22 Mar 2021 18:37:26 - 1.2
+++ cwfg.c 23 Mar 2021 23:49:22 -
@@ -32,12 +32,15 @@
#include
#include
+#include
#include
#include
#include
+#include "apm.h"
+
#defineVERSION_REG 0x00
#defineVCELL_HI_REG0x02
#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");
}
@@ -350,6 +373,11 @@ cwfg_update_sensors(void *arg)
if (val != 0xff) {
sc->sc_sensor[CWFG_SENSOR_SOC].va
Re: arm64: make cwfg(4) report battery information to apm(4)
Better diff at the end thanks to jca's eyeballing, see comments inline.
kettenis: I see room for improvement in our subsystems and their
interactions, but I don't think the current situation is bad enough to
leave those bits out for now.
Feedback? OK?
On Mon, Mar 22, 2021 at 01:21:02AM +0100, Jeremie Courreges-Anglas wrote:
> > Index: dev/fdt/cwfg.c
> > ===
> > RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 cwfg.c
> > --- dev/fdt/cwfg.c 10 Jun 2020 17:51:21 - 1.1
> > +++ dev/fdt/cwfg.c 20 Mar 2021 23:43:13 -
> > @@ -32,12 +32,15 @@
> > #include
> > #include
> >
> > +#include
> > #include
> >
> > #include
> >
> > #include
> >
> > +#include "apm.h"
> > +
> > #defineVERSION_REG 0x00
> > #defineVCELL_HI_REG0x02
> > #define VCELL_HI_MASK 0x3f
> > @@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
> > NULL, "cwfg", DV_DULL
> > };
> >
> > +int cwfg_apminfo(struct apm_power_info *info);
>
> Why out of the #if NAPM > 0 check?
Copied over from the loongson driver; the declaration doesn't hurt
outside but isn't needed either. I've removed it.
> > +#if NAPM > 0
> > +struct apm_power_info cwfg_power;
> > +
> > +int
> > +cwfg_apminfo(struct apm_power_info *info)
> > +{
> > + bcopy(&cwfg_power, info, sizeof(*info));
>
> There's no overlap between source and destination, better use memcpy.
> (Else, better use memmove.)
Right, I also copied this over from loongson (we can dust that one off
as well later on).
>
> > + return 0;
> > +}
> > +#endif
> > +
> > int
> > cwfg_match(struct device *parent, void *match, void *aux)
> > {
> > @@ -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'.
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.
>
> > @@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
> > sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
> > sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
> > }
> > +
> > +#if NAPM > 0
> > + cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
> > + APM_BATT_HIGH : APM_BATT_LOW;
>
> It's cool that this driver keeps track of the "alert level". acpibat(4)
> also does that on amd64, but the apm(4)-through-acpi(4) userland
> interface just hardcodes levels:
>
> /* running on battery */
> pi->battery_life = remaining * 100 / capacity;
> if (pi->battery_life > 50)
> pi->battery_state = APM_BATT_HIGH;
> else if (pi->battery_life > 25)
> pi->battery_state = APM_BATT_LOW;
> else
> pi->battery_state = APM_BATT_CRITICAL;
>
> Maybe the levels reported by hardware couldn't be trusted? Or maybe
> those hardcoded values were deemed good enough, back then. Anyway, on
> this new hardware I think it makes sense to just trust the exported
> values and later act if we get reports.
Yes, I explicitly want to use what hardware provides and we can still
tweak it to more sensible values if need be.
>
> > + cwfg_power.ac_state = APM_AC_UNKNOWN;
>
> This kinda points out the need for an AC driver on this platform.
> If done in another driver, the code will need to be changed. But this
> looks acceptable to me for now.
Yup.
>
> > + cwfg_power.battery_life = bat_percent;
>
> 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
Re: arm64: make cwfg(4) report battery information to apm(4)
Klemens Nanni wrote: > > So which is better: > > > > (1) try to emit some information for the people who quicky-use the apm > > interface > > > > (2) change apm to not print those lines on architectures where we are > > unsure. > > > > I think (1) is acceptable for a tool which has never promised perfect > > accuracy. > I concur (obviously). > > > I suspect hard-wiring this to one driver is going to be better than scanning > > the sensor list and heuristically determining which specific sensors to > > look at, > > because the only good selector now is strcmp(sensor->desc, "battery > > remaining minutes") > > yuck. > To be fair, kettenis proposed a rough idea to have simpler/faster checks > than strcmp(). With a simple flag you wouldn't need any heuristics > either but let the sensor framework -which has all the pieces- do the > work and put a stamp on the ready-to-use value saying "use this as > battery level". that simple flag will probably be too simple, and it won't be long before someone in another architecture wants it to mean something more, or wants another flag, and before long there will be zeal to flag carrying to the sensor framework. and what happens if two drivers set the flag? Another way of doing this is for an platform to name the sensor driver which has the info, and then search the sensor free. Or, simply write per-platform code that finds it's own stuff.
Re: arm64: make cwfg(4) report battery information to apm(4)
On Sun, Mar 21 2021, Mark Kettenis wrote: >> Date: Sun, 21 Mar 2021 17:22:05 +0100 >> From: Klemens Nanni >> >> On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote: >> > > Date: Sun, 21 Mar 2021 01:02:53 +0100 >> > > From: Klemens Nanni >> > > >> > > apm(4/arm64) merely provides an all zero/unknown stub for those values, >> > > e.g. apm(8) output is useless. >> > > >> > > Hardware sensors however provide the information: >> > > >> > > $ sysctl hw.sensors >> > > hw.sensors.rktemp0.temp0=32.22 degC (CPU) >> > > hw.sensors.rktemp0.temp1=33.89 degC (GPU) >> > > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage) >> > > hw.sensors.cwfg0.raw0=259 (battery remaining minutes) >> > > hw.sensors.cwfg0.percent0=58.00% (battery percent) >> > > >> > > Diff below simply copies them over using apm_setinfohook() >> > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same): >> > > >> > > $ apm >> > > Battery state: high, 58% remaining, 259 minutes life estimate >> > > A/C adapter state: not known >> > > Performance adjustment mode: auto (408 MHz) >> > > >> > > Feedback? OK? >> > >> > This doesn't scale. >> What exactly does not scale? The way how various drivers hook into >> apm(4)? I'm not familiar enough (yet) with the framework(s) to judge >> their overall design and/or interop strategy off hand. > > Since there is no firmware abstraction like on i386 (and to some > extent amd64) there will be many drivers that provide battery > informarion on armv7/arm64. > >> > The whole apm(4) interface is too closely tied to the original APM >> > BIOS model. Even with acpi(4) it is a bit of a square peg for a round >> > hole, for example on machines with more than one battery. >> Right, there is hardware which doesn't fit this model and our apm* is >> not exactly up to speed with those, but I still don't see actual issues >> (as per above). >> >> > I'm not even sure apm(8) should bother reporting some sort of battery >> > status. >> apm* showing battery information seems only naturally to me since that >> is the driver/daemon/tool one issues power commands with, often based >> on such information - think of `apmd -z 15': given that I want my box >> to safely suspend around 15% remaining battery, it seems only >> consequential that apm* is the same tool I query such information with. > > apmd(8) and apm(8) are not the same tool though. Having a deamon to > run scripts on certain power-state changes makes some sense, but I'd > argue that sensorsd(8) could be a reasonable place to implement this > as well. When I dived into the apm(4) and apmd(8) code I noted how redundant it was considering sensorsd(8). But the user interface of sensorsd(8) needs a fair bit of pondering before it can be proposed as a replacement. sensorsd(8) is fairly generic and its configuration format, while extensible, isn't easy to use. To be fair I didn't even try to make it execute stuff based on battery level. I suspect it would just be useful to execute commands based on changes in one driver, when what you need to mimic apmd(8) is to merge the values coming from different drivers. > The need to run apmd(8) in order to suspend/hibernate is a bit of an > historical wart. It's not clear to me right now why this was needed and why it's not needed anymore. (I'm just talking about basic suspend/hibernate, not about the nifty -z/-Z additions in apmd(8)). > And we already removed powermanagement state support > from apmd(8). These days, users should probably just use > /etc/sysctl.conf to set the powermanagement state instead of starting > apmd(8) with the appropriate -A, -H or -L option. Indeed, -A/-H/-L are just syntactical sugar. I wouldn't mind seeing them go. >> > But if it does, my suggestion would be to make it use the >> > sensors framework. That would need some work though such that drivers >> > can mark sensors as providing battery information. Maybe add a >> > SENSOR_FBATTERY flag? Why the 'F' and not just SENSOR_BATTERY? >> You mean apm(8) directly using sensor_find(9) or so to look for >> suitable sensors instead of relaying values throuh apm(4)? > > That's a possibility. But having apm(4) do that might be a useful > middle ground as it would mean you don't have to change the userland > tools just yet. I'm not familiar at all with the sensors framework, but if presenting the same simple apm(4) interface is doable using sensors, then you can count me as a reviewer! :) (I don't know where the apm(4) interface comes from, but it looks like we have a bunch of ports also using it.) > My main concern with your diff is having to add APM > hooks in all the drivers... Or maybe we need a nicer way to register APM hooks. IIUC apm_setinfohook was copied from miod's work on loongson where a single driver is in charge, depending on the underlying hardware. So it probably can't solve all use cases as is. >> It sounds simpler in principal but that's just my naive guess; >> I'll take a curious look
Re: arm64: make cwfg(4) report battery information to apm(4)
On Sun, Mar 21, 2021 at 11:56:42AM -0600, Theo de Raadt wrote: > The sensor framework generally does not know where a sensor is. A > sensor could reside on some device which has been plugged in, rather > than be the primary sensor. > > But the users of apm are only hoping for best effort. > > meaning "some information", not "perfect information". Many PC BIOS > have lied in these fields before also, yet all the apm command users > survived. That's what my diff does really: best effort for some information. > So which is better: > > (1) try to emit some information for the people who quicky-use the apm > interface > > (2) change apm to not print those lines on architectures where we are unsure. > > I think (1) is acceptable for a tool which has never promised perfect > accuracy. I concur (obviously). > I suspect hard-wiring this to one driver is going to be better than scanning > the sensor list and heuristically determining which specific sensors to look > at, > because the only good selector now is strcmp(sensor->desc, "battery remaining > minutes") > yuck. To be fair, kettenis proposed a rough idea to have simpler/faster checks than strcmp(). With a simple flag you wouldn't need any heuristics either but let the sensor framework -which has all the pieces- do the work and put a stamp on the ready-to-use value saying "use this as battery level".
Re: arm64: make cwfg(4) report battery information to apm(4)
On Sun, Mar 21 2021, Klemens Nanni wrote:
> apm(4/arm64) merely provides an all zero/unknown stub for those values,
> e.g. apm(8) output is useless.
>
> Hardware sensors however provide the information:
>
> $ sysctl hw.sensors
> hw.sensors.rktemp0.temp0=32.22 degC (CPU)
> hw.sensors.rktemp0.temp1=33.89 degC (GPU)
> hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
> hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
> hw.sensors.cwfg0.percent0=58.00% (battery percent)
>
> Diff below simply copies them over using apm_setinfohook()
> (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
>
> $ apm
> Battery state: high, 58% remaining, 259 minutes life estimate
> A/C adapter state: not known
> Performance adjustment mode: auto (408 MHz)
>
> Feedback? OK?
Feedback:
> Index: dev/fdt/cwfg.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 cwfg.c
> --- dev/fdt/cwfg.c10 Jun 2020 17:51:21 - 1.1
> +++ dev/fdt/cwfg.c20 Mar 2021 23:43:13 -
> @@ -32,12 +32,15 @@
> #include
> #include
>
> +#include
> #include
>
> #include
>
> #include
>
> +#include "apm.h"
> +
> #define VERSION_REG 0x00
> #define VCELL_HI_REG0x02
> #define VCELL_HI_MASK 0x3f
> @@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
> NULL, "cwfg", DV_DULL
> };
>
> +int cwfg_apminfo(struct apm_power_info *info);
Why out of the #if NAPM > 0 check?
> +#if NAPM > 0
> +struct apm_power_info cwfg_power;
> +
> +int
> +cwfg_apminfo(struct apm_power_info *info)
> +{
> + bcopy(&cwfg_power, info, sizeof(*info));
There's no overlap between source and destination, better use memcpy.
(Else, better use memmove.)
> + return 0;
> +}
> +#endif
> +
> int
> cwfg_match(struct device *parent, void *match, void *aux)
> {
> @@ -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.
> @@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
> sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
> sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
> }
> +
> +#if NAPM > 0
> + cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
> + APM_BATT_HIGH : APM_BATT_LOW;
It's cool that this driver keeps track of the "alert level". acpibat(4)
also does that on amd64, but the apm(4)-through-acpi(4) userland
interface just hardcodes levels:
/* running on battery */
pi->battery_life = remaining * 100 / capacity;
if (pi->battery_life > 50)
pi->battery_state = APM_BATT_HIGH;
else if (pi->battery_life > 25)
pi->battery_state = APM_BATT_LOW;
else
pi->battery_state = APM_BATT_CRITICAL;
Maybe the levels reported by hardware couldn't be trusted? Or maybe
those hardcoded values were deemed good enough, back then. Anyway, on
this new hardware I think it makes sense to just trust the exported
values and later act if we get reports.
> + cwfg_power.ac_state = APM_AC_UNKNOWN;
This kinda points out the need for an AC driver on this platform.
If done in another driver, the code will need to be changed. But this
looks acceptable to me for now.
> + cwfg_power.battery_life = bat_percent;
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 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.
What's the underlying hardware, does it involve a pluggable battery?
> + cwfg_power.minutes_left = sc->sc_sensor[CWFG_SENSOR_RTT].value;
> +#endif
>
> done:
> cwfg_unlock(sc);
>
--
jca | P
Re: arm64: make cwfg(4) report battery information to apm(4)
The sensor framework generally does not know where a sensor is. A sensor could reside on some device which has been plugged in, rather than be the primary sensor. But the users of apm are only hoping for best effort. meaning "some information", not "perfect information". Many PC BIOS have lied in these fields before also, yet all the apm command users survived. So which is better: (1) try to emit some information for the people who quicky-use the apm interface (2) change apm to not print those lines on architectures where we are unsure. I think (1) is acceptable for a tool which has never promised perfect accuracy. I suspect hard-wiring this to one driver is going to be better than scanning the sensor list and heuristically determining which specific sensors to look at, because the only good selector now is strcmp(sensor->desc, "battery remaining minutes") yuck. Mark Kettenis wrote: > > Date: Sun, 21 Mar 2021 17:22:05 +0100 > > From: Klemens Nanni > > > > On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote: > > > > Date: Sun, 21 Mar 2021 01:02:53 +0100 > > > > From: Klemens Nanni > > > > > > > > apm(4/arm64) merely provides an all zero/unknown stub for those values, > > > > e.g. apm(8) output is useless. > > > > > > > > Hardware sensors however provide the information: > > > > > > > > $ sysctl hw.sensors > > > > hw.sensors.rktemp0.temp0=32.22 degC (CPU) > > > > hw.sensors.rktemp0.temp1=33.89 degC (GPU) > > > > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage) > > > > hw.sensors.cwfg0.raw0=259 (battery remaining minutes) > > > > hw.sensors.cwfg0.percent0=58.00% (battery percent) > > > > > > > > Diff below simply copies them over using apm_setinfohook() > > > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same): > > > > > > > > $ apm > > > > Battery state: high, 58% remaining, 259 minutes life estimate > > > > A/C adapter state: not known > > > > Performance adjustment mode: auto (408 MHz) > > > > > > > > Feedback? OK? > > > > > > This doesn't scale. > > What exactly does not scale? The way how various drivers hook into > > apm(4)? I'm not familiar enough (yet) with the framework(s) to judge > > their overall design and/or interop strategy off hand. > > Since there is no firmware abstraction like on i386 (and to some > extent amd64) there will be many drivers that provide battery > informarion on armv7/arm64. > > > > The whole apm(4) interface is too closely tied to the original APM > > > BIOS model. Even with acpi(4) it is a bit of a square peg for a round > > > hole, for example on machines with more than one battery. > > Right, there is hardware which doesn't fit this model and our apm* is > > not exactly up to speed with those, but I still don't see actual issues > > (as per above). > > > > > I'm not even sure apm(8) should bother reporting some sort of battery > > > status. > > apm* showing battery information seems only naturally to me since that > > is the driver/daemon/tool one issues power commands with, often based > > on such information - think of `apmd -z 15': given that I want my box > > to safely suspend around 15% remaining battery, it seems only > > consequential that apm* is the same tool I query such information with. > > apmd(8) and apm(8) are not the same tool though. Having a deamon to > run scripts on certain power-state changes makes some sense, but I'd > argue that sensorsd(8) could be a reasonable place to implement this > as well. > > The need to run apmd(8) in order to suspend/hibernate is a bit of an > historical wart. And we already removed powermanagement state support > from apmd(8). These days, users should probably just use > /etc/sysctl.conf to set the powermanagement state instead of starting > apmd(8) with the appropriate -A, -H or -L option. > > > > But if it does, my suggestion would be to make it use the > > > sensors framework. That would need some work though such that drivers > > > can mark sensors as providing battery information. Maybe add a > > > SENSOR_FBATTERY flag? > > You mean apm(8) directly using sensor_find(9) or so to look for > > suitable sensors instead of relaying values throuh apm(4)? > > That's a possibility. But having apm(4) do that might be a useful > middle ground as it would mean you don't have to change the userland > tools just yet. My main concern with your diff is having to add APM > hooks in all the drivers... > > > It sounds simpler in principal but that's just my naive guess; > > I'll take a curious look into whether/how this could work. >
Re: arm64: make cwfg(4) report battery information to apm(4)
> Date: Sun, 21 Mar 2021 17:22:05 +0100 > From: Klemens Nanni > > On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote: > > > Date: Sun, 21 Mar 2021 01:02:53 +0100 > > > From: Klemens Nanni > > > > > > apm(4/arm64) merely provides an all zero/unknown stub for those values, > > > e.g. apm(8) output is useless. > > > > > > Hardware sensors however provide the information: > > > > > > $ sysctl hw.sensors > > > hw.sensors.rktemp0.temp0=32.22 degC (CPU) > > > hw.sensors.rktemp0.temp1=33.89 degC (GPU) > > > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage) > > > hw.sensors.cwfg0.raw0=259 (battery remaining minutes) > > > hw.sensors.cwfg0.percent0=58.00% (battery percent) > > > > > > Diff below simply copies them over using apm_setinfohook() > > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same): > > > > > > $ apm > > > Battery state: high, 58% remaining, 259 minutes life estimate > > > A/C adapter state: not known > > > Performance adjustment mode: auto (408 MHz) > > > > > > Feedback? OK? > > > > This doesn't scale. > What exactly does not scale? The way how various drivers hook into > apm(4)? I'm not familiar enough (yet) with the framework(s) to judge > their overall design and/or interop strategy off hand. Since there is no firmware abstraction like on i386 (and to some extent amd64) there will be many drivers that provide battery informarion on armv7/arm64. > > The whole apm(4) interface is too closely tied to the original APM > > BIOS model. Even with acpi(4) it is a bit of a square peg for a round > > hole, for example on machines with more than one battery. > Right, there is hardware which doesn't fit this model and our apm* is > not exactly up to speed with those, but I still don't see actual issues > (as per above). > > > I'm not even sure apm(8) should bother reporting some sort of battery > > status. > apm* showing battery information seems only naturally to me since that > is the driver/daemon/tool one issues power commands with, often based > on such information - think of `apmd -z 15': given that I want my box > to safely suspend around 15% remaining battery, it seems only > consequential that apm* is the same tool I query such information with. apmd(8) and apm(8) are not the same tool though. Having a deamon to run scripts on certain power-state changes makes some sense, but I'd argue that sensorsd(8) could be a reasonable place to implement this as well. The need to run apmd(8) in order to suspend/hibernate is a bit of an historical wart. And we already removed powermanagement state support from apmd(8). These days, users should probably just use /etc/sysctl.conf to set the powermanagement state instead of starting apmd(8) with the appropriate -A, -H or -L option. > > But if it does, my suggestion would be to make it use the > > sensors framework. That would need some work though such that drivers > > can mark sensors as providing battery information. Maybe add a > > SENSOR_FBATTERY flag? > You mean apm(8) directly using sensor_find(9) or so to look for > suitable sensors instead of relaying values throuh apm(4)? That's a possibility. But having apm(4) do that might be a useful middle ground as it would mean you don't have to change the userland tools just yet. My main concern with your diff is having to add APM hooks in all the drivers... > It sounds simpler in principal but that's just my naive guess; > I'll take a curious look into whether/how this could work.
Re: arm64: make cwfg(4) report battery information to apm(4)
On Sun, Mar 21, 2021 at 02:02:00PM +0100, Mark Kettenis wrote: > > Date: Sun, 21 Mar 2021 01:02:53 +0100 > > From: Klemens Nanni > > > > apm(4/arm64) merely provides an all zero/unknown stub for those values, > > e.g. apm(8) output is useless. > > > > Hardware sensors however provide the information: > > > > $ sysctl hw.sensors > > hw.sensors.rktemp0.temp0=32.22 degC (CPU) > > hw.sensors.rktemp0.temp1=33.89 degC (GPU) > > hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage) > > hw.sensors.cwfg0.raw0=259 (battery remaining minutes) > > hw.sensors.cwfg0.percent0=58.00% (battery percent) > > > > Diff below simply copies them over using apm_setinfohook() > > (I've looked at sys/arch/loongson/dev/kb3310.c which does the same): > > > > $ apm > > Battery state: high, 58% remaining, 259 minutes life estimate > > A/C adapter state: not known > > Performance adjustment mode: auto (408 MHz) > > > > Feedback? OK? > > This doesn't scale. What exactly does not scale? The way how various drivers hook into apm(4)? I'm not familiar enough (yet) with the framework(s) to judge their overall design and/or interop strategy off hand. > The whole apm(4) interface is too closely tied to the original APM > BIOS model. Even with acpi(4) it is a bit of a square peg for a round > hole, for example on machines with more than one battery. Right, there is hardware which doesn't fit this model and our apm* is not exactly up to speed with those, but I still don't see actual issues (as per above). > I'm not even sure apm(8) should bother reporting some sort of battery > status. apm* showing battery information seems only naturally to me since that is the driver/daemon/tool one issues power commands with, often based on such information - think of `apmd -z 15': given that I want my box to safely suspend around 15% remaining battery, it seems only consequential that apm* is the same tool I query such information with. > But if it does, my suggestion would be to make it use the > sensors framework. That would need some work though such that drivers > can mark sensors as providing battery information. Maybe add a > SENSOR_FBATTERY flag? You mean apm(8) directly using sensor_find(9) or so to look for suitable sensors instead of relaying values throuh apm(4)? It sounds simpler in principal but that's just my naive guess; I'll take a curious look into whether/how this could work.
Re: arm64: make cwfg(4) report battery information to apm(4)
> Date: Sun, 21 Mar 2021 01:02:53 +0100
> From: Klemens Nanni
>
> apm(4/arm64) merely provides an all zero/unknown stub for those values,
> e.g. apm(8) output is useless.
>
> Hardware sensors however provide the information:
>
> $ sysctl hw.sensors
> hw.sensors.rktemp0.temp0=32.22 degC (CPU)
> hw.sensors.rktemp0.temp1=33.89 degC (GPU)
> hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
> hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
> hw.sensors.cwfg0.percent0=58.00% (battery percent)
>
> Diff below simply copies them over using apm_setinfohook()
> (I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
>
> $ apm
> Battery state: high, 58% remaining, 259 minutes life estimate
> A/C adapter state: not known
> Performance adjustment mode: auto (408 MHz)
>
> Feedback? OK?
This doesn't scale.
The whole apm(4) interface is too closely tied to the original APM
BIOS model. Even with acpi(4) it is a bit of a square peg for a round
hole, for example on machines with more than one battery.
I'm not even sure apm(8) should bother reporting some sort of battery
status. But if it does, my suggestion would be to make it use the
sensors framework. That would need some work though such that drivers
can mark sensors as providing battery information. Maybe add a
SENSOR_FBATTERY flag?
> Index: dev/fdt/cwfg.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 cwfg.c
> --- dev/fdt/cwfg.c10 Jun 2020 17:51:21 - 1.1
> +++ dev/fdt/cwfg.c20 Mar 2021 23:43:13 -
> @@ -32,12 +32,15 @@
> #include
> #include
>
> +#include
> #include
>
> #include
>
> #include
>
> +#include "apm.h"
> +
> #define VERSION_REG 0x00
> #define VCELL_HI_REG0x02
> #define VCELL_HI_MASK 0x3f
> @@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
> NULL, "cwfg", DV_DULL
> };
>
> +int cwfg_apminfo(struct apm_power_info *info);
> +#if NAPM > 0
> +struct apm_power_info cwfg_power;
> +
> +int
> +cwfg_apminfo(struct apm_power_info *info)
> +{
> + bcopy(&cwfg_power, info, sizeof(*info));
> + return 0;
> +}
> +#endif
> +
> int
> cwfg_match(struct device *parent, void *match, void *aux)
> {
> @@ -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;
> }
> @@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
> sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
> sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
> }
> +
> +#if NAPM > 0
> + cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
> + APM_BATT_HIGH : APM_BATT_LOW;
> + cwfg_power.ac_state = APM_AC_UNKNOWN;
> + cwfg_power.battery_life = bat_percent;
> + cwfg_power.minutes_left = sc->sc_sensor[CWFG_SENSOR_RTT].value;
> +#endif
>
> done:
> cwfg_unlock(sc);
>
>
arm64: make cwfg(4) report battery information to apm(4)
apm(4/arm64) merely provides an all zero/unknown stub for those values,
e.g. apm(8) output is useless.
Hardware sensors however provide the information:
$ sysctl hw.sensors
hw.sensors.rktemp0.temp0=32.22 degC (CPU)
hw.sensors.rktemp0.temp1=33.89 degC (GPU)
hw.sensors.cwfg0.volt0=3.76 VDC (battery voltage)
hw.sensors.cwfg0.raw0=259 (battery remaining minutes)
hw.sensors.cwfg0.percent0=58.00% (battery percent)
Diff below simply copies them over using apm_setinfohook()
(I've looked at sys/arch/loongson/dev/kb3310.c which does the same):
$ apm
Battery state: high, 58% remaining, 259 minutes life estimate
A/C adapter state: not known
Performance adjustment mode: auto (408 MHz)
Feedback? OK?
Index: dev/fdt/cwfg.c
===
RCS file: /cvs/src/sys/dev/fdt/cwfg.c,v
retrieving revision 1.1
diff -u -p -r1.1 cwfg.c
--- dev/fdt/cwfg.c 10 Jun 2020 17:51:21 - 1.1
+++ dev/fdt/cwfg.c 20 Mar 2021 23:43:13 -
@@ -32,12 +32,15 @@
#include
#include
+#include
#include
#include
#include
+#include "apm.h"
+
#defineVERSION_REG 0x00
#defineVCELL_HI_REG0x02
#define VCELL_HI_MASK 0x3f
@@ -119,6 +122,18 @@ struct cfdriver cwfg_cd = {
NULL, "cwfg", DV_DULL
};
+int cwfg_apminfo(struct apm_power_info *info);
+#if NAPM > 0
+struct apm_power_info cwfg_power;
+
+int
+cwfg_apminfo(struct apm_power_info *info)
+{
+ bcopy(&cwfg_power, info, sizeof(*info));
+ return 0;
+}
+#endif
+
int
cwfg_match(struct device *parent, void *match, void *aux)
{
@@ -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;
}
@@ -363,6 +386,14 @@ cwfg_update_sensors(void *arg)
sc->sc_sensor[CWFG_SENSOR_RTT].value = rtt;
sc->sc_sensor[CWFG_SENSOR_RTT].flags &= ~SENSOR_FINVALID;
}
+
+#if NAPM > 0
+ cwfg_power.battery_state = bat_percent > sc->sc_alert_level ?
+ APM_BATT_HIGH : APM_BATT_LOW;
+ cwfg_power.ac_state = APM_AC_UNKNOWN;
+ cwfg_power.battery_life = bat_percent;
+ cwfg_power.minutes_left = sc->sc_sensor[CWFG_SENSOR_RTT].value;
+#endif
done:
cwfg_unlock(sc);
