[patch] complete get and set lcd patch

[David Zeuthen]

On Thu, 2005-09-01 at 01:15 +0100, Richard Hughes wrote:
> > Both of these need to check that the brightness file is actually there
> > and throw an exception otherwise (the .Unsupported exception is fine)
> 
> Sure, and I've checked writable too, as some ppl might not be running
> HAL as root. Shell script is great.

No, this
         
        checkpath() {
        	vendor=$1
        	file=$2
        	procfs="/proc"
        	procpath="${procfs}/acpi/${vendor}/${file}"
        	if [ ! -e ${procpath} ]; then
        		notpresent ${procpath}
        	elif [ ! -r ${procpath} ]; then
        		notreadable ${procpath}
        	fi
        	echo ${procpath}
        }

looks wrong. Also

+unsupported() {
+	echo "org.freedesktop.Hal.Device.LaptopPanel.NotSupported" >&2
+	echo "No acpi method script found" >&2
+	exit 1
+}
+
+notreadable() {
+	echo "org.freedesktop.Hal.Device.LaptopPanel.NotReadable" >&2
+	echo "The proc device file '$1' is not readable" >&2
+	exit 1
+}
+
+notpresent() {
+	echo "org.freedesktop.Hal.Device.LaptopPanel.NotPresent" >&2
+	echo "The proc device file '$1' is not presest" >&2
+	exit 1
+}

I don't think the caller is interested in whether it's notpresent() or
whether it's notreadable(). Called is only interested if this succeeds
or not (it's the "keep in mind who you are designing the API for" bit).

Another thing.. your patch seems wrong insofar that it sets 

 linux.acpi_path

incorrect for the laptop panel hal device object. On my IBM Thinkpad T41
it gives '/proc/acpi/ibm' when it should be '/proc/acpi/ibm/brightness'.

Second, we need to make the scripts as small and compact as possible
(for both performance reasons and the reasons we want it to be easily
reviewable). It's pretty crack IMHO to check whether it's writeable /
readable. Just check if it's there and bail out if it's not.

Other comments

 - your patch was missing the file
    fdi/policy/10osvendor/10-laptop-panel-mgmt-policy.fdi

 - all this didn't worked out of the box for me; I got this

                14:03:56.589 [I] hald_dbus.c:2895: OK for method 'SetBrightness' with signature 'i' on interface 'org.freedesktop.Hal.Device.LaptopPanel' for UDI '/org/freedesktop/Hal/devices/acpi_ibm' and execpath 'hal-system-lcd-set-brightness'
                14:03:56.602 [I] util.c:554: child exited for pid 4013
                14:03:56.602 [I] hald_dbus.c:2529: failed with '../tools/hal-system-lcd-set-brightness: line 44: let: value = 0 / ( 100 /  ): syntax error: operand expected (error token is ")")' 'Adjusting brightness of IBM LCD Panel to 0% ( / )'
                3624: arguments to dbus_message_new_error() were incorrect, assertion "_dbus_check_is_valid_error_name (error_name)" failed in file dbus-message.c line 925.
                This is normally a bug in some application using the D-BUS library.
                *** [DIE] hald_dbus.c:hald_exec_method_cb():2535 : No memory

   which made hald crash. I committed a fix for that to CVS, btw.

+if [ "$HAL_PROP_LAPTOPPANEL_ACPI_METHOD" == "toshiba" ]; then

Here, and in other places.. you need to s/LAPTOPPANEL/LAPTOP_PANEL/.

+elif [ "$HAL_PROP_LAPTOPPANEL_ACPI_METHOD" == "ibm" ]; then
+	# cat /proc/acpi/ibm/brightness
+	#  level:          5
+	#  commands:       up, down
+	#  commands:       level <level> (<level> is 0-7)
+	procpath=`checkpath "ibm" "brightness"`
+	value="`cat ${procpath} | grep level | awk '{print $2;}'`"

This is obviously wrong; you need to "grep 'level:'" instead of just
"grep level".

+let "percentage = ${value} * ( 100 / ${HAL_PROP_LAPTOPPANEL_NUM_LEVELS} )"

This is wrong if you keep in mind we are doing integer maths. Do this
instead

let "percentage =  ( ( $value + 1 ) *  100 ) / $HAL_PROP_LAPTOP_PANEL_NUM_LEVELS"

+RET=$?

Not needed.

+#Print debug text to console
+echo -n "Brightness of $HAL_PROP_INFO_PRODUCT = ${percentage}%" >&2
+echo " ($value / $HAL_PROP_LAPTOPPANEL_NUM_LEVELS)" >&2

Not in production code please. It's okay to keep it in comments and
developers can uncomment as appropriate.

+#Work out the percentage
+let "value = ${percentage} / ( 100 / $HAL_PROP_LAPTOPPANEL_NUM_LEVELS )"

This is wrong. Suggest this

let "value = ( $percentage * $HAL_PROP_LAPTOP_PANEL_NUM_LEVELS ) / 100"

instead.

> > So, if you can change this and send the patch again I think we're
> > getting there!
> 
> New patch attached. If we can commit soon, I'll promise to patch any
> remaining issues against CVS :-)

If you can test the next version of the patch a bit more carefully it
would be good. Thanks.

Cheers,
David