[Debian-eeepc-devel] Changes for eeepc-acpi-scripts

[Nico Golde]

Hi jcsid,
* jcsid <jc.sid at free.fr> [2008-06-12 12:01]:
> So, here are my changes to eeepc-acpi-scripts (with diff)
> What changed:
[...] 
> --- Desktop/eeepc-acpi-scripts/eeepc-acpi-scripts/actions/hotkey.sh	2008-04-25 14:29:09.000000000 +0200
> +++ /etc/acpi/actions/hotkey.sh	2008-06-12 10:26:23.000000000 +0200
> @@ -10,9 +10,8 @@
>      echo "$@"  # for /var/log/acpid
>      if [ -S /tmp/.X11-unix/X0 ]; then
>  	export DISPLAY=:0
> -	user=$(who | sed -n '/ (:0[\.0]*)$\| :0 /{s/ .*//p;q}')
> -        home=$(getent passwd $user | cut -d: -f6)
> -	XAUTHORITY=$home/.Xauthority
> +	user=$(who | sed -n '/ (:0[\.0].*)$\| :0 /{s/ .*//p;q}')

This change doesn't look correct to me. The brackets around 
[\.0] are useless now and I am not sure if we can rely on 
everyone having a display of :0.0 and not :0 in the output, 
both is correct.

> +	XAUTHORITY=/home/$user/.Xauthority

Not correct, this relies on everyone having is $HOME in 
/home which is not always the case, thus the getent usage.

>  	[ -f $XAUTHORITY ] && export XAUTHORITY
>  
>      if [ "x$ENABLE_OSD" = "xno" ]; then
> @@ -20,16 +19,13 @@
>      fi
>  
>  	killall -q aosd_cat
> -	if [ -n "$2" -a -z "$(echo $2 | sed 's/[0-9]//g')" ]; then
> -		echo "$@%" | aosd_cat -f 0 -u 100 -o 0 -n "$OSD_FONT" &
> -	else
> -		echo "$@" | aosd_cat -n "$OSD_FONT" -f 100 -u 1000 -o 100 &
> -	fi
> +		echo "$@" | aosd_cat -n "$OSD_FONT" -f 80 -u 1000 -o 80 -e 0 -b 155 -B $OSD_BG_COLOR -R $OSD_COLOR -x $OSD_X -y $OSD_Y  &

This change is also broken, the purpose of the if here is to 
have osd messages without some numeric value (like Audio 
off) displayed longer than the Volume level as these outputs 
do not have the problem of changing quite fast.

>      else
>  	echo "$@" > /dev/console
>      fi
>  }
>  
> +
>  show_wireless() {
>      if grep -q ath0 /proc/net/wireless; then
>  	status=On
> @@ -46,16 +42,29 @@
>  
>  show_volume() {
>      percent=$(amixer get $VOLUME_LABEL | sed -n '/%/{s/.*\[\(.*\)%\].*/\1/p;q}')
> -    notify Volume $percent
> +    notify Volume $percent%

The % is handled by notify but you removed this in the above 
change.

> +}
> +
> +show_brightness() {
> +    # final digit of ACPI code is brightness level in hex
> +    level=$(cat /proc/acpi/asus/brn)
> +    # convert hex digit to percent
> +    percent=$(((100 * $level + 8) / 15))
> +    notify Brightness $percent%
>  }

We decided to not use something like this in the past 
because having if you look at the brightness on your screen 
this is way more meaningful than some percentage value.
That's why it was commented out.

> +show_vga() {
> +	notify ""
> +	status=$(xrandr -q | grep VGA | cut -d ' ' -f 1,2,3 )
> +	notify ${status%(*}
> +}

Should be the same, look at your screen which you would have 
to do anyway for seeing an osd status. Note that the notidy 
was used before just to get the DISPLAY which is needed for 
xrandr.

> +show_batt() {
> +	status=$(cat /proc/acpi/battery/BAT0/state | grep 'charging')
> +	percent=$(cat /proc/acpi/battery/BAT0/state | grep 'remaining capacity')
> +	percent=${percent%*mAh}
> +	notify Batt.${status#*:}: ${percent#*:}%
> +}

This is broken, the battery paths are not hardcoded in acpi 
and besides that /proc/acpi is not available in 2.6.25 
anymore.

>  case $code in
>      # Fn+F2 -- toggle wireless
> @@ -77,24 +86,41 @@
>  	;;
>      # Fn+F8 -- decrease volume
>      00000014)
> -	amixer -q set $VOLUME_LABEL 2- unmute
> +	amixer -q set $VOLUME_LABEL 4- unmute

At least for me steps by 2- are ok, what do others think?

[...] 
> +    # Fn+F6 -- launch $AP_APP
> +    00000012)
> +	notify $AP_MSG
> +	$AP_APP
> +	;;

Also useless OSD output, But having fn-f6 work might be 
nice.

> --- Desktop/eeepc-acpi-scripts/eeepc-acpi-scripts/actions/suspend.sh	2008-04-25 14:29:09.000000000 +0200
> +++ /etc/acpi/actions/suspend.sh	2008-06-10 16:58:52.000000000 +0200
> @@ -10,5 +10,13 @@
>  brn_control=/proc/acpi/asus/brn
>  
>  brightness=$(cat $brn_control)
> +wireless=$(cat /proc/acpi/asus/wlan)
> +/etc/acpi/actions/wireless.sh off

I am opposed to that, if you need to switch off wireless 
because of suspend that's a bug in the driver and should be 
fixed there.

> --- Desktop/eeepc-acpi-scripts/eeepc-acpi-scripts/actions/wireless.sh	2008-04-25 14:29:09.000000000 +0200
> +++ /etc/acpi/actions/wireless.sh	2008-06-12 10:33:13.000000000 +0200
> @@ -1,25 +1,23 @@
>  #!/bin/sh
>  
>  wlan_control=/proc/acpi/asus/wlan
> -
>  case $1 in
> -    on|enable)
> -	if [ $(cat $wlan_control) = 0 ]; then
> -	    modprobe -r pciehp
> -	    modprobe pciehp pciehp_force=1
> -	    echo 1 > $wlan_control
> -	    modprobe ath_pci
> -	    if ! ifconfig ath0 up; then exec $0 off; fi
> -	fi
> -	;;
> -    off|disable)
> -	if [ $(cat $wlan_control) = 1 ]; then
> -	    ifdown --force ath0
> -	    modprobe -r ath_pci
> -	    echo 0 > $wlan_control
> -	fi
> +    on)
> +	echo 1 > $wlan_control
> +	modprobe ath_pci
> +	ifup ath0 &

Why did you remove the part that checks if the wireless is 
already on and does nothing if this is true?
You also removed loading pciehp without an obvious reason.

A bit of reasoning for your changes would be nice.

Cheers
Nico

-- 
Nico Golde - http://www.ngolde.de - nion at jabber.ccc.de - GPG: 0x73647CFF
For security reasons, all text in this mail is double-rot13 encrypted.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
Url : http://lists.alioth.debian.org/pipermail/debian-eeepc-devel/attachments/20080612/09cda65b/attachment.pgp