spi(all): ensure halt is idempotent (#1173)

gen2thomas · web-flow · commit 7bfec7cdd49b · 2025-10-04T10:49:25.000+02:00
diff --git a/drivers/spi/apa102.go b/drivers/spi/apa102.go
@@ -86,5 +86,5 @@ func (d *APA102Driver) Draw() error {
 		tx[i] = 0xff
 	}
 
-	return d.connection.WriteBytes(tx)
+	return d.writeBytes(tx)
 }
diff --git a/drivers/spi/apa102_test.go b/drivers/spi/apa102_test.go
@@ -34,6 +34,15 @@ func TestNewAPA102Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "APA102"))
 }
 
+func TestAPA102Halt(t *testing.T) {
+	// arrange
+	d := NewAPA102Driver(newSpiTestAdaptor(), 10, 31)
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestDriverLEDs(t *testing.T) {
 	d := initTestAPA102DriverWithStubbedAdaptor()
 
diff --git a/drivers/spi/mcp3002.go b/drivers/spi/mcp3002.go
@@ -48,7 +48,7 @@ func (d *MCP3002Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 2)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil {
+	if err := d.readCommandData(tx, rx); err != nil {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3002_test.go b/drivers/spi/mcp3002_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3002Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3002"))
 }
 
+func TestMCP3002Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3002Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3002Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mcp3004.go b/drivers/spi/mcp3004.go
@@ -49,7 +49,7 @@ func (d *MCP3004Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 3)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil || len(rx) != 3 {
+	if err := d.readCommandData(tx, rx); err != nil || len(rx) != 3 {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3004_test.go b/drivers/spi/mcp3004_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3004Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3004"))
 }
 
+func TestMCP3004Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3004Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3004Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mcp3008.go b/drivers/spi/mcp3008.go
@@ -49,7 +49,7 @@ func (d *MCP3008Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 3)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil || len(rx) != 3 {
+	if err := d.readCommandData(tx, rx); err != nil || len(rx) != 3 {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3008_test.go b/drivers/spi/mcp3008_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3008Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3008"))
 }
 
+func TestMCP3008Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3008Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3008Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mcp3202.go b/drivers/spi/mcp3202.go
@@ -51,7 +51,7 @@ func (d *MCP3202Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 3)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil || len(rx) != 3 {
+	if err := d.readCommandData(tx, rx); err != nil || len(rx) != 3 {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3202_test.go b/drivers/spi/mcp3202_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3202Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3202"))
 }
 
+func TestMCP3202Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3202Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3202Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mcp3204.go b/drivers/spi/mcp3204.go
@@ -51,7 +51,7 @@ func (d *MCP3204Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 3)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil || len(rx) != 3 {
+	if err := d.readCommandData(tx, rx); err != nil || len(rx) != 3 {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3204_test.go b/drivers/spi/mcp3204_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3204Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3204"))
 }
 
+func TestMCP3204Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3204Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3204Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mcp3208.go b/drivers/spi/mcp3208.go
@@ -51,7 +51,7 @@ func (d *MCP3208Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 3)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil || len(rx) != 3 {
+	if err := d.readCommandData(tx, rx); err != nil || len(rx) != 3 {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3208_test.go b/drivers/spi/mcp3208_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3208Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3208"))
 }
 
+func TestMCP3208Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3208Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3208Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mcp3304.go b/drivers/spi/mcp3304.go
@@ -51,7 +51,7 @@ func (d *MCP3304Driver) Read(channel int) (int, error) {
 
 	rx := make([]byte, 3)
 
-	if err := d.connection.ReadCommandData(tx, rx); err != nil || len(rx) != 3 {
+	if err := d.readCommandData(tx, rx); err != nil || len(rx) != 3 {
 		return 0, err
 	}
 
diff --git a/drivers/spi/mcp3304_test.go b/drivers/spi/mcp3304_test.go
@@ -38,6 +38,15 @@ func TestNewMCP3304Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MCP3304"))
 }
 
+func TestMCP3304Halt(t *testing.T) {
+	// arrange
+	d := NewMCP3304Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMCP3304Read(t *testing.T) {
 	tests := map[string]struct {
 		chanNum     int
diff --git a/drivers/spi/mfrc522_driver_test.go b/drivers/spi/mfrc522_driver_test.go
@@ -35,6 +35,15 @@ func TestNewMFRC522Driver(t *testing.T) {
 	assert.True(t, strings.HasPrefix(d.Name(), "MFRC522"))
 }
 
+func TestMFRC522Halt(t *testing.T) {
+	// arrange
+	d := NewMFRC522Driver(newSpiTestAdaptor())
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
+	require.NoError(t, d.Halt())
+}
+
 func TestMFRC522WriteByteData(t *testing.T) {
 	// arrange
 	d, a := initTestMFRC522DriverWithStubbedAdaptor()
diff --git a/drivers/spi/spi_connection.go b/drivers/spi/spi_connection.go
@@ -24,6 +24,14 @@ func NewConnection(spiSystem gobot.SpiSystemDevicer) *spiConnection {
 	return &spiConnection{spiSystem: spiSystem}
 }
 
+// Close connection to underlying SPI device.
+func (c *spiConnection) Close() error {
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
+	return c.spiSystem.Close()
+}
+
 // ReadCommandData uses the SPI device TX to send/receive data. Implements gobot.SpiOperations
 // On write command, the first byte normally contains the address and mode.
 // On read data, the return value is most likely one byte behind the command.
@@ -35,14 +43,6 @@ func (c *spiConnection) ReadCommandData(command []byte, data []byte) error {
 	return c.txRxAndCheckReadLength(command, data)
 }
 
-// Close connection to underlying SPI device.
-func (c *spiConnection) Close() error {
-	c.mutex.Lock()
-	defer c.mutex.Unlock()
-
-	return c.spiSystem.Close()
-}
-
 // ReadByteData reads a byte from the given register of SPI device. Implements gobot.BusOperations.
 func (c *spiConnection) ReadByteData(reg uint8) (uint8, error) {
 	c.mutex.Lock()
diff --git a/drivers/spi/spi_driver.go b/drivers/spi/spi_driver.go
@@ -1,6 +1,7 @@
 package spi
 
 import (
+	"fmt"
 	"log"
 	"sync"
 
@@ -103,13 +104,13 @@ func NewDriver(a Connector, name string, options ...func(Config)) *Driver {
 	return d
 }
 
-// Name returns the name of the device.
+// Name returns the name of the SPI device.
 func (d *Driver) Name() string { return d.name }
 
-// SetName sets the name of the device.
+// SetName sets the name of the SPI device.
 func (d *Driver) SetName(n string) { d.name = n }
 
-// Connection returns the Connection of the device.
+// Connection returns the gobot connection of the SPI device.
 func (d *Driver) Connection() gobot.Connection {
 	if conn, ok := d.connector.(gobot.Connection); ok {
 		return conn
@@ -147,7 +148,68 @@ func (d *Driver) Halt() error {
 		return err
 	}
 
+	d.connection = nil
+
 	// currently there is nothing to do here for the driver, the connection is cached on adaptor side
 	// and will be closed on adaptor Finalize()
 	return nil
 }
+
+func (d *Driver) readCommandData(command []byte, data []byte) error {
+	if d.connection == nil {
+		return fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.ReadCommandData(command, data)
+}
+
+//nolint:unused // ok for now
+func (d *Driver) readByteData(reg uint8) (uint8, error) {
+	if d.connection == nil {
+		return 0, fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.ReadByteData(reg)
+}
+
+//nolint:unused // ok for now
+func (d *Driver) readBlockData(reg uint8, data []byte) error {
+	if d.connection == nil {
+		return fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.ReadBlockData(reg, data)
+}
+
+func (d *Driver) writeByte(val byte) error {
+	if d.connection == nil {
+		return fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.WriteByte(val)
+}
+
+//nolint:unused // ok for now
+func (d *Driver) writeByteData(reg byte, data byte) error {
+	if d.connection == nil {
+		return fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.WriteByteData(reg, data)
+}
+
+func (d *Driver) writeBlockData(reg byte, data []byte) error {
+	if d.connection == nil {
+		return fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.WriteBlockData(reg, data)
+}
+
+func (d *Driver) writeBytes(data []byte) error {
+	if d.connection == nil {
+		return fmt.Errorf("spi driver not started")
+	}
+
+	return d.connection.WriteBytes(data)
+}
diff --git a/drivers/spi/spi_driver_test.go b/drivers/spi/spi_driver_test.go
@@ -36,7 +36,11 @@ func TestStart(t *testing.T) {
 }
 
 func TestHalt(t *testing.T) {
-	d, _ := initTestDriverWithStubbedAdaptor()
+	// arrange
+	d := NewDriver(newSpiTestAdaptor(), "SPI_BASIC")
+	// act, assert
+	require.NoError(t, d.Halt()) // must be idempotent
+	require.NoError(t, d.Start())
 	require.NoError(t, d.Halt())
 }
 
diff --git a/drivers/spi/ssd1306_driver.go b/drivers/spi/ssd1306_driver.go
diff --git a/drivers/spi/ssd1306_driver_test.go b/drivers/spi/ssd1306_driver_test.go

Original file line number	Diff line number	Diff line change
`@@ -86,5 +86,5 @@ func (d *APA102Driver) Draw() error {`
`86`	`86`	`tx[i] = 0xff`
`87`	`87`	`}`
`88`	`88`
`89`		`- return d.connection.WriteBytes(tx)`
	`89`	`+ return d.writeBytes(tx)`
`90`	`90`	`}`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ func (d *MCP3002Driver) Read(channel int) (int, error) {`
`48`	`48`
`49`	`49`	`rx := make([]byte, 2)`
`50`	`50`
`51`		`- if err := d.connection.ReadCommandData(tx, rx); err != nil {`
	`51`	`+ if err := d.readCommandData(tx, rx); err != nil {`
`52`	`52`	`return 0, err`
`53`	`53`	`}`
`54`	`54`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ func (d *MCP3004Driver) Read(channel int) (int, error) {`
`49`	`49`
`50`	`50`	`rx := make([]byte, 3)`
`51`	`51`
`52`		`- if err := d.connection.ReadCommandData(tx, rx); err != nil \|\| len(rx) != 3 {`
	`52`	`+ if err := d.readCommandData(tx, rx); err != nil \|\| len(rx) != 3 {`
`53`	`53`	`return 0, err`
`54`	`54`	`}`
`55`	`55`
Original file line number	Diff line number	Diff line change
`@@ -49,7 +49,7 @@ func (d *MCP3008Driver) Read(channel int) (int, error) {`
`49`	`49`
`50`	`50`	`rx := make([]byte, 3)`
`51`	`51`
`52`		`- if err := d.connection.ReadCommandData(tx, rx); err != nil \|\| len(rx) != 3 {`
	`52`	`+ if err := d.readCommandData(tx, rx); err != nil \|\| len(rx) != 3 {`
`53`	`53`	`return 0, err`
`54`	`54`	`}`
`55`	`55`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ func (d *MCP3202Driver) Read(channel int) (int, error) {`
`51`	`51`
`52`	`52`	`rx := make([]byte, 3)`
`53`	`53`
`54`		`- if err := d.connection.ReadCommandData(tx, rx); err != nil \|\| len(rx) != 3 {`
	`54`	`+ if err := d.readCommandData(tx, rx); err != nil \|\| len(rx) != 3 {`
`55`	`55`	`return 0, err`
`56`	`56`	`}`
`57`	`57`