[ryoku] hub: key identical SMBus lighting devices by slot

ryoku/hub/CHANGELOG.md

@@ -3,6 +3,12 @@
 ## Unreleased
 
 ### Fixed
+- **Identical RAM sticks are separate lighting devices.** SMBus devices report
+  no serial, so every matching stick shared one entry and Hand back restored
+  only the first. Each stick is now keyed by its adapter and address (the
+  `/dev/i2c-N` number is ignored, since it moves between boots), the old shared
+  entry is copied to every stick on the next scan, and identical sticks list in
+  a stable order (`backend/lighting.go`).
 - Shortcut names, hints, categories and "not on" reasons follow the interface
   language in Keybinds and in Kit. The shortcut catalogue now enters the
   translation catalog, and the Keybinds page translates what it shows

ryoku/hub/backend/lighting.go

@@ -137,16 +137,63 @@ func saveLighting(s lightingState) error {
 }
 
 // deviceKey: an identity that survives a reboot. The USB path and the SDK index
-// both move, so the name plus the serial is the stable pair. Two identical
-// devices with no serial share one entry, which is honest: nothing about them
-// distinguishes one from the other between sessions.
+// both move, so the name plus the serial is the stable pair. An SMBus device
+// (RAM sticks, some boards and graphics cards) reports no serial, but its
+// adapter and address are fixed by the slot it sits in, so that pair tells
+// identical sticks apart. Two identical devices with neither share one entry,
+// which is honest: nothing about them distinguishes one from the other between
+// sessions.
 func deviceKey(d orgbDevice) string {
 	if d.Serial != "" {
 		return d.Name + "#" + d.Serial
 	}
+	if slot, ok := smbusSlot(d.Location); ok {
+		return d.Name + "#" + slot
+	}
 	return d.Name
 }
 
+var (
+	smbusLocation = regexp.MustCompile(`^I2C: (.*), address (0x[0-9A-Fa-f]+)$`)
+	// the bus number follows driver load order, so it can move between boots
+	// while the adapter name and the address stay put.
+	smbusBusNode = regexp.MustCompile(`\s*\(?/dev/i2c-\d+\)?`)
+)
+
+// smbusSlot reads "smbus:<adapter>@<address>" out of an OpenRGB I2C location.
+func smbusSlot(location string) (string, bool) {
+	m := smbusLocation.FindStringSubmatch(location)
+	if m == nil {
+		return "", false
+	}
+	adapter := strings.TrimSpace(smbusBusNode.ReplaceAllString(m[1], ""))
+	return "smbus:" + adapter + "@" + strings.ToLower(m[2]), true
+}
+
+// legacyKeys: the keys this device's settings may still be stored under. An
+// SMBus device was once keyed by its bare name, shared by every identical stick.
+func legacyKeys(d orgbDevice) []string {
+	keys := d.Aliases
+	if d.Serial == "" {
+		if _, ok := smbusSlot(d.Location); ok {
+			keys = append(append([]string(nil), keys...), d.Name)
+		}
+	}
+	return keys
+}
+
+func cloneSettings(s *lightingSettings) *lightingSettings {
+	c := *s
+	c.Colors = append([]string(nil), s.Colors...)
+	if s.ZoneColors != nil {
+		c.ZoneColors = make(map[string]string, len(s.ZoneColors))
+		for z, hex := range s.ZoneColors {
+			c.ZoneColors[z] = hex
+		}
+	}
+	return &c
+}
+
 // ── the wire shape the Hub renders ──────────────────────────────────────────
 
 // lightMode = one mode reduced to the knobs it actually offers, so the UI can
@@ -321,28 +368,32 @@ func lightingScan() lightReport {
 func rememberFirstSight(live []orgbDevice) lightingState {
 	st := loadLighting()
 	dirty := false
+	// a legacy entry is dropped only after every device it stood for has taken
+	// its own copy, so identical sticks that shared one all keep the look.
+	claimed := map[string]bool{}
 	for _, d := range live {
 		key := deviceKey(d)
 		s := st.Devices[key]
-		for _, alias := range d.Aliases {
+		for _, alias := range legacyKeys(d) {
 			old := st.Devices[alias]
-			if old == nil {
+			if old == nil || alias == key {
 				continue
 			}
 			if s == nil || firstSightDefaults(s) {
-				old.Name = d.Name
-				st.Devices[key] = old
-				s = old
+				moved := cloneSettings(old)
+				moved.Name = d.Name
+				st.Devices[key] = moved
+				s = moved
 				if d.Provider == auraProvider {
-					if old.Effect != "" {
-						old.Mode = auraModeForEffect(old.Effect, d)
-						old.Effect = ""
+					if moved.Effect != "" {
+						moved.Mode = auraModeForEffect(moved.Effect, d)
+						moved.Effect = ""
 					} else {
-						old.Mode = auraModeForLegacyMode(old.Mode, d)
+						moved.Mode = auraModeForLegacyMode(moved.Mode, d)
 					}
 				}
 			}
-			delete(st.Devices, alias)
+			claimed[alias] = true
 			dirty = true
 		}
 		if s == nil {
@@ -355,6 +406,11 @@ func rememberFirstSight(live []orgbDevice) lightingState {
 			dirty = true
 		}
 	}
+	for alias := range claimed {
+		if !liveKey(live, alias) {
+			delete(st.Devices, alias)
+		}
+	}
 	if dirty {
 		_ = saveLighting(st)
 	}
@@ -482,13 +538,16 @@ func savedView(key string, s *lightingSettings) lightDevice {
 }
 
 // sortDevices: online devices first, then by name, so the list a user sees does
-// not reshuffle between scans.
+// not reshuffle between scans. Identical sticks fall back to their key.
 func sortDevices(ds []lightDevice) {
 	sort.SliceStable(ds, func(i, j int) bool {
 		if ds[i].Online != ds[j].Online {
 			return ds[i].Online
 		}
-		return ds[i].Name < ds[j].Name
+		if ds[i].Name != ds[j].Name {
+			return ds[i].Name < ds[j].Name
+		}
+		return ds[i].Key < ds[j].Key
 	})
 }
 
@@ -1023,6 +1082,11 @@ func findLive(live []orgbDevice, key string) (orgbDevice, bool) {
 	return orgbDevice{}, false
 }
 
+func liveKey(live []orgbDevice, key string) bool {
+	_, ok := findLive(live, key)
+	return ok
+}
+
 // ── applying one device ─────────────────────────────────────────────────────
 
 func applyOne(o *orgbConn, live []orgbDevice, key string, s *lightingSettings, accent string) error {

ryoku/hub/backend/lighting_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"os"
 	"path/filepath"
@@ -289,6 +290,104 @@ func TestDeviceKeyIgnoresThePort(t *testing.T) {
 	}
 }
 
+func dramStick(address uint32, bus int, slot string) orgbDevice {
+	return orgbDevice{
+		Name: "ENE DRAM", Vendor: "ENE", Type: "DRAM", Address: address,
+		Location: fmt.Sprintf("I2C: SMBus PIIX4 adapter port 0 at 0b00 (/dev/i2c-%d), address %s", bus, slot),
+		Modes: []orgbMode{
+			{Name: "Direct", Value: 0, Flags: modeHasPerLEDColor},
+			{Name: "Breathing", Value: 2, Flags: modeHasSpeed | modeHasPerLEDColor},
+			{Name: "Rainbow", Value: 5, Flags: modeHasSpeed},
+		},
+	}
+}
+
+func TestDeviceKeySeparatesIdenticalSMBusSticks(t *testing.T) {
+	first, second := dramStick(0, 8, "0x71"), dramStick(1, 8, "0x73")
+	if deviceKey(first) == deviceKey(second) {
+		t.Fatalf("two sticks share %q", deviceKey(first))
+	}
+	if moved := dramStick(0, 9, "0x71"); deviceKey(moved) != deviceKey(first) {
+		t.Fatalf("a renumbered i2c bus moved the stick from %q to %q", deviceKey(first), deviceKey(moved))
+	}
+	if got, want := deviceKey(first), "ENE DRAM#smbus:SMBus PIIX4 adapter port 0 at 0b00@0x71"; got != want {
+		t.Fatalf("key = %q, want %q", got, want)
+	}
+	if got := deviceKey(orgbDevice{Name: "Pad", Location: "HID: /dev/hidraw4"}); got != "Pad" {
+		t.Fatalf("a serial-less USB device keyed as %q, want its bare name", got)
+	}
+}
+
+func TestFirstSightGivesEverySharedStickItsOwnCopy(t *testing.T) {
+	setupFollowTest(t)
+	if err := saveLighting(lightingState{Enabled: true, Devices: map[string]*lightingSettings{
+		"ENE DRAM": {
+			Name: "ENE DRAM", Managed: true, Mode: "Breathing", Effect: "pulse", Source: "accent",
+			Colors: []string{"#E79389"}, Brightness: -1, Speed: 40, Restore: "Rainbow",
+		},
+	}}); err != nil {
+		t.Fatal(err)
+	}
+	first, second := dramStick(0, 8, "0x71"), dramStick(1, 8, "0x73")
+
+	st := rememberFirstSight([]orgbDevice{first, second})
+	if _, ok := st.Devices["ENE DRAM"]; ok {
+		t.Fatal("the shared entry outlived the migration")
+	}
+	a, b := st.Devices[deviceKey(first)], st.Devices[deviceKey(second)]
+	for _, s := range []*lightingSettings{a, b} {
+		if s == nil || !s.Managed || s.Mode != "Breathing" || s.Effect != "pulse" ||
+			s.Speed != 40 || s.Restore != "Rainbow" || len(s.Colors) != 1 {
+			t.Fatalf("stick settings = %+v, want the shared look", s)
+		}
+	}
+	a.Colors[0] = "#000000"
+	if b.Colors[0] != "#E79389" {
+		t.Fatal("the sticks still share one settings value")
+	}
+	if saved := loadLighting(); len(saved.Devices) != 2 {
+		t.Fatalf("saved devices = %d, want 2", len(saved.Devices))
+	}
+}
+
+func TestReleaseRestoresOnlyTheChosenStick(t *testing.T) {
+	setupFollowTest(t)
+	first, second := dramStick(4, 8, "0x71"), dramStick(7, 8, "0x73")
+	managed := func() *lightingSettings {
+		return &lightingSettings{Name: "ENE DRAM", Managed: true, Mode: "Breathing",
+			Source: "accent", Brightness: -1, Speed: -1, Restore: "Rainbow"}
+	}
+	if err := saveLighting(lightingState{Enabled: true, Devices: map[string]*lightingSettings{
+		deviceKey(first): managed(), deviceKey(second): managed(),
+	}}); err != nil {
+		t.Fatal(err)
+	}
+
+	oldAccess := lightingAccess
+	t.Cleanup(func() { lightingAccess = oldAccess })
+	packet := captureWrite(t, 5, func(conn *orgbConn) error {
+		lightingAccess = func(_ bool, _ bool, _ lightingProviders, fn func(*orgbConn, []orgbDevice) error) error {
+			return fn(conn, []orgbDevice{first, second})
+		}
+		captureStdout(t, func() {
+			if err := lightingRelease(deviceKey(second)); err != nil {
+				t.Fatal(err)
+			}
+		})
+		return nil
+	})
+	if packet.id != pktUpdateMode || packet.device != second.Address {
+		t.Fatalf("restore went to device %d (packet %d), want %d", packet.device, packet.id, second.Address)
+	}
+	st := loadLighting()
+	if st.Devices[deviceKey(second)].Managed {
+		t.Fatal("the released stick is still managed")
+	}
+	if !st.Devices[deviceKey(first)].Managed {
+		t.Fatal("releasing one stick released the other")
+	}
+}
+
 func TestStateDefaultsToOffAndSurvivesGarbage(t *testing.T) {
 	dir := t.TempDir()
 	t.Setenv("XDG_CONFIG_HOME", dir)