From 9e311b28e41d3393aeb066a32e11d1b314b25bd2 Mon Sep 17 00:00:00 2001 From: Thomas Kohler Date: Wed, 1 Nov 2023 15:49:02 +0100 Subject: [PATCH] gpio: fix data race in PIRMotionDriver (#1028) --- drivers/gpio/button_driver.go | 6 +- drivers/gpio/button_driver_test.go | 8 +- drivers/gpio/pir_motion_driver.go | 93 ++++++++------- drivers/gpio/pir_motion_driver_test.go | 151 ++++++++++++++++++------- 4 files changed, 170 insertions(+), 88 deletions(-) diff --git a/drivers/gpio/button_driver.go b/drivers/gpio/button_driver.go index 978ae375b..72c62d320 100644 --- a/drivers/gpio/button_driver.go +++ b/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 } diff --git a/drivers/gpio/button_driver_test.go b/drivers/gpio/button_driver_test.go index 681c02ffe..c9112e42a 100644 --- a/drivers/gpio/button_driver_test.go +++ b/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): diff --git a/drivers/gpio/pir_motion_driver.go b/drivers/gpio/pir_motion_driver.go index f64d8b589..aa32b50e5 100644 --- a/drivers/gpio/pir_motion_driver.go +++ b/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) + } + } +} diff --git a/drivers/gpio/pir_motion_driver_test.go b/drivers/gpio/pir_motion_driver_test.go index 79ab0d710..b1ad17cd3 100644 --- a/drivers/gpio/pir_motion_driver_test.go +++ b/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 TestPIRMotionDriverHalt(t *testing.T) { - d := initTestPIRMotionDriver() - go func() { - <-d.halt - }() - assert.NoError(t, d.Halt()) +func initTestPIRMotionDriverWithStubbedAdaptor() (*PIRMotionDriver, *gpioTestAdaptor) { + a := newGpioTestAdaptor() + d := NewPIRMotionDriver(a, "1") + return d, a } -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): - t.Errorf("PIRMotionDriver Event \"Error\" was not published") } } -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 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 TestPIRDriverSetName(t *testing.T) { - d := initTestPIRMotionDriver() - d.SetName("mybot") - assert.Equal(t, "mybot", d.Name()) +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()) + }) + } }