[Sugar-devel] [PATCH sugar] Control Panel: making about my computer section hardware independent, OLPC #11232

[Sascha Silbe]

Excerpts from Simon Schampijer's message of 2011-10-07 11:13:15 +0200:
> '/ofw' is not used anymore on the XO, '/proc/device-tree' is used now
> instead. Instead of just adjusting for that change we took the
> chance to make the section hardware independent. As the firmware
> version we display the bios version if available on non XO
> hardware. As ethtool has become a depedency of Sugar we can now
                                    ^ dependency

> display the wireless firmware as well on all hardwares.

There's no plural form of hardware. Maybe "any hardware"?
Not that important, though.

> The serial number is often only readable by root on non-XO
> hardware. If the serial number can not be found on an XO we hide
> that part of the section.
> 
> The patch has been tested on XO 1, 1.5, 1.75 and on a Thinkpad T61.

I like the general approach. Thanks for considering (and even testing
on) non-XO hardware!


[extensions/cpsection/aboutcomputer/model.py]
> @@ -17,7 +17,6 @@
>  
>  import os
>  import logging
> -import re
>  import subprocess
>  from gettext import gettext as _
>  import errno
> @@ -35,6 +34,7 @@ _NM_DEVICE_TYPE_WIFI = 2
>  
>  _OFW_TREE = '/ofw'
>  _PROC_TREE = '/proc/device-tree'
> +_SYS_TREE = '/sys/class/dmi/id'

Both /ofw and /proc/device/tree are mount points for the OFW device
tree. /sys/class/dmi/id OTOH contains different entries. So
_DMI_DIRECTORY might be a better name.


> @@ -58,6 +58,8 @@ def get_serial_number():
>          serial_no = _read_file(os.path.join(_OFW_TREE, _SN))
>      elif os.path.exists(os.path.join(_PROC_TREE, _SN)):
>          serial_no = _read_file(os.path.join(_PROC_TREE, _SN))
> +    else:
> +        return None
>      if serial_no is None:
>          serial_no = _not_available
>      return serial_no

We are hiding the serial number completely if the kernel doesn't expose
device tree information, but show "Not available" if the file couldn't
be read or parsed. That doesn't really match the patch description.

How about:

def get_serial_number():
    if os.path.exists(os.path.join(_OFW_TREE, _SN)):
        return _read_file(os.path.join(_OFW_TREE, _SN))
    elif os.path.exists(os.path.join(_PROC_TREE, _SN)):
        return _read_file(os.path.join(_PROC_TREE, _SN))
    elif os.path.exists(os.path.join(_DMI_DIRECTORY, 'board_serial'):
        return _read_file(os.path.join(_DMI_DIRECTORY, 'board_serial'))
    return None



> @@ -100,14 +102,14 @@ def get_firmware_number():
>      firmware_no = None
>      if os.path.exists(os.path.join(_OFW_TREE, _MODEL)):
>          firmware_no = _read_file(os.path.join(_OFW_TREE, _MODEL))
> +        firmware_no = firmware_no[6:13]
>      elif os.path.exists(os.path.join(_PROC_TREE, _MODEL)):
>          firmware_no = _read_file(os.path.join(_PROC_TREE, _MODEL))
> +        firmware_no = firmware_no[6:13]
> +    elif os.path.exists(os.path.join(_SYS_TREE, 'bios_version')):
> +        firmware_no = _read_file(os.path.join(_SYS_TREE, 'bios_version'))

The above assumes OLPC format (e.g. "CL2   Q4B11  Q4B") for
.../openprom/model and will break on other machines with device tree
(e.g. PPC Macs). The old code at least only did the cutting if there
actually were three space-separated parts:

>      if firmware_no is None:
>          firmware_no = _not_available
> -    else:
> -        firmware_no = re.split(' +', firmware_no)
> -        if len(firmware_no) == 3:
> -            firmware_no = firmware_no[1]
>      return firmware_no


[extensions/cpsection/aboutcomputer/view.py]
> @@ -127,37 +128,36 @@ class AboutComputer(SectionView):
>          box_software.pack_start(box_sugar, expand=False)
>          box_sugar.show()
>  
> -        if os.path.exists('/ofw'):

I wonder if we should hide the Firmware version if it's unavailable,
like we now do for the serial number. Is it useful to know that Sugar
would display it, but can't access it? Could it confuse users if it
doesn't get mentioned at all?

Sascha

-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20111008/ab5baa9a/attachment.pgp>