From 1cc532d600c9fa5176defac8d86360239f8b6a5e Mon Sep 17 00:00:00 2001 From: Daniel Pavel Date: Sat, 1 Dec 2012 19:12:53 +0200 Subject: [PATCH] fixed orphaned weakrefs when unpairing a device --- app/listener.py | 65 ++++++++--------- app/ui/main_window.py | 82 +++++++--------------- app/ui/status_icon.py | 8 +-- lib/logitech/unifying_receiver/listener.py | 2 +- lib/logitech/unifying_receiver/receiver.py | 12 ++-- lib/logitech/unifying_receiver/status.py | 34 ++++++--- 6 files changed, 94 insertions(+), 109 deletions(-) diff --git a/app/listener.py b/app/listener.py index 0e21c7e4..66f20649 100644 --- a/app/listener.py +++ b/app/listener.py @@ -10,8 +10,6 @@ from types import MethodType as _MethodType from logitech.unifying_receiver import (Receiver, listener as _listener, - hidpp10 as _hidpp10, - hidpp20 as _hidpp20, status as _status) # @@ -19,7 +17,6 @@ from logitech.unifying_receiver import (Receiver, # class _DUMMY_RECEIVER(object): - # __slots__ = ['name', 'max_devices', 'status'] __slots__ = [] name = Receiver.name kind = None @@ -29,12 +26,15 @@ class _DUMMY_RECEIVER(object): __str__ = lambda self: 'DUMMY' DUMMY = _DUMMY_RECEIVER() +from collections import namedtuple +_GHOST_DEVICE = namedtuple('_GHOST_DEVICE', ['number', 'name', 'kind', 'status']) +del namedtuple + # # # -_DEVICE_STATUS_POLL = 30 # seconds -_DEVICE_TIMEOUT = 2 * _DEVICE_STATUS_POLL # seconds +_POLL_TICK = 30 # seconds class ReceiverListener(_listener.EventsListener): @@ -42,7 +42,7 @@ class ReceiverListener(_listener.EventsListener): """ def __init__(self, receiver, status_changed_callback=None): super(ReceiverListener, self).__init__(receiver, self._events_handler) - self.tick_period = _DEVICE_STATUS_POLL + self.tick_period = _POLL_TICK self._last_tick = 0 self.status_changed_callback = status_changed_callback @@ -57,13 +57,13 @@ class ReceiverListener(_listener.EventsListener): # read these as soon as possible, they will be used everywhere dev.protocol, dev.codename dev.status = _status.DeviceStatus(dev, self._status_changed) + self._status_changed(r) return dev receiver.__register_new_device = receiver.register_new_device receiver.register_new_device = _MethodType(_register_with_status, receiver) def has_started(self): _log.info("events listener has started") - self._status_changed(self.receiver) self.receiver.enable_notifications() self.receiver.notify_devices() self._status_changed(self.receiver, _status.ALERT.LOW) @@ -80,7 +80,7 @@ class ReceiverListener(_listener.EventsListener): if _log.isEnabledFor(_DEBUG): _log.debug("tick: polling status: %s %s", self.receiver, list(iter(self.receiver))) - if self._last_tick > 0 and timestamp - self._last_tick > _DEVICE_STATUS_POLL * 2: + if self._last_tick > 0 and timestamp - self._last_tick > _POLL_TICK * 2: # if we missed a couple of polls, most likely the computer went into # sleep, and we have to reinitialize the receiver again _log.warn("possible sleep detected, closing this listener") @@ -90,58 +90,51 @@ class ReceiverListener(_listener.EventsListener): # read these in case they haven't been read already self.receiver.serial, self.receiver.firmware - if self.receiver.status.lock_open: # don't mess with stuff while pairing return for dev in self.receiver: - if dev.status: - # read these in case they haven't been read already - dev.protocol, dev.serial, dev.firmware - - if _status.BATTERY_LEVEL not in dev.status: - battery = _hidpp20.get_battery(dev) or _hidpp10.get_battery(dev) - if battery: - dev.status[_status.BATTERY_LEVEL], dev.status[_status.BATTERY_STATUS] = battery - self._status_changed(dev) - - elif len(dev.status) > 0 and timestamp - dev.status.updated > _DEVICE_TIMEOUT: - dev.status.clear() - self._status_changed(dev, _status.ALERT.LOW) + assert dev.status is not None + dev.status.poll(timestamp) def _status_changed(self, device, alert=_status.ALERT.NONE, reason=None): if _log.isEnabledFor(_DEBUG): _log.debug("status_changed %s: %s (%X) %s", device, None if device is None else device.status, alert, reason or '') if self.status_changed_callback: + r = self.receiver or DUMMY if device is None or device.kind is None: - self.status_changed_callback(self.receiver or DUMMY, None, alert, reason) + self.status_changed_callback(r, None, alert, reason) else: - self.status_changed_callback(self.receiver or DUMMY, device, alert, reason) - # if device.status is None: - # self.status_changed_callback(self.receiver, None) + if device.status is None: + # device was unpaired, and since the object is weakref'ed + # it won't be valid for much longer + device = _GHOST_DEVICE(number=device.number, name=device.name, kind=device.kind, status=None) + self.status_changed_callback(r, device, alert, reason) + if device.status is None: + self.status_changed_callback(r) def _events_handler(self, event): assert self.receiver if event.devnumber == 0xFF: - # a receiver envent + # a receiver event if self.receiver.status is not None: self.receiver.status.process_event(event) else: - # a paired device envent + # a device event assert event.devnumber > 0 and event.devnumber <= self.receiver.max_devices + already_known = event.devnumber in self.receiver dev = self.receiver[event.devnumber] if dev: - if dev.status is not None: - dev.status.process_event(event) - else: - if self.receiver.status.lock_open: + assert dev.status is not None + dev.status.process_event(event) + if self.receiver.status.lock_open and not already_known: + # this should be the first event after a device was paired assert event.sub_id == 0x41 and event.address == 0x04 _log.info("pairing detected new device") - dev = self.receiver.status.device_paired(event.devnumber) - dev.status.process_event(event) - else: - _log.warn("received event %s for invalid device %d", event, event.devnumber) + self.receiver.status.new_device = dev + else: + _log.warn("received event %s for invalid device %d", event, event.devnumber) def __str__(self): return '' % (self.receiver.path, self.receiver.status) diff --git a/app/ui/main_window.py b/app/ui/main_window.py index bb5eb8d8..6dd19f50 100644 --- a/app/ui/main_window.py +++ b/app/ui/main_window.py @@ -59,7 +59,8 @@ def _make_receiver_box(name): info_box.add(info_label) info_box.set_shadow_type(Gtk.ShadowType.ETCHED_IN) - toggle_info_action = ui.action._toggle_action('info', 'Receiver info', _toggle_info_box, info_label, info_box, frame, _update_receiver_info_label) + toggle_info_action = ui.action._toggle_action('info', 'Receiver info', + _toggle_info_box, info_box, frame, _update_receiver_info_label) toolbar.insert(toggle_info_action.create_tool_item(), 0) toolbar.insert(ui.action.pair(frame).create_tool_item(), -1) # toolbar.insert(ui.action.about.create_tool_item(), -1) @@ -132,8 +133,10 @@ def _make_device_box(index): info_box = Gtk.Frame() info_box.add(info_label) + info_box.set_shadow_type(Gtk.ShadowType.ETCHED_IN) - toggle_info_action = ui.action._toggle_action('info', 'Device info', _toggle_info_box, info_label, info_box, frame, _update_device_info_label) + toggle_info_action = ui.action._toggle_action('info', 'Device info', + _toggle_info_box, info_box, frame, _update_device_info_label) toolbar.insert(toggle_info_action.create_tool_item(), 0) toolbar.insert(ui.action.unpair(frame).create_tool_item(), -1) @@ -216,46 +219,15 @@ def create(title, name, max_devices, systray=False): # def _update_device_info_label(label, dev): - need_update = False + items = [('Wireless PID', dev.wpid), ('Serial', dev.serial)] + hid = dev.protocol + if hid: + items += [('Protocol', 'HID++ %1.1f' % dev.protocol)] + firmware = dev.firmware + if firmware: + items += [(f.kind, f.name + ' ' + f.version) for f in firmware] - if 'wpid' in label._fields: - wpid = label._fields['wpid'] - else: - wpid = label._fields['wpid'] = dev.wpid - need_update = True - - if 'serial' in label._fields: - serial = label._fields['serial'] - else: - serial = label._fields['serial'] = dev.serial - need_update = True - - if 'firmware' in label._fields: - firmware = label._fields['firmware'] - else: - if dev.status: - firmware = label._fields['firmware'] = dev.firmware - need_update = True - else: - firmware = None - - if 'hid' in label._fields: - hid = label._fields['hid'] - else: - if dev.status: - hid = label._fields['hid'] = 'HID++ %1.1f' % dev.protocol - need_update = True - else: - hid = None - - if need_update: - items = [('Wireless PID', wpid), ('Serial', serial)] - if hid: - items += [('Protocol', hid)] - if firmware: - items += [(f.kind, f.name + ' ' + f.version) for f in firmware] - - label.set_markup('' + '\n'.join('%-12s: %s' % item for item in items) + '') + label.set_markup('' + '\n'.join('%-12s: %s' % item for item in items) + '') def _update_receiver_info_label(label, dev): @@ -265,12 +237,12 @@ def _update_receiver_info_label(label, dev): label.set_markup('' + '\n'.join('%-10s: %s' % item for item in items) + '') -def _toggle_info_box(action, label_widget, box_widget, frame, update_function): +def _toggle_info_box(action, box, frame, update_function): if action.get_active(): - box_widget.set_visible(True) - GObject.timeout_add(50, update_function, label_widget, frame._device) + box.set_visible(True) + GObject.timeout_add(50, update_function, box.get_child(), frame._device) else: - box_widget.set_visible(False) + box.set_visible(False) def _update_receiver_box(frame, receiver): @@ -282,7 +254,7 @@ def _update_receiver_box(frame, receiver): icon.set_sensitive(True) if receiver.status.lock_open: if pairing_icon._tick == 0: - def _tick(i, s): + def _pairing_tick(i, s): if s and s.lock_open: i.set_sensitive(bool(i._tick % 2)) i._tick += 1 @@ -291,7 +263,7 @@ def _update_receiver_box(frame, receiver): i.set_sensitive(True) i._tick = 0 pairing_icon.set_visible(True) - GObject.timeout_add(1000, _tick, pairing_icon, receiver.status) + GObject.timeout_add(1000, _pairing_tick, pairing_icon, receiver.status) else: pairing_icon.set_visible(False) pairing_icon.set_sensitive(True) @@ -383,14 +355,7 @@ def update(window, receiver, device=None): frames = list(vbox.get_children()) assert len(frames) == 1 + receiver.max_devices, frames - if device is None: - _update_receiver_box(frames[0], receiver) - if not receiver: - for frame in frames[1:]: - frame.set_visible(False) - frame.set_name(_PLACEHOLDER) - frame._device = None - else: + if device: frame = frames[device.number] if device.status is None: frame.set_visible(False) @@ -398,3 +363,10 @@ def update(window, receiver, device=None): frame._device = None else: _update_device_box(frame, device) + else: + _update_receiver_box(frames[0], receiver) + if not receiver: + for frame in frames[1:]: + frame.set_visible(False) + frame.set_name(_PLACEHOLDER) + frame._device = None diff --git a/app/ui/status_icon.py b/app/ui/status_icon.py index ed2de42c..ef2eadb1 100644 --- a/app/ui/status_icon.py +++ b/app/ui/status_icon.py @@ -45,7 +45,7 @@ def _icon_with_battery(s): assert mask mask = GdkPixbuf.Pixbuf.new_from_file(mask) assert mask.get_width() == 128 and mask.get_height() == 128 - mask.saturate_and_pixelate(mask, 0.8, False) + mask.saturate_and_pixelate(mask, 0.7, False) battery = ui.icon_file(battery_icon, 128) assert battery @@ -64,12 +64,12 @@ def update(icon, receiver, device=None): # print ("icon update", receiver, receiver.status, len(receiver._devices), device) battery_status = None - if device is not None: - icon._devices[device.number] = device + if device: + icon._devices[device.number] = None if device.status is None else device lines = [ui.NAME + ': ' + str(receiver.status), ''] if receiver: - for k in range(1, 1+ receiver.max_devices): + for k in range(1, 1 + receiver.max_devices): dev = icon._devices.get(k) if dev is None: continue diff --git a/lib/logitech/unifying_receiver/listener.py b/lib/logitech/unifying_receiver/listener.py index c057a676..6000f0b3 100644 --- a/lib/logitech/unifying_receiver/listener.py +++ b/lib/logitech/unifying_receiver/listener.py @@ -83,7 +83,7 @@ class ThreadedHandle(object): # _EVENT_READ_TIMEOUT = 500 -_IDLE_READS = 5 +_IDLE_READS = 4 class EventsListener(_threading.Thread): diff --git a/lib/logitech/unifying_receiver/receiver.py b/lib/logitech/unifying_receiver/receiver.py index b89e399f..9013a629 100644 --- a/lib/logitech/unifying_receiver/receiver.py +++ b/lib/logitech/unifying_receiver/receiver.py @@ -12,7 +12,7 @@ del getLogger from . import base as _base from . import hidpp10 as _hidpp10 from . import hidpp20 as _hidpp20 -from .common import strhex as _strhex, FirmwareInfo as _FirmwareInfo +from .common import strhex as _strhex from .devices import DEVICES as _DEVICES # @@ -221,6 +221,7 @@ class Receiver(object): if self._devices.get(number) is not None: raise IndexError("device number %d already registered" % number) dev = PairedDevice(self, number) + # create a device object, but only use it if the receiver knows about it if dev.wpid: _log.info("registered new device %d (%s)", number, dev.wpid) self._devices[number] = dev @@ -241,7 +242,7 @@ class Receiver(object): def __iter__(self): for number in range(1, 1 + MAX_PAIRED_DEVICES): - dev = self.__getitem__(number) + dev = self._devices.get(number) if dev is not None: yield dev @@ -249,8 +250,9 @@ class Receiver(object): if not self.handle: return None - if key in self._devices: - return self._devices[key] + dev = self._devices.get(key) + if dev is not None: + return dev if type(key) != int: raise TypeError('key must be an integer') @@ -278,7 +280,7 @@ class Receiver(object): def __contains__(self, dev): if type(dev) == int: - return dev in self._devices + return self._devices.get(dev) is not None return self.__contains__(dev.number) diff --git a/lib/logitech/unifying_receiver/status.py b/lib/logitech/unifying_receiver/status.py index a3109fcd..6461d8d2 100644 --- a/lib/logitech/unifying_receiver/status.py +++ b/lib/logitech/unifying_receiver/status.py @@ -27,6 +27,9 @@ BATTERY_STATUS='battery-status' LIGHT_LEVEL='light-level' ERROR='error' +# make sure we try to update the device status at least once a minute +_STATUS_TIMEOUT = 60 # seconds + # # # @@ -51,11 +54,6 @@ class ReceiverStatus(dict): '1 device found.' if count == 1 else '%d devices found.' % count) - def device_paired(self, number): - _log.info("new device paired") - dev = self.new_device = self._receiver.register_new_device(number) - return dev - def _changed(self, alert=ALERT.LOW, reason=None): # self.updated = _timestamp() self._changed_callback(self._receiver, alert=alert, reason=reason) @@ -110,7 +108,7 @@ class DeviceStatus(dict): return self.updated and self._active __nonzero__ = __bool__ - def _changed(self, active=True, alert=ALERT.NONE, reason=None): + def _changed(self, active=True, alert=ALERT.NONE, reason=None, timestamp=None): assert self._changed_callback self._active = active if not active: @@ -120,11 +118,32 @@ class DeviceStatus(dict): self[BATTERY_LEVEL] = battery if self.updated == 0: alert |= ALERT.LOW - self.updated = _timestamp() + self.updated = timestamp or _timestamp() # if _log.isEnabledFor(_DEBUG): # _log.debug("device %d changed: active=%s %s", self._device.number, self._active, dict(self)) self._changed_callback(self._device, alert, reason) + def poll(self, timestamp): + if self._active: + d = self._device + # read these in case they haven't been read already + d.protocol, d.serial, d.firmware + + if BATTERY_LEVEL not in self: + if d.protocol >= 2.0: + battery = _hidpp20.get_battery(d) + else: + battery = _hidpp10.get_battery(d) + if battery: + self[BATTERY_LEVEL], self[BATTERY_STATUS] = battery + self._changed(timestamp=timestamp) + + elif len(self) > 0 and timestamp - self.updated > _STATUS_TIMEOUT: + # if the device has been inactive for too long, clear out any known + # properties, they are most likely obsolete anyway + self.clear() + self._changed(active=False, alert=ALERT.LOW, timestamp=timestamp) + def process_event(self, event): if event.sub_id == 0x40: if event.address == 0x02: @@ -132,7 +151,6 @@ class DeviceStatus(dict): self.clear() self._device.status = None self._changed(False, ALERT.HIGH, 'unpaired') - self._device = None else: _log.warn("device %d disconnection notification %s with unknown type %02X", self._device.number, event, event.address) return True