[Sugar-devel] [PATCH sugar v2] battery frame device: replace HAL with UPower

Simon Schampijer simon at schampijer.de
Fri Feb 18 12:32:51 EST 2011


Hi Sascha,

thanks for that patch.

On 01/21/2011 10:35 AM, Sascha Silbe wrote:
> HAL is deprecated, UPower is the designated replacement w.r.t. power
> supplies. As a bonus the time displayed is now correct (calculated at
> run-time by UPower, self-adjusting to changes in power consumption).

Ok, I was first put off by the new behavior but I guess it is a good 
thing in the end. We should maybe note it in the release notes as a Feature.

> Tested on XO-1.5 running Debian Squeeze with stock UPower (0.9.5-5) and a
> 2.6.35 OLPC kernel.

I have tested the patch on the latest 11.2.0 build as working fine.

The overall patch looks great - a few smaller comments inline.

> Signed-off-by: Sascha Silbe<sascha-pgp at silbe.org>
> ---
> v1->v2: Rebased on mainline/master
>
>   extensions/deviceicon/battery.py |  194 +++++++++++++++++++-------------------
>   1 files changed, 98 insertions(+), 96 deletions(-)
>
> diff --git a/extensions/deviceicon/battery.py b/extensions/deviceicon/battery.py
> index 4028878..0d7381e 100644
> --- a/extensions/deviceicon/battery.py
> +++ b/extensions/deviceicon/battery.py
> @@ -16,8 +16,9 @@
>
>   import logging
>   from gettext import gettext as _
> -import gconf
> +import sys
>
> +import gconf
>   import gobject
>   import gtk
>   import dbus
> @@ -38,17 +39,24 @@ _STATUS_DISCHARGING = 1
>   _STATUS_FULLY_CHARGED = 2
>   _STATUS_NOT_PRESENT = 3
>
> -_LEVEL_PROP = 'battery.charge_level.percentage'
> -_CHARGING_PROP = 'battery.rechargeable.is_charging'
> -_DISCHARGING_PROP = 'battery.rechargeable.is_discharging'
> -_PRESENT_PROP = 'battery.present'
> +_UP_TYPE_BATTERY = 2
> +
> +_UP_STATE_UNKNOWN = 0
> +_UP_STATE_CHARGING = 1
> +_UP_STATE_DISCHARGING = 2
> +_UP_STATE_EMPTY = 3
> +_UP_STATE_FULL = 4
> +_UP_STATE_CHARGE_PENDING = 5
> +_UP_STATE_DISCHARGE_PENDING = 6
> +
> +_WARN_MIN_PERCENTAGE = 15
>
>
>   class DeviceView(TrayIcon):
>
>       FRAME_POSITION_RELATIVE = 102
>
> -    def __init__(self, udi):
> +    def __init__(self, battery):
>           client = gconf.client_get_default()
>           self._color = XoColor(client.get_string('/desktop/sugar/user/color'))
>
> @@ -56,17 +64,10 @@ class DeviceView(TrayIcon):
>
>           self.set_palette_invoker(FrameWidgetInvoker(self))
>
> -        self._model = DeviceModel(udi)
> +        self._model = DeviceModel(battery)
>           self.palette = BatteryPalette(_('My Battery'))
>           self.palette.set_group_id('frame')
> -
> -        self._model.connect('notify::level',
> -                            self._battery_status_changed_cb)
> -        self._model.connect('notify::charging',
> -                            self._battery_status_changed_cb)
> -        self._model.connect('notify::discharging',
> -                            self._battery_status_changed_cb)
> -        self._model.connect('notify::present',
> +        self._model.connect('updated',
>                               self._battery_status_changed_cb)

If we touch the code already, maybe make it private, two '__'.

>           self._update_info()
>
> @@ -88,7 +89,7 @@ class DeviceView(TrayIcon):
>                                             style.COLOR_WHITE.get_svg()))
>           elif self._model.props.discharging:
>               status = _STATUS_DISCHARGING
> -            if current_level<= 15:
> +            if current_level<= _WARN_MIN_PERCENTAGE:
>                   badge_name = 'emblem-warning'
>           else:
>               status = _STATUS_FULLY_CHARGED
> @@ -98,10 +99,10 @@ class DeviceView(TrayIcon):
>           self.icon.props.xo_color = xo_color
>           self.icon.props.badge_name = badge_name
>
> -        self.palette.set_level(current_level)
> -        self.palette.set_status(status)
> +        self.palette.set_info(current_level, self._model.props.time_remaining,
> +            status)
>
> -    def _battery_status_changed_cb(self, pspec, param):
> +    def _battery_status_changed_cb(self, *args):
>           self._update_info()
>
>
> @@ -109,8 +110,9 @@ class BatteryPalette(Palette):
>
>       def __init__(self, primary_text):
>           Palette.__init__(self, primary_text)
> -
>           self._level = 0
> +        self._time = 0
> +        self._status = _STATUS_NOT_PRESENT
>           self._progress_bar = gtk.ProgressBar()
>           self._progress_bar.set_size_request(
>               style.zoom(style.GRID_CELL_SIZE * 4), -1)
> @@ -126,29 +128,29 @@ class BatteryPalette(Palette):
>           self._progress_widget = vbox
>           self.set_content(self._progress_widget)
>
> -    def set_level(self, percent):
> +    def set_info(self, percent, seconds, status):
>           self._level = percent
> -        fraction = percent / 100.0
> -        self._progress_bar.set_fraction(fraction)
> +        self._time = seconds
> +        self._status = status
> +        self._progress_bar.set_fraction(percent / 100.0)
> +        self._update_secondary()
>
> -    def set_status(self, status):
> -        current_level = self._level
> +    def _update_secondary(self):
>           secondary_text = ''
> -        status_text = '%s%%' % current_level
> +        status_text = '%s%%' % self._level
>
>           progress_widget = self._progress_widget
> -        if status == _STATUS_NOT_PRESENT:
> +        if self._status == _STATUS_NOT_PRESENT:
>               secondary_text = _('Removed')
>               progress_widget = None
> -        elif status == _STATUS_CHARGING:
> +        elif self._status == _STATUS_CHARGING:
>               secondary_text = _('Charging')
> -        elif status == _STATUS_DISCHARGING:
> -            if current_level<= 15:
> +        elif self._status == _STATUS_DISCHARGING:
> +            if self._level<= _WARN_MIN_PERCENTAGE:
>                   secondary_text = _('Very little power remaining')
>               else:
> -                #TODO: make this less of an wild/educated guess
> -                minutes_remaining = int(current_level / 0.59)
> -                remaining_hourpart = minutes_remaining / 60
> +                minutes_remaining = self._time // 60
> +                remaining_hourpart = minutes_remaining // 60
>                   remaining_minpart = minutes_remaining % 60
>                   secondary_text = _('%(hour)d:%(min).2d remaining') % \
>                           {'hour': remaining_hourpart, 'min': remaining_minpart}
> @@ -163,91 +165,91 @@ class BatteryPalette(Palette):
>   class DeviceModel(gobject.GObject):
>       __gproperties__ = {
>           'level': (int, None, None, 0, 100, 0, gobject.PARAM_READABLE),
> +        'time-remaining': (int, None, None, 0, sys.maxint, 0,
> +                           gobject.PARAM_READABLE),  # unit: seconds
>           'charging': (bool, None, None, False, gobject.PARAM_READABLE),
>           'discharging': (bool, None, None, False, gobject.PARAM_READABLE),
>           'present': (bool, None, None, False, gobject.PARAM_READABLE),
>       }
>
> -    def __init__(self, udi):
> -        gobject.GObject.__init__(self)
> -
> -        bus = dbus.Bus(dbus.Bus.TYPE_SYSTEM)
> -        proxy = bus.get_object('org.freedesktop.Hal', udi,
> -                               follow_name_owner_changes=True)
> -        self._battery = dbus.Interface(proxy, 'org.freedesktop.Hal.Device')
> -        bus.add_signal_receiver(self._battery_changed,
> -                                'PropertyModified',
> -                                'org.freedesktop.Hal.Device',
> -                                'org.freedesktop.Hal',
> -                                udi)
> -
> -        self._level = self._get_level()
> -        self._charging = self._get_charging()
> -        self._discharging = self._get_discharging()
> -        self._present = self._get_present()
> -
> -    def _get_level(self):
> -        try:
> -            return self._battery.GetProperty(_LEVEL_PROP)
> -        except dbus.DBusException:
> -            logging.error('Cannot access %s', _LEVEL_PROP)
> -            return 0
> -
> -    def _get_charging(self):
> -        try:
> -            return self._battery.GetProperty(_CHARGING_PROP)
> -        except dbus.DBusException:
> -            logging.error('Cannot access %s', _CHARGING_PROP)
> -            return False
> +    __gsignals__ = {
> +        'updated': (gobject.SIGNAL_RUN_FIRST, gobject.TYPE_NONE, ([])),
> +    }
>
> -    def _get_discharging(self):
> +    def __init__(self, battery):
> +        gobject.GObject.__init__(self)
> +        self._battery = battery
> +        self._battery_props_iface = dbus.Interface(self._battery,
> +            dbus.PROPERTIES_IFACE)
> +        self._battery.connect_to_signal('Changed', self._battery_changed,
> +            dbus_interface='org.freedesktop.UPower.Device')

For the callback name I would use self.__battery_changed_cb

> +        self._fetch()

> +    def _fetch(self):

Why not make it _get_battery_properties ?

> +        """Get current values from UPower."""
> +        # pylint: disable-msg=W0201
>           try:
> -            return self._battery.GetProperty(_DISCHARGING_PROP)
> +            dbus_props = self._battery_props_iface.GetAll(
> +                'org.freedesktop.UPower.Device')
>           except dbus.DBusException:
> -            logging.error('Cannot access %s', _DISCHARGING_PROP)
> -            return False
> +            logging.error('Cannot access battery properties')
> +            dbus_props = {}
>
> -    def _get_present(self):
> -        try:
> -            return self._battery.GetProperty(_PRESENT_PROP)
> -        except dbus.DBusException:
> -            logging.error('Cannot access %s', _PRESENT_PROP)
> -            return False
> +        self._level = dbus_props.get('Percentage', 0)
> +        self._state = dbus_props.get('State', _UP_STATE_UNKNOWN)
> +        self._present = dbus_props.get('IsPresent', False)
> +        self._time_to_empty = dbus_props.get('TimeToEmpty', 0)
> +        self._time_to_full = dbus_props.get('TimeToFull', 0)
>
>       def do_get_property(self, pspec):
> +        """Return current value of given GObject property."""
>           if pspec.name == 'level':
>               return self._level
>           if pspec.name == 'charging':
> -            return self._charging
> +            return self._state == _UP_STATE_CHARGING
>           if pspec.name == 'discharging':
> -            return self._discharging
> +            return self._state == _UP_STATE_DISCHARGING
>           if pspec.name == 'present':
>               return self._present
> +        if pspec.name == 'time-remaining':
> +            if self._state == _UP_STATE_CHARGING:
> +                return self._time_to_full
> +            if self._state == _UP_STATE_DISCHARGING:
> +                return self._time_to_empty
> +            return 0
>
>       def get_type(self):
>           return 'battery'
>
> -    def _battery_changed(self, num_changes, changes_list):
> -        for change in changes_list:
> -            if change[0] == _LEVEL_PROP:
> -                self._level = self._get_level()
> -                self.notify('level')
> -            elif change[0] == _CHARGING_PROP:
> -                self._charging = self._get_charging()
> -                self.notify('charging')
> -            elif change[0] == _DISCHARGING_PROP:
> -                self._discharging = self._get_discharging()
> -                self.notify('discharging')
> -            elif change[0] == _PRESENT_PROP:
> -                self._present = self._get_present()
> -                self.notify('present')
> +    def _battery_changed(self, *args):
> +        old_level = self._level
> +        old_state = self._state
> +        old_present = self._present
> +        old_time = self.props.time_remaining
> +        self._fetch()
> +        if self._level != old_level:
> +            self.notify('level')
> +        if self._state != old_state:
> +            self.notify('charging')
> +            self.notify('discharging')
> +        if self._present != old_present:
> +            self.notify('present')
> +        if self.props.time_remaining != old_time:
> +            self.notify('time-remaining')
> +
> +        self.emit('updated')
>
>
>   def setup(tray):
>       bus = dbus.Bus(dbus.Bus.TYPE_SYSTEM)
> -    proxy = bus.get_object('org.freedesktop.Hal',
> -                            '/org/freedesktop/Hal/Manager')
> -    hal_manager = dbus.Interface(proxy, 'org.freedesktop.Hal.Manager')
> -
> -    for udi in hal_manager.FindDeviceByCapability('battery'):
> -        tray.add_device(DeviceView(udi))
> +    up_proxy = bus.get_object('org.freedesktop.UPower',
> +                            '/org/freedesktop/UPower')
> +    upower = dbus.Interface(up_proxy, 'org.freedesktop.UPower')
> +
> +    for battery_path in upower.EnumerateDevices():

I would not call it battery_path here as you are enumerating devices, 
hence device_path (of course you need to follow this as well in the code 
below).

> +        battery = bus.get_object('org.freedesktop.UPower', battery_path)
> +        batt_prop_iface = dbus.Interface(battery, dbus.PROPERTIES_IFACE)
> +        device_type = batt_prop_iface.Get('org.freedesktop.UPower.Device',
> +            'Type')
> +        if device_type == _UP_TYPE_BATTERY:
> +            tray.add_device(DeviceView(battery))

Regards,
    Simon


More information about the Sugar-devel mailing list