[patch] complete get and set lcd patch

[Richard Hughes]

On Thu, 2005-09-01 at 15:08 -0400, David Zeuthen wrote:
> 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).

Sure, I've made the checks a bit simpler.

> 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'.

But that is the acpi_path.... ohh i see what you mean, pass down the
procfs path to the script to make it even easier. I've changed that too.

> 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.

Sure.

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

OOps. fixed.

>  - 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/.

Okay, fixed.

> +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".

Ahh yes, fixed.

> +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"

Good plan, fixed, thanks.

> +RET=$?
> 
> Not needed.

Removed.

> +#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.

Removed, if you wanted to debug, it would be trivial to echo out a debug
value.

> +#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.

Sure, I'm making assumptions about floating point :-) Changed.

> > > 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.

Yes, sorry. Late night hacking can have this effect.

Newest patch attached. Tell me what you think.

Richard.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: hal-laptop_panel.diff
Type: text/x-patch
Size: 18670 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/hal/attachments/20050901/5aee1648/hal-laptop_panel-0001.bin