Re: problems with sysmon power

[Matthias Drochner <M.Drochner%fz-juelich.de@localhost>]

joerg%britannica.bec.de@localhost said:
> The firmware will shutdown the system forcefully at a point before the
> deep draining happens.

I was assuming that. But to trigger the bugs I'd still need
a lot of load/unload cycles at a low charge level, which
wouldn't help battery lifetime either. Letalone the effect
of forced powerdowns on the file system.

> What does envstat report for the battery with and without AC adapter?
> Note that it can take a few seconds until it switches between both modi.

envstat reports the correct state, and does react rather quickly.
But a printf in sme_acadapter_check() shows that the "value_cur"
inside the kernel is not updated until either the state changes
(if I unplug/replug the power supply), or "envstat" is invoked. So
if I boot with AC plugged on and don't touch it, the fact is simply
not recognized by the event handling code.
Some sme_update_dictionary() improve it for me, but this doesn't
look like a correct fix. (There is code to refresh in
sme_events_worker() -- it just doesn't do it.)

The battery state check didn't work because it accesses uninitialized
variables of empty battery slots, and because the logics in
sme_battery_check() is generally flawed. I'll append a patch
which makes it work. (I wonder whether the ENVSYS_SVALID flag
should be checked everywhere, but it seems that it isn't maintained
consistently within the framework.)
Note that also the sme_acadapter_check() won't work correctly
if more than one adapter is configured - it does't even pretend to
test a second one. (And there is a slight abstraction violation -
use of the loop variable after TAILQ_FOREACH().)

> The charge state is about the capacity range the battery is currently in
> [...]

I'm not going to discuss about this - this doesn't lead to anything.
I just want to state that the use of these terms is inconsistent
to at least APM and ACPI code, and that observables and units
of measurement are wildly mixed.

best regards
Matthias





-------------------------------------------------------------------
-------------------------------------------------------------------
Forschungszentrum Juelich GmbH
52425 Juelich

Sitz der Gesellschaft: Juelich
Eingetragen im Handelsregister des Amtsgerichts Dueren Nr. HR B 3498
Vorsitzende des Aufsichtsrats: MinDir'in Baerbel Brumme-Bothe
Geschaeftsfuehrung: Prof. Dr. Achim Bachem (Vorsitzender),
Dr. Ulrich Krafft (stellv. Vorsitzender), Prof. Dr. Harald Bolt,
Dr. Sebastian M. Schmidt
-------------------------------------------------------------------
-------------------------------------------------------------------

#
# old_revision [e18e83c5edb47018fd144b1edb5375a6a7d1031f]
#
# patch "sys/dev/sysmon/sysmon_envsys_events.c"
#  from [bf90368a99608d282f1892904f022d13840dc0d9]
#    to [9f8d83ea154b82c7dd305800244f9670fb6aa099]
#
============================================================
--- sys/dev/sysmon/sysmon_envsys_events.c       
bf90368a99608d282f1892904f022d13840dc0d9
+++ sys/dev/sysmon/sysmon_envsys_events.c       
9f8d83ea154b82c7dd305800244f9670fb6aa099
@@ -652,6 +652,8 @@ sme_acadapter_check(void)
         */
        if (!sme)
                return false;
+
+       sme_update_dictionary(sme);
        /*
         * Check if there's an AC adapter device connected.
         */
@@ -673,12 +675,11 @@ sme_battery_check(void)
 {
        struct sysmon_envsys *sme;
        envsys_data_t *edata;
-       bool battery, batterycap, batterycharge;
+       int batteriesfound = 0;
+       bool present, batterycap, batterycharge;
 
        KASSERT(mutex_owned(&sme_mtx));
 
-       battery = batterycap = batterycharge = false;
-
        /*
         * Check for battery devices and its state.
         */
@@ -686,10 +687,23 @@ sme_battery_check(void)
                if (sme->sme_class != SME_CLASS_BATTERY)
                        continue;
 
+               sme_update_dictionary(sme);
+
+               present = true;
+               TAILQ_FOREACH(edata, &sme->sme_sensors_list, sensors_head) {
+                       if (edata->units == ENVSYS_INDICATOR &&
+                           !edata->value_cur) {
+                               present = false;
+                               break;
+                       }
+               }
+               if (!present)
+                       continue;
                /*
                 * We've found a battery device...
                 */
-               battery = true;
+               batteriesfound++;
+               batterycap = batterycharge = false;
                TAILQ_FOREACH(edata, &sme->sme_sensors_list, sensors_head) {
                        if (edata->units == ENVSYS_BATTERY_CAPACITY) {
                                batterycap = true;
@@ -701,8 +715,10 @@ sme_battery_check(void)
                                        return false;
                        }
                }
+               if (!batterycap || !batterycharge)
+                       return false;
        }
-       if (!battery || !batterycap || !batterycharge)
+       if (!batteriesfound)
                return false;
 
        /*