gpio: fix data race in ButtonDriver (#1027)

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

@@ -3,10 +3,12 @@ package gpio
 import (
 	"errors"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gobot.io/x/gobot/v2"
 )
 
@@ -14,43 +16,66 @@ 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()
-	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):
@@ -58,15 +83,11 @@ func TestButtonDriverStart(t *testing.T) {
 	}
 
 	_ = 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 +98,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 +109,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 +118,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,15 +153,11 @@ 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):
@@ -147,13 +165,56 @@ func TestButtonDriverDefaultState(t *testing.T) {
 	}
 }
 
-func TestButtonDriverDefaultName(t *testing.T) {
-	g := initTestButtonDriver()
-	assert.True(t, strings.HasPrefix(g.Name(), "Button"))
+func TestButtonHalt(t *testing.T) {
+	// arrange
+	d, _ := initTestButtonDriverWithStubbedAdaptor()
+	const timeout = 10 * time.Microsecond
+	wg := sync.WaitGroup{}
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		select {
+		case <-d.halt: // wait until halt was set to the channel
+		case <-time.After(timeout): // otherwise run into the timeout
+			t.Errorf("halt was not received within %s", timeout)
+		}
+	}()
+	// act & assert
+	assert.NoError(t, d.Halt())
+	wg.Wait() // wait until the go function was really finished
 }
 
-func TestButtonDriverSetName(t *testing.T) {
-	g := initTestButtonDriver()
-	g.SetName("mybot")
-	assert.Equal(t, "mybot", g.Name())
+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 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())
+		})
+	}
 }

drivers/gpio/makey_button_driver.go

@@ -1,101 +0,0 @@
-package gpio
-
-import (
-	"time"
-
-	"gobot.io/x/gobot/v2"
-)
-
-// MakeyButtonDriver Represents a Makey Button
-type MakeyButtonDriver struct {
-	name       string
-	pin        string
-	halt       chan bool
-	connection DigitalReader
-	Active     bool
-	interval   time.Duration
-	gobot.Eventer
-}
-
-// NewMakeyButtonDriver returns a new MakeyButtonDriver with a polling interval of
-// 10 Milliseconds given a DigitalReader and pin.
-//
-// Optionally accepts:
-//
-//	time.Duration: Interval at which the ButtonDriver is polled for new information
-func NewMakeyButtonDriver(a DigitalReader, pin string, v ...time.Duration) *MakeyButtonDriver {
-	m := &MakeyButtonDriver{
-		name:       gobot.DefaultName("MakeyButton"),
-		connection: a,
-		pin:        pin,
-		Active:     false,
-		Eventer:    gobot.NewEventer(),
-		interval:   10 * time.Millisecond,
-		halt:       make(chan bool),
-	}
-
-	if len(v) > 0 {
-		m.interval = v[0]
-	}
-
-	m.AddEvent(Error)
-	m.AddEvent(ButtonPush)
-	m.AddEvent(ButtonRelease)
-
-	return m
-}
-
-// Name returns the MakeyButtonDrivers name
-func (b *MakeyButtonDriver) Name() string { return b.name }
-
-// SetName sets the MakeyButtonDrivers name
-func (b *MakeyButtonDriver) SetName(n string) { b.name = n }
-
-// Pin returns the MakeyButtonDrivers pin
-func (b *MakeyButtonDriver) Pin() string { return b.pin }
-
-// Connection returns the MakeyButtonDrivers Connection
-func (b *MakeyButtonDriver) Connection() gobot.Connection { return b.connection.(gobot.Connection) }
-
-// Start starts the MakeyButtonDriver 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 *MakeyButtonDriver) Start() (err error) {
-	state := 1
-	go func() {
-		timer := time.NewTimer(b.interval)
-		timer.Stop()
-		for {
-			newValue, err := b.connection.DigitalRead(b.Pin())
-			if err != nil {
-				b.Publish(Error, err)
-			} else if newValue != state && newValue != -1 {
-				state = newValue
-				if newValue == 0 {
-					b.Active = true
-					b.Publish(ButtonPush, newValue)
-				} else {
-					b.Active = false
-					b.Publish(ButtonRelease, newValue)
-				}
-			}
-			timer.Reset(b.interval)
-			select {
-			case <-timer.C:
-			case <-b.halt:
-				return
-			}
-		}
-	}()
-	return
-}
-
-// Halt stops polling the makey button for new information
-func (b *MakeyButtonDriver) Halt() (err error) {
-	b.halt <- true
-	return
-}

drivers/gpio/makey_button_driver_test.go

@@ -1,117 +0,0 @@
-package gpio
-
-import (
-	"errors"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/assert"
-	"gobot.io/x/gobot/v2"
-)
-
-var _ gobot.Driver = (*MakeyButtonDriver)(nil)
-
-const makeyTestDelay = 250
-
-func initTestMakeyButtonDriver() *MakeyButtonDriver {
-	return NewMakeyButtonDriver(newGpioTestAdaptor(), "1")
-}
-
-func TestMakeyButtonDriverHalt(t *testing.T) {
-	d := initTestMakeyButtonDriver()
-	done := make(chan struct{})
-	go func() {
-		<-d.halt
-		close(done)
-	}()
-	assert.NoError(t, d.Halt())
-	select {
-	case <-done:
-	case <-time.After(makeyTestDelay * time.Millisecond):
-		t.Errorf("MakeyButton was not halted")
-	}
-}
-
-func TestMakeyButtonDriver(t *testing.T) {
-	d := initTestMakeyButtonDriver()
-	assert.Equal(t, "1", d.Pin())
-	assert.NotNil(t, d.Connection())
-	assert.Equal(t, 10*time.Millisecond, d.interval)
-
-	d = NewMakeyButtonDriver(newGpioTestAdaptor(), "1", 30*time.Second)
-	assert.Equal(t, 30*time.Second, d.interval)
-}
-
-func TestMakeyButtonDriverStart(t *testing.T) {
-	sem := make(chan bool)
-	a := newGpioTestAdaptor()
-	d := NewMakeyButtonDriver(a, "1")
-
-	assert.NoError(t, d.Start())
-
-	_ = d.Once(ButtonPush, func(data interface{}) {
-		assert.True(t, d.Active)
-		sem <- true
-	})
-
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 0
-		return
-	}
-
-	select {
-	case <-sem:
-	case <-time.After(makeyTestDelay * time.Millisecond):
-		t.Errorf("MakeyButton Event \"Push\" was not published")
-	}
-
-	_ = d.Once(ButtonRelease, func(data interface{}) {
-		assert.False(t, d.Active)
-		sem <- true
-	})
-
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 1
-		return
-	}
-
-	select {
-	case <-sem:
-	case <-time.After(makeyTestDelay * time.Millisecond):
-		t.Errorf("MakeyButton Event \"Release\" was not published")
-	}
-
-	_ = d.Once(Error, func(data interface{}) {
-		assert.Equal(t, "digital read error", data.(error).Error())
-		sem <- true
-	})
-
-	a.digitalReadFunc = func(string) (val int, err error) {
-		err = errors.New("digital read error")
-		return
-	}
-
-	select {
-	case <-sem:
-	case <-time.After(makeyTestDelay * time.Millisecond):
-		t.Errorf("MakeyButton Event \"Error\" was not published")
-	}
-
-	// send a halt message
-	_ = d.Once(ButtonRelease, func(data interface{}) {
-		sem <- true
-	})
-
-	a.digitalReadFunc = func(string) (val int, err error) {
-		val = 1
-		return
-	}
-
-	d.halt <- true
-
-	select {
-	case <-sem:
-		t.Errorf("MakeyButton Event should not have been published")
-	case <-time.After(makeyTestDelay * time.Millisecond):
-	}
-}

examples/beaglebone_makey_button.go

@@ -1,38 +0,0 @@
-//go:build example
-// +build example
-
-//
-// Do not build by default.
-
-package main
-
-import (
-	"fmt"
-
-	"gobot.io/x/gobot/v2"
-	"gobot.io/x/gobot/v2/drivers/gpio"
-	"gobot.io/x/gobot/v2/platforms/beaglebone"
-)
-
-func main() {
-	beagleboneAdaptor := beaglebone.NewAdaptor()
-	button := gpio.NewMakeyButtonDriver(beagleboneAdaptor, "P8_09")
-
-	work := func() {
-		button.On(gpio.ButtonPush, func(data interface{}) {
-			fmt.Println("button pressed")
-		})
-
-		button.On(gpio.ButtonRelease, func(data interface{}) {
-			fmt.Println("button released")
-		})
-	}
-
-	robot := gobot.NewRobot("makeyBot",
-		[]gobot.Connection{beagleboneAdaptor},
-		[]gobot.Device{button},
-		work,
-	)
-
-	robot.Start()
-}

examples/firmata_makey_button.go

@@ -1,46 +0,0 @@
-//go:build example
-// +build example
-
-//
-// Do not build by default.
-
-/*
- How to run
- Pass serial port to use as the first param:
-
-	go run examples/firmata_makey_button.go /dev/ttyACM0
-*/
-
-package main
-
-import (
-	"os"
-
-	"gobot.io/x/gobot/v2"
-	"gobot.io/x/gobot/v2/drivers/gpio"
-	"gobot.io/x/gobot/v2/platforms/firmata"
-)
-
-func main() {
-	firmataAdaptor := firmata.NewAdaptor(os.Args[1])
-	button := gpio.NewMakeyButtonDriver(firmataAdaptor, "2")
-	led := gpio.NewLedDriver(firmataAdaptor, "13")
-
-	work := func() {
-		button.On(gpio.ButtonPush, func(data interface{}) {
-			led.On()
-		})
-
-		button.On(gpio.ButtonRelease, func(data interface{}) {
-			led.Off()
-		})
-	}
-
-	robot := gobot.NewRobot("makeyBot",
-		[]gobot.Connection{firmataAdaptor},
-		[]gobot.Device{button, led},
-		work,
-	)
-
-	robot.Start()
-}