[Gnome-Power-Devel] Question re on battery vs on mains

[Richard Hughes]

On Tue, 2005-08-02 at 00:53 -0400, Andrew Duggan wrote:
> Hi,
> 
> 
> Richard Hughes wrote:
> > On Sun, 2005-07-31 at 00:35 -0400, Andrew Duggan wrote:
> >>However, hal-get-property --udi /org/freedesktop/Hal/devices/acpi_ACAD 
> >>--key ac_adapter.present
> >>returns "false"
> > Hmm.
> > 
> > Sounds like our friend ACPI playing tricks....
> > 
> Yes, as it turns out.
> > 
> > Could you please send me the output of:
> > 
> > lshal --monitor
> > 
> > when your battery is about 98%, charging towards 100% (when the
> > ac_adapter disappears)
> In addition to doing an lshal --monitor I also did an acpi_listen and a hal 
> --daemon=no --verbose=yes
> 
> After looking at that output myself,  it turns out my laptop does not emit 
> any ac_adapter acpi events at all. :-(
> 
> When I initially started looking at why the gpm icon was not tracking my AC 
> adapter status and started messing with hal-set-property, I must have left 
> it (ac_adapter.present) false.  Then your logic in gpm compensates and does 
> the best it can considering that it gets battery but no ac_adapter events.
> At some point I must have forgotten I left it in that state (false) (of 
> course, I was expecting that hal would reset to to "actual" when it 
> detected at state change, but since my acpi doesn't do that the property 
> never changed unless I manually did it.) - I don't restart my laptop a lot, 
> it mostly gets hibernated and resumed.

It should have rescanned it every 30 seconds anyway (as an acpi
fallback) - ahh, just spotted an error in HAL.

HAL only does the fallback reading for battery.type = primary.

This needs to be fixed.

I'll CC in hal-devel.

> So at least for my purposes, I hacked a patch to hal addon-acpi.c that 
> forces a rescan of ac_adapter on a battery event.

Probably shouldn't patch the addon, rather than the file acpi.c - the
addon just relays the event to the rescan method on HAL.
Adding one file lookup to the fast-path of battery updates should be
fine.

> I had to add the sleep (3) to overcome the race condition that happens when 
> the code I added tried to find the udi for the ac_adapter capability before 
> the main hal daemon had built the data-structure.

Probably fixed by adding to acpi.c file rather than the addon.

>   At least I didn't 
> hard-code the udi and call libhal_find_device_by_capability () instead. I 
> will probably reverse the if logic I added, since I clobbered it before I 
> say the race condition.
> 
> I'm attaching the patch so you can see what I did to fix it, but I doubt 
> anyone but me will find it of any use -- I'm sure I broke all the 
> coding-style rules anyway.  :-\

I'll split the problem up into logical sized chunks and send to
hal-devel.

> It would be nice if there was a way to query to determine if ac_adapter 
> events were possible, but at this point that is way beyond me.

I don't believe so, but the extra battery fast-path lookup should be
fine.

> I should have jumped to this conclusion right away, since I saw that when 
> the power was unplugged, it was not until I manually did a cat 
> /proc/acpi/ac_adapter/ACAD/state that the backlight of the panel went to 
> 50%.  Hal's grep of that should have caused that to happen, but since there 
> were no ac_adapter events, it was never triggered.  Oh, well live and 
> learn.   Reason 5972490 why everybody hates (or at least I hate) ACPI.

But we can abstract away the problems to the userspace programs -
hopefully.

> Sorry for the bother.

No! Thanks for the bug-finding.

I'll send you a patch later today for you to try.

Richard.

> 
> > 
> > Thanks.
> > 
> > Richard.
> > 
> plain text document attachment (hal-0.5.3-scanACAD.patch)
> --- hald-orig/linux2/addons/addon-acpi.c	2005-05-16 13:42:05.000000000 -0400
> +++ hald/linux2/addons/addon-acpi.c	2005-08-02 00:04:24.000000000 -0400
> @@ -99,6 +99,10 @@ main (int argc, char *argv[])
>  	char acpi_name[256];
>  	unsigned int acpi_num1;
>  	unsigned int acpi_num2;
> +	char acpi_ACAD[256];
> +	unsigned int scan_ACAD;
> +	char **udis;
> +	int num_udis;
>  
>  	fd = -1;
>  
> @@ -128,6 +132,29 @@ main (int argc, char *argv[])
>  			goto out;
>  		}
>  	}
> +
> +	/* Sleep for 2 seconds to see if we can pass by the race */
> +
> +	sleep (3);
> +
> +	/* find the udi of acpi_ACAD so we can rescan it on battery updates */
> +
> +	scan_ACAD = 0;
> +	udis = libhal_find_device_by_capability (ctx, "ac_adapter", &num_udis, &error);
> +	if (dbus_error_is_set (&error)) 
> +		dbg ("Could find capabiltiy ac_adapter: %s %s", error.name, error.message);
> +	else
> +		if (num_udis > 0) {
> +			/* only use the 1st one anyway */
> +			memset(&acpi_ACAD, 0, sizeof(acpi_ACAD));
> +			snprintf (acpi_ACAD, sizeof (acpi_ACAD), "%s", udis[0]);
> +			scan_ACAD = 1;
> +			libhal_free_string_array (udis);
> +			dbg ("Found ACAD udi %s ", acpi_ACAD);
> +		}
> +		else
> +			dbg ("Looking for ac_adapter returned no error but with num_udis = 0");
> +
>  		
>  	/* main loop */
>  	while (1) {
> @@ -167,6 +194,11 @@ main (int argc, char *argv[])
>  				dbg ("battery event");
>  				dbus_error_init (&error);
>  				libhal_device_rescan (ctx, udi, &error);
> +				if (scan_ACAD) {
> +					dbg ("scan ACAD on battery");
> +					dbus_error_init (&error);
> +					libhal_device_rescan (ctx, acpi_ACAD, &error);
> +				}
>  			}
>  
>  		} else {

_______________________________________________
hal mailing list
hal at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/hal