diff --git a/lib/logitech_receiver/device.py b/lib/logitech_receiver/device.py index 725a7259..575d6c7a 100644 --- a/lib/logitech_receiver/device.py +++ b/lib/logitech_receiver/device.py @@ -463,7 +463,7 @@ class Device: if self.persister is not None: self.persister["_config_cookie"] = [cookie[0], cookie[1]] - def apply_settings_if_needed(self): + def apply_settings_if_needed(self, force=False): """Cookie-gated dedup helper for repeat WIRELESS_DEVICE_STATUS reconfig notifications on an already-active device. Skips when the live ConfigChange cookie matches the value stored by the most @@ -472,10 +472,17 @@ class Device: whose firmware resets the cookie to a fixed value would falsely match a stored cookie from a prior session and skip the apply the device actually needs. + `force` skips the cookie comparison (but still re-records): pass it + when the device powered on while already marked active — no + activation apply ran in changed(), yet volatile state (RGB buffers, + host-mode lighting) is gone even on devices whose ConfigChange + cookie survives the power cycle (e.g. G915 TKL). When the device + was inactive, changed() just applied everything, so forcing here + would only push a redundant duplicate. Returns True if apply ran, False if it was skipped.""" if not self.online: return False - if self.protocol >= 2.0 and self.features and SupportedFeature.CONFIG_CHANGE in self.features: + if not force and self.protocol >= 2.0 and self.features and SupportedFeature.CONFIG_CHANGE in self.features: live = _hidpp20.get_configuration_cookie(self) if live and len(live) >= 2: stored = self.persister.get("_config_cookie") if self.persister else None diff --git a/lib/logitech_receiver/device_quirks.py b/lib/logitech_receiver/device_quirks.py index e2c09009..6fac2c52 100644 --- a/lib/logitech_receiver/device_quirks.py +++ b/lib/logitech_receiver/device_quirks.py @@ -69,6 +69,38 @@ HEADSET_SIGNATURE_EFFECTS_ALLOWED: dict[str, dict[int, set[str]]] = { } +# Keyboards whose firmware breaks the F-row / media keys when the RGB +# SW-control claim switches them to host mode (ONBOARD_PROFILES fn 0x10 mode +# 0x02) — Solaar #1100. A USB capture of G HUB's bring-up shows it drives the +# LEDs from onboard mode via the RGB SetSWControl claim alone, never making +# that switch. For these models the claim keeps onboard mode, which also avoids +# the host-mode transition that made the firmware re-assert the M/MR indicator +# over a per-key cell (the G915 TKL F4 blackout). +RGB_CLAIM_KEEPS_ONBOARD_MODE: set[str] = { + "B35F408EC343", # G915 TKL LIGHTSPEED — verified: host mode disables F-keys/media keys +} + +# Keyboards where switching onboard profiles drops the software per-key/zone +# paint (the firmware loads the switched-to profile's own onboard lighting). +# For these, re-assert the claim + repaint on a profile-change notification so +# an accidental profile switch doesn't strand the user's software scheme. +RGB_REPAINT_ON_PROFILE_CHANGE: set[str] = { + "B35F408EC343", # G915 TKL LIGHTSPEED +} + + +def rgb_claim_keeps_onboard_mode(device) -> bool: + """True when the RGB SW-control claim must NOT switch this model to host + mode (it would disable the F-row / media keys).""" + return (getattr(device, "modelId", None) or "") in RGB_CLAIM_KEEPS_ONBOARD_MODE + + +def rgb_repaint_on_profile_change(device) -> bool: + """True when a profile-change notification should re-apply the claimed + software RGB state on this model (the switch drops the per-key paint).""" + return (getattr(device, "modelId", None) or "") in RGB_REPAINT_ON_PROFILE_CHANGE + + def rgb_effects_nvconfig_allowed_fields(device, cap_id: int) -> set[str] | None: """Color fields to expose for an 0x8071 NvConfig boot effect. diff --git a/lib/logitech_receiver/notifications.py b/lib/logitech_receiver/notifications.py index 933d2477..132fa354 100644 --- a/lib/logitech_receiver/notifications.py +++ b/lib/logitech_receiver/notifications.py @@ -322,15 +322,21 @@ def _process_feature_notification(device: Device, notification: HIDPPNotificatio if notification.address == 0x00: if logger.isEnabledFor(logging.DEBUG): logger.debug("wireless status: %s", notification) - reason = "powered on" if notification.data[2] == 1 else None + powered_on = notification.data[2] == 1 + reason = "powered on" if powered_on else None if notification.data[1] == 1: # device is asking for software reconfiguration alert = Alert.NONE + was_active = bool(device.online) # before changed() updates it device.changed(active=True, alert=alert, reason=reason) - # changed(active=True) already runs apply_settings_if_needed on - # the first transition; for follow-up reconfig notifications - # on an already-active device, fire the gate here so the - # cookie comparison decides whether to re-push. - device.apply_settings_if_needed() + # changed(active=True) applies all settings unconditionally on + # the inactive→active transition; for follow-up reconfig + # notifications on an already-active device, fire the gate here + # so the cookie comparison decides whether to re-push. A + # power-on while already marked active forces the apply: no + # activation apply ran, yet volatile state (RGB buffers, + # host-mode lighting) is gone even when the cookie survived + # the cycle (e.g. G915 TKL). + device.apply_settings_if_needed(force=powered_on and was_active) else: logger.warning("%s: unknown WIRELESS %s", device, notification) diff --git a/lib/logitech_receiver/settings_templates.py b/lib/logitech_receiver/settings_templates.py index a3891b35..8cc6957d 100644 --- a/lib/logitech_receiver/settings_templates.py +++ b/lib/logitech_receiver/settings_templates.py @@ -542,6 +542,18 @@ def profile_change(device, profile_sector): device.setting_callback(device, AdjustableDpi, [profile.resolutions[resolution_index]]) device.setting_callback(device, ReportRate, [profile.report_rate]) break + # A profile switch reloads that profile's onboard lighting, dropping the + # software per-key/zone paint. On models prone to this, re-assert the claim + # and repaint (only when Solaar already holds the LED claim) so an + # accidental profile switch doesn't strand the user's scheme. + if device_quirks.rgb_repaint_on_profile_change(device): + for s in device.settings or []: + if s.name == "rgb_control" and getattr(s, "_value", None): + try: + s._claim_sw_control(device) + except Exception as e: + logger.warning("%s: repaint after profile change failed: %s", device, e) + break class OnboardProfiles(settings.Setting): @@ -3330,11 +3342,16 @@ class RGBControl(settings.Setting): return value def _claim_sw_control(self, device): - # Disable firmware power management via profile management or onboard profiles - if device.features and _F.PROFILE_MANAGEMENT in device.features: - device.feature_request(_F.PROFILE_MANAGEMENT, 0x60, b"\x05") - elif device.features and _F.ONBOARD_PROFILES in device.features: - device.feature_request(_F.ONBOARD_PROFILES, 0x10, b"\x02") + # Disable firmware power management via profile management or onboard + # profiles. Skipped on models where the onboard->host mode switch + # disables the F-row / media keys (device_quirks, Solaar #1100): they + # drive the LEDs from onboard mode via the SetSWControl claim alone, + # exactly as G HUB does. + if not device_quirks.rgb_claim_keeps_onboard_mode(device): + if device.features and _F.PROFILE_MANAGEMENT in device.features: + device.feature_request(_F.PROFILE_MANAGEMENT, 0x60, b"\x05") + elif device.features and _F.ONBOARD_PROFILES in device.features: + device.feature_request(_F.ONBOARD_PROFILES, 0x10, b"\x02") # Claim LED pipeline: SetSWControl(mode=3, flags=5) device.feature_request(_F.RGB_EFFECTS, 0x50, rgb_power.SW_ACTIVE) # Reset per-key one-shot flags so the first write after this claim @@ -3349,15 +3366,15 @@ class RGBControl(settings.Setting): # Register cleanup for graceful release on device close if rgb_power.cleanup not in device.cleanups: device.cleanups.append(rgb_power.cleanup) - # Repaint LEDs with Solaar's saved state. Without this the firmware's - # last-active onboard profile keeps showing until the user changes - # something — the takeover would look like it did nothing. + # Repaint saved zone effects and the per-key buffer after the claim. + # (No settle-repaint timer needed: keeping onboard mode on the affected + # models avoids the host-mode transition that caused the F4 blackout.) self._repaint_after_claim(device) def _repaint_after_claim(self, device): - """Push saved zone effects and (if opted in) per-key buffer to the - device after a fresh SW claim. Best-effort: individual failures get - logged but don't abort the rest of the repaint.""" + """Push saved zone effects and the per-key buffer to the device after + a fresh SW claim. Best-effort: individual failures get logged but + don't abort the rest of the repaint.""" for s in device.settings: if s.name.startswith("rgb_zone_") and s._value is not None: try: @@ -3871,12 +3888,37 @@ class PerKeyLighting(settings.Settings): return common.ColorInt(value) return value + def _valid_zones(self): + """Zone IDs the device reported, or None when no validator is + attached yet (then no filtering can happen).""" + choices = getattr(getattr(self, "_validator", None), "choices", None) + return {int(k) for k in choices} if choices is not None else None + + def _sanitize_map(self, value): + """Drop zone keys the device never reported (e.g. the editor's gap + placeholder, or junk from an older persisted config). A single bad + key would otherwise abort the whole frame at pack time. + Filters map KEYS only — the -1 "No change" sentinel as a map VALUE + is legitimate and handled separately in _send_perkey_frame.""" + valid = self._valid_zones() + if not isinstance(value, dict) or valid is None: + return value + bad = [k for k in value if int(k) not in valid] + if bad: + logger.warning("%s: dropping invalid per-key zones %s", self.name, bad) + value = {k: v for k, v in value.items() if int(k) in valid} + return value + def update(self, value, save=True): if isinstance(value, dict): - value = {k: self._wrap_color(v) for k, v in value.items()} + value = {k: self._wrap_color(v) for k, v in self._sanitize_map(value).items()} super().update(value, save) def update_key_value(self, key, value, save=True): + valid = self._valid_zones() + if valid is not None and int(key) not in valid: + logger.warning("%s: ignoring write to invalid per-key zone %s", self.name, key) + return super().update_key_value(key, self._wrap_color(value), save) def _sw_control_held(self): @@ -4003,6 +4045,9 @@ class PerKeyLighting(settings.Settings): # live line — it now matches what's actually on the keyboard. self._pre_read(cached) if self._value is not None: + # The persisted map may carry zones from a buggy editor or an + # older config — heal it here so they age out on the next save. + self._value = self._sanitize_map(self._value) return self._value reply_map = {} for key in self._validator.choices: @@ -4064,6 +4109,7 @@ class PerKeyLighting(settings.Settings): def write(self, map, save=True): if self._device.online: + map = self._sanitize_map(map) # the frame pack below can't take invalid zones # Persist undimmed (single source of truth). self.update(map, save) if not self._sw_control_held(): @@ -4100,6 +4146,17 @@ class PerKeyLighting(settings.Settings): def write_key_value(self, key, value, save=True): no_change = special_keys.COLORSPLUS["No change"] zone_id = int(key) + valid = self._valid_zones() + if valid is not None and zone_id not in valid: + logger.warning("%s: ignoring write to invalid per-key zone %s", self.name, zone_id) + return value + if self._value is None: + # 0x8081 is write-only — read() loads persisted state or the all- + # "No change" sentinel map. Both branches below mutate _value, so + # guard here (like the base class does). `is None` rather than the + # base class's falsy test: read() never yields an empty map here, + # so None is precisely "never initialized". + self.read() if value != no_change: self.update_key_value(zone_id, value, save) if not self._device.online: diff --git a/lib/solaar/ui/perkey/canvas.py b/lib/solaar/ui/perkey/canvas.py index 995f0a50..cea32d07 100644 --- a/lib/solaar/ui/perkey/canvas.py +++ b/lib/solaar/ui/perkey/canvas.py @@ -54,6 +54,11 @@ GUTTER_PX = 4 STRIP_GAP_PX = 16 PADDING_PX = 8 +# Phantom anchor zone for matrix gaps (positions with no key). Never a real +# zone, never sent to the device — distinct from the -1 "No change" color +# VALUE (special_keys.COLORSPLUS / control.NO_CHANGE). +GAP_ZONE_ID = -1 + class KeyboardCanvas(Gtk.DrawingArea): __gsignals__ = { @@ -178,7 +183,7 @@ class KeyboardCanvas(Gtk.DrawingArea): col = int((x - PADDING_PX) // (CELL_PX + GUTTER_PX)) row = int((y - PADDING_PX) // (CELL_PX + GUTTER_PX)) if 0 <= col < cols and 0 <= row < rows: - return BoundCell(cell=Cell(zone_id=-1, row=row, col=col), bound=False) + return BoundCell(cell=Cell(zone_id=GAP_ZONE_ID, row=row, col=col), bound=False) return None # ---- draw ---- @@ -438,6 +443,10 @@ class KeyboardCanvas(Gtk.DrawingArea): delta: dict[int, int] = {} if tool is not None: delta = tool.compute(self._press_cell, self._motion_cell, list(self._brush_path), ctx) + # Strokes can sweep over layout gaps; drop their phantom cells so + # they never reach the sink. This filters KEYS — "No change" color + # VALUES (-1) pass through untouched. + delta = {z: c for z, c in delta.items() if z != GAP_ZONE_ID} self._press_cell = None self._motion_cell = None self._brush_path = [] diff --git a/tests/logitech_receiver/test_notifications.py b/tests/logitech_receiver/test_notifications.py index cbe51aab..8dc8a4f4 100644 --- a/tests/logitech_receiver/test_notifications.py +++ b/tests/logitech_receiver/test_notifications.py @@ -279,6 +279,22 @@ def test_process_feature_notification(mocker, hidpp_notification, feature): assert result is True +@pytest.mark.parametrize("was_active, expected_force", [(True, True), (False, False)]) +def test_wireless_power_on_reconfig_forces_apply_only_when_already_active(mocker, was_active, expected_force): + """A powered-on reconfig request must bypass the config-cookie gate + exactly when changed() ran no activation apply (device already active). + On the inactive→active transition changed() just applied everything, so + forcing there would push a redundant duplicate.""" + device = mocker.Mock() + device.online = was_active # state before changed() updates it + device.features.get_feature.return_value = SupportedFeature.WIRELESS_DEVICE_STATUS + hidpp_notification = HIDPPNotification(0, 0, sub_id=0x05, address=0x00, data=b"\x00\x01\x01") + + notifications._process_feature_notification(device, hidpp_notification) + + device.apply_settings_if_needed.assert_called_once_with(force=expected_force) + + def test_process_receiver_notification_invalid(mocker): invalid_sub_id = 0x30 notification_data = b"\x02" diff --git a/tests/logitech_receiver/test_rgb_power.py b/tests/logitech_receiver/test_rgb_power.py index cafe6d59..d614f598 100644 --- a/tests/logitech_receiver/test_rgb_power.py +++ b/tests/logitech_receiver/test_rgb_power.py @@ -672,3 +672,121 @@ def test_perkey_write_key_value_skipped_when_zone_is_animation(monkeypatch): s.write_key_value(7, 0xFF0000) s._send_zone_color.assert_not_called() + + +def test_perkey_write_key_value_initializes_unread_value(monkeypatch): + """The CLI writes a single key without ever reading the setting, so + _value is still None. write_key_value must initialize it via read() + (persisted state or the sentinel map — 0x8081 is write-only) instead + of crashing with a None dereference in update_key_value.""" + from unittest.mock import MagicMock + + from logitech_receiver import settings_templates + + static_zone = _FakeZoneSetting("rgb_zone_1", _ValueWithID(0x01)) + + s = settings_templates.PerKeyLighting.__new__(settings_templates.PerKeyLighting) + device = MagicMock() + device.online = True + device.settings = [static_zone] + s._device = device + s._value = None # fresh CLI invocation: nothing has read the setting + s._validator = MagicMock() + s._validator.choices = [7] + s._pre_read = lambda cached=True: None + s._has_rgb_effects = True + s._send_with_retry = MagicMock(return_value=True) + s._send_zone_color = MagicMock(return_value=True) + s._fill_unset_zones_with_base_color = MagicMock(return_value=True) + monkeypatch.setattr(rgb_power, "translate_for_device", lambda d, c: c) + monkeypatch.setattr(rgb_power, "get_manager", lambda d: None) + + s.write_key_value(7, 0xFF0000) + + assert s._value[7] == 0xFF0000 + s._send_zone_color.assert_called_once() + + +def _perkey_under_test(monkeypatch, choices): + """PerKeyLighting wired to a static-zone device with mocked wire sends.""" + from unittest.mock import MagicMock + + from logitech_receiver import settings_templates + + static_zone = _FakeZoneSetting("rgb_zone_1", _ValueWithID(0x01)) + s = settings_templates.PerKeyLighting.__new__(settings_templates.PerKeyLighting) + device = MagicMock() + device.online = True + device.settings = [static_zone] + device.kind = None # not a mouse — no artanis prep + s._device = device + s._value = None + s._validator = MagicMock() + s._validator.choices = choices + s._pre_read = lambda cached=True: None + s._has_rgb_effects = True + s._send_with_retry = MagicMock(return_value=True) + s._send_zone_color = MagicMock(return_value=True) + s._fill_unset_zones_with_base_color = MagicMock(return_value=True) + monkeypatch.setattr(rgb_power, "translate_for_device", lambda d, c: c) + monkeypatch.setattr(rgb_power, "get_manager", lambda d: None) + return s + + +def test_perkey_write_drops_invalid_zones(monkeypatch): + """A persisted or editor-supplied map can carry zones the device never + reported (e.g. the canvas' -1 gap placeholder). They must be dropped, + not abort the whole frame with OverflowError at pack time.""" + s = _perkey_under_test(monkeypatch, choices=[7]) + + s.write({-1: 0xFF0000, 7: 0x00FF00}) # must not raise + + assert -1 not in s._value + assert s._value[7] == 0x00FF00 + + +def test_perkey_write_key_value_ignores_invalid_zone(monkeypatch): + s = _perkey_under_test(monkeypatch, choices=[7]) + + s.write_key_value(-1, 0xFF0000) + + s._send_zone_color.assert_not_called() + + +def test_perkey_read_heals_poisoned_persisted_map(monkeypatch): + """A -1 zone saved by an older/buggy build must be dropped on read so + it ages out of the persisted config on the next save.""" + s = _perkey_under_test(monkeypatch, choices=[7]) + s._value = {-1: 0xFF0000, 7: 0x00FF00} # as loaded from persister + + result = s.read() + + assert -1 not in result + assert result[7] == 0x00FF00 + + +# --- post-claim repaint ------------------------------------------------------ + + +def test_repaint_after_claim_pushes_zones_and_perkey(monkeypatch): + """The post-claim repaint pushes saved zone effects and the per-key + buffer. (The F4/MR blackout is handled structurally by keeping affected + models in onboard mode — device_quirks.rgb_claim_keeps_onboard_mode — not + by any timed repaint.)""" + from unittest.mock import MagicMock + + from logitech_receiver import settings_templates + + ctl = settings_templates.RGBControl.__new__(settings_templates.RGBControl) + zone = MagicMock() + zone.name = "rgb_zone_1" + zone._value = object() + device = MagicMock() + device.settings = [zone] + perkey = MagicMock() + perkey._value = {1: 0xFF0000} + monkeypatch.setattr(rgb_power, "perkey_has_paint", lambda d: (perkey, True)) + + ctl._repaint_after_claim(device) + zone.write.assert_called_once() + perkey.write.assert_called_once()