fix(controller): propagate close-state to all shared-controller holders

lifecycle_test.go

@@ -0,0 +1,132 @@
+package so_arm
+
+import (
+	"context"
+	"errors"
+	"testing"
+)
+
+// TestController_PostCloseReturnsSentinel verifies that every gated controller
+// method returns ErrControllerClosed when the controller's closed flag is set,
+// rather than panicking or hitting the (closed) bus. Table-driven so adding a
+// new method to the gated list without an entry here is a visible omission.
+func TestController_PostCloseReturnsSentinel(t *testing.T) {
+	ctx := context.Background()
+	cases := []struct {
+		name string
+		call func(*SafeSoArmController) error
+	}{
+		{"Ping", func(c *SafeSoArmController) error { return c.Ping(ctx) }},
+		{"SetTorqueEnable", func(c *SafeSoArmController) error { return c.SetTorqueEnable(ctx, true) }},
+		{"Stop", func(c *SafeSoArmController) error { return c.Stop(ctx) }},
+		{"MoveToJointPositions", func(c *SafeSoArmController) error {
+			return c.MoveToJointPositions(ctx, []float64{0, 0, 0, 0, 0}, 0, 0)
+		}},
+		{"MoveServosToPositions", func(c *SafeSoArmController) error {
+			return c.MoveServosToPositions(ctx, []int{1}, []float64{0}, 0, 0)
+		}},
+		{"WriteServoRegister", func(c *SafeSoArmController) error {
+			return c.WriteServoRegister(ctx, 1, "goal_position", []byte{0, 0})
+		}},
+		{"SetCalibration", func(c *SafeSoArmController) error {
+			return c.SetCalibration(SO101FullCalibration{})
+		}},
+		{"GetJointPositions", func(c *SafeSoArmController) error {
+			_, err := c.GetJointPositions(ctx)
+			return err
+		}},
+		{"GetJointPositionsForServos", func(c *SafeSoArmController) error {
+			_, err := c.GetJointPositionsForServos(ctx, []int{1})
+			return err
+		}},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			bus, _ := newMockBus(t)
+			ctrl := &SafeSoArmController{
+				bus:    bus,
+				logger: newTestLogger(t),
+			}
+			ctrl.closed.Store(true)
+
+			if err := tc.call(ctrl); !errors.Is(err, ErrControllerClosed) {
+				t.Errorf("%s after close: expected ErrControllerClosed, got %v", tc.name, err)
+			}
+		})
+	}
+}
+
+// TestRegistry_SamePointerForSamePort verifies that two callers acquiring a
+// controller for the same port receive the *same* *SafeSoArmController, so
+// that close-state propagates correctly across all consumers.
+func TestRegistry_SamePointerForSamePort(t *testing.T) {
+	registry := NewControllerRegistry()
+	port := "/dev/test-port"
+	cfg := testConfig(port)
+
+	// Inject a pre-built entry so we don't need a real bus.
+	bus, _ := newMockBus(t)
+	ctrl := &SafeSoArmController{
+		bus:    bus,
+		logger: cfg.Logger,
+	}
+	registry.entries[port] = &ControllerEntry{
+		controller:  ctrl,
+		config:      cfg,
+		calibration: DefaultSO101FullCalibration,
+		refCount:    0,
+	}
+
+	first, err := registry.GetController(port, cfg, DefaultSO101FullCalibration, false)
+	if err != nil {
+		t.Fatalf("first GetController: %v", err)
+	}
+	second, err := registry.GetController(port, cfg, DefaultSO101FullCalibration, false)
+	if err != nil {
+		t.Fatalf("second GetController: %v", err)
+	}
+
+	if first != second {
+		t.Errorf("expected same pointer for same port; got %p and %p", first, second)
+	}
+	if first != ctrl {
+		t.Errorf("expected cached controller pointer to be returned")
+	}
+}
+
+// TestRegistry_ReleaseClosesAllConsumers verifies that ReleaseController
+// at refcount zero closes the bus and sets the closed flag on the shared
+// controller, so other holders observe ErrControllerClosed on next call.
+func TestRegistry_ReleaseClosesAllConsumers(t *testing.T) {
+	registry := NewControllerRegistry()
+	port := "/dev/test-port"
+	cfg := testConfig(port)
+
+	bus, _ := newMockBus(t)
+	ctrl := &SafeSoArmController{
+		bus:    bus,
+		logger: cfg.Logger,
+	}
+	registry.entries[port] = &ControllerEntry{
+		controller:  ctrl,
+		config:      cfg,
+		calibration: DefaultSO101FullCalibration,
+		refCount:    2, // simulate arm + gripper both holding
+	}
+
+	// First release: refcount drops to 1, controller stays alive.
+	registry.ReleaseController(port)
+	if ctrl.closed.Load() {
+		t.Fatalf("controller closed prematurely at refcount > 0")
+	}
+
+	// Second release: refcount drops to 0, controller closes.
+	registry.ReleaseController(port)
+	if !ctrl.closed.Load() {
+		t.Errorf("expected controller.closed=true after final release")
+	}
+	if err := ctrl.Ping(context.Background()); !errors.Is(err, ErrControllerClosed) {
+		t.Errorf("Ping after final release: expected ErrControllerClosed, got %v", err)
+	}
+}

manager.go

@@ -2,6 +2,7 @@ package so_arm
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math"
 	"sync"
@@ -13,6 +14,11 @@ import (
 	"go.viam.com/rdk/utils"
 )
 
+// ErrControllerClosed is returned by SafeSoArmController methods after the
+// underlying bus has been closed via the registry. Callers holding a stale
+// reference should treat this as a permanent failure for that controller.
+var ErrControllerClosed = errors.New("so101: controller is closed")
+
 // isGripperServo checks if a servo ID is the gripper (servo 6)
 func isGripperServo(servoID int) bool {
 	return servoID == 6
@@ -27,6 +33,18 @@ type SafeSoArmController struct {
 	logger           logging.Logger
 	calibration      SO101FullCalibration
 	mu               sync.RWMutex
+	closed           atomic.Bool
+}
+
+// checkClosed returns ErrControllerClosed if the controller has been released.
+// checkClosed is best-effort: callers must hold a registry refcount for the
+// duration of any controller call. A concurrent ReleaseController can race the
+// unlocked Load() with an in-flight method.
+func (s *SafeSoArmController) checkClosed() error {
+	if s.closed.Load() {
+		return ErrControllerClosed
+	}
+	return nil
 }
 
 // writePositions writes goal positions to the servo group. When speed > 0 it commands
@@ -45,6 +63,9 @@ func (s *SafeSoArmController) writePositions(ctx context.Context, rawPositions f
 }
 
 func (s *SafeSoArmController) MoveToJointPositions(ctx context.Context, jointAngles []float64, speed, acc int) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -73,6 +94,9 @@ func (s *SafeSoArmController) MoveToJointPositions(ctx context.Context, jointAng
 }
 
 func (s *SafeSoArmController) MoveServosToPositions(ctx context.Context, servoIDs []int, jointAngles []float64, speed, acc int) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -106,6 +130,9 @@ func (s *SafeSoArmController) MoveServosToPositions(ctx context.Context, servoID
 }
 
 func (s *SafeSoArmController) GetJointPositions(ctx context.Context) ([]float64, error) {
+	if err := s.checkClosed(); err != nil {
+		return nil, err
+	}
 	s.mu.RLock()
 	defer s.mu.RUnlock()
 
@@ -144,6 +171,9 @@ func (s *SafeSoArmController) GetJointPositions(ctx context.Context) ([]float64,
 }
 
 func (s *SafeSoArmController) GetJointPositionsForServos(ctx context.Context, servoIDs []int) ([]float64, error) {
+	if err := s.checkClosed(); err != nil {
+		return nil, err
+	}
 	s.mu.RLock()
 	defer s.mu.RUnlock()
 
@@ -173,6 +203,9 @@ func (s *SafeSoArmController) GetJointPositionsForServos(ctx context.Context, se
 }
 
 func (s *SafeSoArmController) SetTorqueEnable(ctx context.Context, enable bool) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -189,6 +222,9 @@ func (s *SafeSoArmController) SetTorqueEnable(ctx context.Context, enable bool)
 }
 
 func (s *SafeSoArmController) Stop(ctx context.Context) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -260,13 +296,17 @@ func (s *SafeSoArmController) Close() error {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
+	s.closed.Store(true)
 	if s.bus != nil {
 		return s.bus.Close()
 	}
 	return nil
 }
 
 func (s *SafeSoArmController) Ping(ctx context.Context) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.RLock()
 	defer s.mu.RUnlock()
 
@@ -280,6 +320,9 @@ func (s *SafeSoArmController) Ping(ctx context.Context) error {
 
 // WriteServoRegister writes to a specific servo register by name
 func (s *SafeSoArmController) WriteServoRegister(ctx context.Context, servoID int, registerName string, data []byte) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
@@ -292,6 +335,9 @@ func (s *SafeSoArmController) WriteServoRegister(ctx context.Context, servoID in
 }
 
 func (s *SafeSoArmController) SetCalibration(calibration SO101FullCalibration) error {
+	if err := s.checkClosed(); err != nil {
+		return err
+	}
 	s.mu.Lock()
 	defer s.mu.Unlock()