gpio: fix data race in ButtonDriver

README.md

@@ -284,7 +284,7 @@ a shared set of drivers provided using the `gobot/drivers/gpio` package:
   - HC-SR04 Ultrasonic Ranging Module
   - HD44780 LCD controller
   - LED
-  - Makey Button
+  - Makey Button (by using driver for Button)
   - MAX7219 LED Dot Matrix
   - Motor
   - Proximity Infra Red (PIR) Motion Sensor

drivers/gpio/README.md

@@ -26,7 +26,7 @@ Gobot has a extensible system for connecting to hardware devices. The following
 - HC-SR04 Ultrasonic Ranging Module
 - HD44780 LCD controller
 - LED
-- Makey Button
+- Makey Button (by using driver for Button)
 - MAX7219 LED Dot Matrix
 - Motor
 - Proximity Infra Red (PIR) Motion Sensor

drivers/gpio/button_driver.go

@@ -8,14 +8,13 @@ import (
 
 // ButtonDriver Represents a digital Button
 type ButtonDriver struct {
-	Active       bool
-	DefaultState int
+	*Driver
+	gobot.Eventer
 	pin          string
-	name         string
+	active       bool
+	defaultState int
 	halt         chan bool
 	interval     time.Duration
-	connection   DigitalReader
-	gobot.Eventer
 }
 
 // NewButtonDriver returns a new ButtonDriver with a polling interval of
@@ -26,15 +25,16 @@ type ButtonDriver struct {
 //	time.Duration: Interval at which the ButtonDriver is polled for new information
 func NewButtonDriver(a DigitalReader, pin string, v ...time.Duration) *ButtonDriver {
 	b := &ButtonDriver{
-		name:         gobot.DefaultName("Button"),
-		connection:   a,
-		pin:          pin,
-		Active:       false,
-		DefaultState: 0,
+		Driver:       NewDriver(a.(gobot.Connection), "Button"),
 		Eventer:      gobot.NewEventer(),
+		pin:          pin,
+		active:       false,
+		defaultState: 0,
 		interval:     10 * time.Millisecond,
 		halt:         make(chan bool),
 	}
+	b.afterStart = b.initialize
+	b.beforeHalt = b.shutdown
 
 	if len(v) > 0 {
 		b.interval = v[0]
@@ -47,18 +47,39 @@ func NewButtonDriver(a DigitalReader, pin string, v ...time.Duration) *ButtonDri
 	return b
 }
 
-// Start starts the ButtonDriver and polls the state of the button at the given interval.
+// Pin returns the ButtonDrivers pin
+func (b *ButtonDriver) Pin() string { return b.pin }
+
+// Active gets the current state
+func (b *ButtonDriver) Active() bool {
+	// ensure that read and write can not interfere
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+
+	return b.active
+}
+
+// SetDefaultState for the next start.
+func (b *ButtonDriver) SetDefaultState(s int) {
+	// ensure that read and write can not interfere
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+
+	b.defaultState = s
+}
+
+// initialize the ButtonDriver and polls the state of the button at the given interval.
 //
 // Emits the Events:
 //
 //	Push int - On button push
 //	Release int - On button release
 //	Error error - On button error
-func (b *ButtonDriver) Start() (err error) {
-	state := b.DefaultState
+func (b *ButtonDriver) initialize() (err error) {
+	state := b.defaultState
 	go func() {
 		for {
-			newValue, err := b.connection.DigitalRead(b.Pin())
+			newValue, err := b.connection.(DigitalReader).DigitalRead(b.Pin())
 			if err != nil {
 				b.Publish(Error, err)
 			} else if newValue != state && newValue != -1 {
@@ -75,30 +96,21 @@ func (b *ButtonDriver) Start() (err error) {
 	return
 }
 
-// Halt stops polling the button for new information
-func (b *ButtonDriver) Halt() (err error) {
+func (b *ButtonDriver) shutdown() (err error) {
 	b.halt <- true
-	return
+	return nil
 }
 
-// Name returns the ButtonDrivers name
-func (b *ButtonDriver) Name() string { return b.name }
-
-// SetName sets the ButtonDrivers name
-func (b *ButtonDriver) SetName(n string) { b.name = n }
-
-// Pin returns the ButtonDrivers pin
-func (b *ButtonDriver) Pin() string { return b.pin }
-
-// Connection returns the ButtonDrivers Connection
-func (b *ButtonDriver) Connection() gobot.Connection { return b.connection.(gobot.Connection) }
-
 func (b *ButtonDriver) update(newValue int) {
-	if newValue != b.DefaultState {
-		b.Active = true
+	// ensure that read and write can not interfere
+	b.mutex.Lock()
+	defer b.mutex.Unlock()
+
+	if newValue != b.defaultState {
+		b.active = true
 		b.Publish(ButtonPush, newValue)
 	} else {
-		b.Active = false
+		b.active = false
 		b.Publish(ButtonRelease, newValue)
 	}
 }

drivers/gpio/button_driver_test.go

@@ -7,66 +7,94 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gobot.io/x/gobot/v2"
 )
 
 var _ gobot.Driver = (*ButtonDriver)(nil)
 
 const buttonTestDelay = 250
 
-func initTestButtonDriver() *ButtonDriver {
-	return NewButtonDriver(newGpioTestAdaptor(), "1")
+func initTestButtonDriverWithStubbedAdaptor() (*ButtonDriver, *gpioTestAdaptor) {
+	a := newGpioTestAdaptor()
+	d := NewButtonDriver(a, "1")
+	return d, a
 }
 
-func TestButtonDriverHalt(t *testing.T) {
-	d := initTestButtonDriver()
+func TestButtonHalt(t *testing.T) {
+	d, _ := initTestButtonDriverWithStubbedAdaptor()
 	go func() {
 		<-d.halt
 	}()
 	assert.NoError(t, d.Halt())
 }
 
-func TestButtonDriver(t *testing.T) {
-	d := NewButtonDriver(newGpioTestAdaptor(), "1")
-	assert.NotNil(t, d.Connection())
-
+func TestNewButtonDriver(t *testing.T) {
+	// arrange
+	a := newGpioTestAdaptor()
+	// act
+	d := NewButtonDriver(a, "1")
+	// assert
+	assert.IsType(t, &ButtonDriver{}, d)
+	assert.True(t, strings.HasPrefix(d.name, "Button"))
+	assert.Equal(t, a, d.connection)
+	assert.NotNil(t, d.afterStart)
+	assert.NotNil(t, d.beforeHalt)
+	assert.NotNil(t, d.Commander)
+	assert.NotNil(t, d.mutex)
+	assert.NotNil(t, d.Eventer)
+	assert.Equal(t, "1", d.pin)
+	assert.Equal(t, false, d.active)
+	assert.Equal(t, 0, d.defaultState)
+	assert.Equal(t, 10*time.Millisecond, d.interval)
+	assert.NotNil(t, d.halt)
+	// act & assert other interval
 	d = NewButtonDriver(newGpioTestAdaptor(), "1", 30*time.Second)
 	assert.Equal(t, 30*time.Second, d.interval)
 }
 
-func TestButtonDriverStart(t *testing.T) {
+func TestButtonStart(t *testing.T) {
+	// arrange
 	sem := make(chan bool)
-	a := newGpioTestAdaptor()
-	d := NewButtonDriver(a, "1")
+	nextVal := make(chan int, 1)
+	d, a := initTestButtonDriverWithStubbedAdaptor()
+
+	a.digitalReadFunc = func(string) (int, error) {
+		val := 1
+		var err error
+		select {
+		case val = <-nextVal:
+			if val < 0 {
+				err = errors.New("digital read error")
+			}
+			return val, err
+		default:
+			return val, err
+		}
+	}
 
 	_ = d.Once(ButtonPush, func(data interface{}) {
-		assert.True(t, d.Active)
+		assert.True(t, d.Active())
+		nextVal <- 0
 		sem <- true
 	})
 
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 1
-		return
-	}
-
+	// act
 	assert.NoError(t, d.Start())
 
+	// assert & rearrange
 	select {
 	case <-sem:
 	case <-time.After(buttonTestDelay * time.Millisecond):
 		t.Errorf("Button Event \"Push\" was not published")
 	}
 
 	_ = d.Once(ButtonRelease, func(data interface{}) {
-		assert.False(t, d.Active)
+		assert.False(t, d.Active())
+		nextVal <- -1
 		sem <- true
 	})
 
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 0
-		return
-	}
-
 	select {
 	case <-sem:
 	case <-time.After(buttonTestDelay * time.Millisecond):
@@ -77,11 +105,6 @@ func TestButtonDriverStart(t *testing.T) {
 		sem <- true
 	})
 
-	a.digitalReadFunc = func(string) (val int, err error) {
-		err = errors.New("digital read error")
-		return
-	}
-
 	select {
 	case <-sem:
 	case <-time.After(buttonTestDelay * time.Millisecond):
@@ -93,11 +116,7 @@ func TestButtonDriverStart(t *testing.T) {
 	})
 
 	d.halt <- true
-
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 1
-		return
-	}
+	nextVal <- 1
 
 	select {
 	case <-sem:
@@ -106,22 +125,32 @@ func TestButtonDriverStart(t *testing.T) {
 	}
 }
 
-func TestButtonDriverDefaultState(t *testing.T) {
+func TestButtonSetDefaultState(t *testing.T) {
+	// arrange
 	sem := make(chan bool)
-	a := newGpioTestAdaptor()
-	d := NewButtonDriver(a, "1")
-	d.DefaultState = 1
-
+	nextVal := make(chan int, 1)
+	d, a := initTestButtonDriverWithStubbedAdaptor()
+
+	a.digitalReadFunc = func(string) (int, error) {
+		val := 0
+		select {
+		case val = <-nextVal:
+			return val, nil
+		default:
+			return val, nil
+		}
+	}
 	_ = d.Once(ButtonPush, func(data interface{}) {
-		assert.True(t, d.Active)
+		assert.True(t, d.Active())
+		nextVal <- 1
 		sem <- true
 	})
 
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 0
-		return
-	}
+	// act
+	d.SetDefaultState(1)
 
+	// assert & rearrange
+	require.Equal(t, 1, d.defaultState)
 	assert.NoError(t, d.Start())
 
 	select {
@@ -131,29 +160,49 @@ func TestButtonDriverDefaultState(t *testing.T) {
 	}
 
 	_ = d.Once(ButtonRelease, func(data interface{}) {
-		assert.False(t, d.Active)
+		assert.False(t, d.Active())
+
 		sem <- true
 	})
 
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 1
-		return
-	}
-
 	select {
 	case <-sem:
 	case <-time.After(buttonTestDelay * time.Millisecond):
 		t.Errorf("Button Event \"Release\" was not published")
 	}
 }
 
-func TestButtonDriverDefaultName(t *testing.T) {
-	g := initTestButtonDriver()
-	assert.True(t, strings.HasPrefix(g.Name(), "Button"))
+func TestButtonPin(t *testing.T) {
+	tests := map[string]struct {
+		want string
+	}{
+		"10": {want: "10"},
+		"36": {want: "36"},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			// arrange
+			d := ButtonDriver{pin: name}
+			// act & assert
+			assert.Equal(t, tc.want, d.Pin())
+		})
+	}
 }
 
-func TestButtonDriverSetName(t *testing.T) {
-	g := initTestButtonDriver()
-	g.SetName("mybot")
-	assert.Equal(t, "mybot", g.Name())
+func TestButtonActive(t *testing.T) {
+	tests := map[string]struct {
+		want bool
+	}{
+		"active_true":  {want: true},
+		"active_false": {want: false},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			// arrange
+			d := ButtonDriver{Driver: NewDriver(nil, "Button")} // just for mutex
+			d.active = tc.want
+			// act & assert
+			assert.Equal(t, tc.want, d.Active())
+		})
+	}
 }