gpio: fix data race in PIRMotionDriver (#1028)

drivers/gpio/button_driver.go

@@ -75,7 +75,7 @@ func (b *ButtonDriver) SetDefaultState(s int) {
 //	Push int - On button push
 //	Release int - On button release
 //	Error error - On button error
-func (b *ButtonDriver) initialize() (err error) {
+func (b *ButtonDriver) initialize() error {
 	state := b.defaultState
 	go func() {
 		for {
@@ -93,10 +93,10 @@ func (b *ButtonDriver) initialize() (err error) {
 			}
 		}
 	}()
-	return
+	return nil
 }
 
-func (b *ButtonDriver) shutdown() (err error) {
+func (b *ButtonDriver) shutdown() error {
 	b.halt <- true
 	return nil
 }

drivers/gpio/button_driver_test.go

@@ -1,7 +1,7 @@
 package gpio
 
 import (
-	"errors"
+	"fmt"
 	"strings"
 	"sync"
 	"testing"
@@ -58,7 +58,7 @@ func TestButtonStart(t *testing.T) {
 		select {
 		case val = <-nextVal:
 			if val < 0 {
-				err = errors.New("digital read error")
+				err = fmt.Errorf("digital read error")
 			}
 			return val, err
 		default:
@@ -73,9 +73,11 @@ func TestButtonStart(t *testing.T) {
 	})
 
 	// act
-	assert.NoError(t, d.Start())
+	err := d.Start()
 
 	// assert & rearrange
+	assert.NoError(t, err)
+
 	select {
 	case <-sem:
 	case <-time.After(buttonTestDelay * time.Millisecond):

drivers/gpio/pir_motion_driver.go

@@ -8,13 +8,12 @@ import (
 
 // PIRMotionDriver represents a digital Proximity Infra Red (PIR) motion detecter
 type PIRMotionDriver struct {
-	Active     bool
-	pin        string
-	name       string
-	halt       chan bool
-	interval   time.Duration
-	connection DigitalReader
+	*Driver
 	gobot.Eventer
+	pin      string
+	active   bool
+	halt     chan bool
+	interval time.Duration
 }
 
 // NewPIRMotionDriver returns a new PIRMotionDriver with a polling interval of
@@ -25,14 +24,15 @@ type PIRMotionDriver struct {
 //	time.Duration: Interval at which the PIRMotionDriver is polled for new information
 func NewPIRMotionDriver(a DigitalReader, pin string, v ...time.Duration) *PIRMotionDriver {
 	b := &PIRMotionDriver{
-		name:       gobot.DefaultName("PIRMotion"),
-		connection: a,
-		pin:        pin,
-		Active:     false,
-		Eventer:    gobot.NewEventer(),
-		interval:   10 * time.Millisecond,
-		halt:       make(chan bool),
+		Driver:   NewDriver(a.(gobot.Connection), "PIRMotion"),
+		Eventer:  gobot.NewEventer(),
+		pin:      pin,
+		active:   false,
+		interval: 10 * time.Millisecond,
+		halt:     make(chan bool),
 	}
+	b.afterStart = b.initialize
+	b.beforeHalt = b.shutdown
 
 	if len(v) > 0 {
 		b.interval = v[0]
@@ -45,7 +45,19 @@ func NewPIRMotionDriver(a DigitalReader, pin string, v ...time.Duration) *PIRMot
 	return b
 }
 
-// Start starts the PIRMotionDriver and polls the state of the sensor at the given interval.
+// Pin returns the PIRMotionDriver pin
+func (p *PIRMotionDriver) Pin() string { return p.pin }
+
+// Active gets the current state
+func (p *PIRMotionDriver) Active() bool {
+	// ensure that read and write can not interfere
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
+
+	return p.active
+}
+
+// initialize the PIRMotionDriver and polls the state of the sensor at the given interval.
 //
 // Emits the Events:
 //
@@ -57,26 +69,14 @@ func NewPIRMotionDriver(a DigitalReader, pin string, v ...time.Duration) *PIRMot
 // just as long as motion is still being detected.
 // It will only send the MotionStopped event once, however, until
 // motion starts being detected again
-func (p *PIRMotionDriver) Start() (err error) {
+func (p *PIRMotionDriver) initialize() error {
 	go func() {
 		for {
-			newValue, err := p.connection.DigitalRead(p.Pin())
+			newValue, err := p.connection.(DigitalReader).DigitalRead(p.Pin())
 			if err != nil {
 				p.Publish(Error, err)
 			}
-			switch newValue {
-			case 1:
-				if !p.Active {
-					p.Active = true
-					p.Publish(MotionDetected, newValue)
-				}
-			case 0:
-				if p.Active {
-					p.Active = false
-					p.Publish(MotionStopped, newValue)
-				}
-			}
-
+			p.update(newValue)
 			select {
 			case <-time.After(p.interval):
 			case <-p.halt:
@@ -84,23 +84,30 @@ func (p *PIRMotionDriver) Start() (err error) {
 			}
 		}
 	}()
-	return
+	return nil
 }
 
-// Halt stops polling the button for new information
-func (p *PIRMotionDriver) Halt() (err error) {
+// shutdown stops polling
+func (p *PIRMotionDriver) shutdown() error {
 	p.halt <- true
-	return
+	return nil
 }
 
-// Name returns the PIRMotionDriver name
-func (p *PIRMotionDriver) Name() string { return p.name }
+func (p *PIRMotionDriver) update(newValue int) {
+	// ensure that read and write can not interfere
+	p.mutex.Lock()
+	defer p.mutex.Unlock()
 
-// SetName sets the PIRMotionDriver name
-func (p *PIRMotionDriver) SetName(n string) { p.name = n }
-
-// Pin returns the PIRMotionDriver pin
-func (p *PIRMotionDriver) Pin() string { return p.pin }
-
-// Connection returns the PIRMotionDriver Connection
-func (p *PIRMotionDriver) Connection() gobot.Connection { return p.connection.(gobot.Connection) }
+	switch newValue {
+	case 1:
+		if !p.active {
+			p.active = true
+			p.Publish(MotionDetected, newValue)
+		}
+	case 0:
+		if p.active {
+			p.active = false
+			p.Publish(MotionStopped, newValue)
+		}
+	}
+}

drivers/gpio/pir_motion_driver_test.go

@@ -1,8 +1,9 @@
 package gpio
 
 import (
-	"errors"
+	"fmt"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -14,42 +15,67 @@ var _ gobot.Driver = (*PIRMotionDriver)(nil)
 
 const motionTestDelay = 150
 
-func initTestPIRMotionDriver() *PIRMotionDriver {
-	return NewPIRMotionDriver(newGpioTestAdaptor(), "1")
+func initTestPIRMotionDriverWithStubbedAdaptor() (*PIRMotionDriver, *gpioTestAdaptor) {
+	a := newGpioTestAdaptor()
+	d := NewPIRMotionDriver(a, "1")
+	return d, a
 }
 
-func TestPIRMotionDriverHalt(t *testing.T) {
-	d := initTestPIRMotionDriver()
-	go func() {
-		<-d.halt
-	}()
-	assert.NoError(t, d.Halt())
-}
-
-func TestPIRMotionDriver(t *testing.T) {
-	d := NewPIRMotionDriver(newGpioTestAdaptor(), "1")
-	assert.NotNil(t, d.Connection())
-
+func TestNewPIRMotionDriver(t *testing.T) {
+	// arrange
+	a := newGpioTestAdaptor()
+	// act
+	d := NewPIRMotionDriver(a, "1")
+	// assert
+	assert.IsType(t, &PIRMotionDriver{}, d)
+	assert.True(t, strings.HasPrefix(d.name, "PIRMotion"))
+	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, 10*time.Millisecond, d.interval)
+	assert.NotNil(t, d.halt)
+	// act & assert other interval
 	d = NewPIRMotionDriver(newGpioTestAdaptor(), "1", 30*time.Second)
 	assert.Equal(t, 30*time.Second, d.interval)
 }
 
-func TestPIRMotionDriverStart(t *testing.T) {
+func TestPIRMotionStart(t *testing.T) {
+	// arrange
 	sem := make(chan bool)
+	nextVal := make(chan int, 1)
 	a := newGpioTestAdaptor()
 	d := NewPIRMotionDriver(a, "1")
 
-	assert.NoError(t, d.Start())
+	a.digitalReadFunc = func(string) (int, error) {
+		val := 1
+		var err error
+		select {
+		case val = <-nextVal:
+			if val < 0 {
+				err = fmt.Errorf("digital read error")
+			}
+			return val, err
+		default:
+			return val, err
+		}
+	}
 
 	_ = d.Once(MotionDetected, 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
+	err := d.Start()
+
+	// assert & rearrange
+	assert.NoError(t, err)
 
 	select {
 	case <-sem:
@@ -58,15 +84,11 @@ func TestPIRMotionDriverStart(t *testing.T) {
 	}
 
 	_ = d.Once(MotionStopped, 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(motionTestDelay * time.Millisecond):
@@ -77,25 +99,76 @@ func TestPIRMotionDriverStart(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(motionTestDelay * time.Millisecond):
 		t.Errorf("PIRMotionDriver Event \"Error\" was not published")
 	}
+
+	_ = d.Once(MotionDetected, func(data interface{}) {
+		sem <- true
+	})
+
+	d.halt <- true
+	nextVal <- 1
+
+	select {
+	case <-sem:
+		t.Errorf("PIRMotion Event \"MotionDetected\" should not published")
+	case <-time.After(motionTestDelay * time.Millisecond):
+	}
 }
 
-func TestPIRDriverDefaultName(t *testing.T) {
-	d := initTestPIRMotionDriver()
-	assert.True(t, strings.HasPrefix(d.Name(), "PIR"))
+func TestPIRMotionHalt(t *testing.T) {
+	// arrange
+	d, _ := initTestPIRMotionDriverWithStubbedAdaptor()
+	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 TestPIRDriverSetName(t *testing.T) {
-	d := initTestPIRMotionDriver()
-	d.SetName("mybot")
-	assert.Equal(t, "mybot", d.Name())
+func TestPIRMotionPin(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 := PIRMotionDriver{pin: name}
+			// act & assert
+			assert.Equal(t, tc.want, d.Pin())
+		})
+	}
+}
+
+func TestPIRMotionActive(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 := PIRMotionDriver{Driver: NewDriver(nil, "PIRMotion")} // just for mutex
+			d.active = tc.want
+			// act & assert
+			assert.Equal(t, tc.want, d.Active())
+		})
+	}
 }