From c41604f5f9a86ca80dd4fbe0f02b81bf5b011ddb Mon Sep 17 00:00:00 2001 From: Thomas Kohler Date: Wed, 1 Nov 2023 14:51:07 +0100 Subject: [PATCH] gpio: fix data race in ButtonDriver (#1027) --- README.md | 2 +- drivers/gpio/README.md | 2 +- drivers/gpio/button_driver.go | 76 ++++++---- drivers/gpio/button_driver_test.go | 185 +++++++++++++++-------- drivers/gpio/makey_button_driver.go | 101 ------------- drivers/gpio/makey_button_driver_test.go | 117 -------------- examples/beaglebone_makey_button.go | 38 ----- examples/firmata_makey_button.go | 46 ------ 8 files changed, 169 insertions(+), 398 deletions(-) delete mode 100644 drivers/gpio/makey_button_driver.go delete mode 100644 drivers/gpio/makey_button_driver_test.go delete mode 100644 examples/beaglebone_makey_button.go delete mode 100644 examples/firmata_makey_button.go diff --git a/README.md b/README.md index 8a1a4310a..2becbb603 100644 --- a/README.md +++ b/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 diff --git a/drivers/gpio/README.md b/drivers/gpio/README.md index fbd8fe192..8426a02c0 100644 --- a/drivers/gpio/README.md +++ b/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 diff --git a/drivers/gpio/button_driver.go b/drivers/gpio/button_driver.go index b5451de09..978ae375b 100644 --- a/drivers/gpio/button_driver.go +++ b/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) } } diff --git a/drivers/gpio/button_driver_test.go b/drivers/gpio/button_driver_test.go index 7455418cd..681c02ffe 100644 --- a/drivers/gpio/button_driver_test.go +++ b/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 TestButtonDriverHalt(t *testing.T) { - d := initTestButtonDriver() - go func() { - <-d.halt - }() - assert.NoError(t, d.Halt()) +func initTestButtonDriverWithStubbedAdaptor() (*ButtonDriver, *gpioTestAdaptor) { + a := newGpioTestAdaptor() + d := NewButtonDriver(a, "1") + return d, a } -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()) + }) + } } diff --git a/drivers/gpio/makey_button_driver.go b/drivers/gpio/makey_button_driver.go deleted file mode 100644 index fa3a31464..000000000 --- a/drivers/gpio/makey_button_driver.go +++ /dev/null @@ -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 -} diff --git a/drivers/gpio/makey_button_driver_test.go b/drivers/gpio/makey_button_driver_test.go deleted file mode 100644 index 996e6a39a..000000000 --- a/drivers/gpio/makey_button_driver_test.go +++ /dev/null @@ -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): - } -} diff --git a/examples/beaglebone_makey_button.go b/examples/beaglebone_makey_button.go deleted file mode 100644 index f51e172d6..000000000 --- a/examples/beaglebone_makey_button.go +++ /dev/null @@ -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() -} diff --git a/examples/firmata_makey_button.go b/examples/firmata_makey_button.go deleted file mode 100644 index ff3985937..000000000 --- a/examples/firmata_makey_button.go +++ /dev/null @@ -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() -}