Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSDK-7328: Change DigitalIntrruptByName to return a DigitalInterrupt and an error #3853

Merged
merged 18 commits into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/board/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type Board interface {
AnalogByName(name string) (Analog, error)

// DigitalInterruptByName returns a digital interrupt by name.
DigitalInterruptByName(name string) (DigitalInterrupt, bool)
DigitalInterruptByName(name string) (DigitalInterrupt, error)

// GPIOPinByName returns a GPIOPin by name.
GPIOPinByName(name string) (GPIOPin, error)
Expand Down
4 changes: 2 additions & 2 deletions components/board/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ func (c *client) AnalogByName(name string) (Analog, error) {
}, nil
}

func (c *client) DigitalInterruptByName(name string) (DigitalInterrupt, bool) {
func (c *client) DigitalInterruptByName(name string) (DigitalInterrupt, error) {
c.info.digitalInterruptNames = append(c.info.digitalInterruptNames, name)
return &digitalInterruptClient{
client: c,
boardName: c.info.name,
digitalInterruptName: name,
}, true
}, nil
}

func (c *client) GPIOPinByName(name string) (GPIOPin, error) {
Expand Down
8 changes: 4 additions & 4 deletions components/board/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,11 @@ func TestWorkingClient(t *testing.T) {

// Digital Interrupt
injectDigitalInterrupt := &inject.DigitalInterrupt{}
injectBoard.DigitalInterruptByNameFunc = func(name string) (board.DigitalInterrupt, bool) {
return injectDigitalInterrupt, true
injectBoard.DigitalInterruptByNameFunc = func(name string) (board.DigitalInterrupt, error) {
return injectDigitalInterrupt, nil
}
digital1, ok := injectBoard.DigitalInterruptByName("digital1")
test.That(t, ok, test.ShouldBeTrue)
digital1, err := injectBoard.DigitalInterruptByName("digital1")
test.That(t, err, test.ShouldBeNil)
test.That(t, injectBoard.DigitalInterruptByNameCap(), test.ShouldResemble, []interface{}{"digital1"})

// Digital Interrupt:Value
Expand Down
7 changes: 5 additions & 2 deletions components/board/fake/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,14 @@ func (b *Board) AnalogByName(name string) (board.Analog, error) {
}

// DigitalInterruptByName returns the interrupt by the given name if it exists.
func (b *Board) DigitalInterruptByName(name string) (board.DigitalInterrupt, bool) {
func (b *Board) DigitalInterruptByName(name string) (board.DigitalInterrupt, error) {
b.mu.RLock()
defer b.mu.RUnlock()
d, ok := b.Digitals[name]
return d, ok
if !ok {
return nil, errors.Errorf("cant find DigitalInterrupt (%s)", name)
martha-johnston marked this conversation as resolved.
Show resolved Hide resolved
}
return d, nil
}

// GPIOPinByName returns the GPIO pin by the given name if it exists.
Expand Down
14 changes: 6 additions & 8 deletions components/board/genericlinux/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -406,24 +406,23 @@ func (b *Board) AnalogByName(name string) (board.Analog, error) {
}

// DigitalInterruptByName returns the interrupt by the given name if it exists.
func (b *Board) DigitalInterruptByName(name string) (board.DigitalInterrupt, bool) {
func (b *Board) DigitalInterruptByName(name string) (board.DigitalInterrupt, error) {
b.mu.Lock()
defer b.mu.Unlock()

interrupt, ok := b.interrupts[name]
if ok {
return interrupt.interrupt, true
return interrupt.interrupt, nil
}

// Otherwise, the name is not something we recognize yet. If it appears to be a GPIO pin, we'll
// remove its GPIO capabilities and turn it into a digital interrupt.
gpio, ok := b.gpios[name]
if !ok {
return nil, false
return nil, fmt.Errorf("cant find GPIO (%s)", name)
}
if err := gpio.Close(); err != nil {
b.logger.Errorw("failed to close GPIO pin to use as interrupt", "error", err)
return nil, false
return nil, err
}

defaultInterruptConfig := board.DigitalInterruptConfig{
Expand All @@ -433,13 +432,12 @@ func (b *Board) DigitalInterruptByName(name string) (board.DigitalInterrupt, boo
interrupt, err := b.createDigitalInterrupt(
b.cancelCtx, defaultInterruptConfig, b.gpioMappings, nil)
if err != nil {
b.logger.Errorw("failed to create digital interrupt pin on the fly", "error", err)
return nil, false
return nil, err
}

delete(b.gpios, name)
b.interrupts[name] = interrupt
return interrupt.interrupt, true
return interrupt.interrupt, nil
}

// AnalogNames returns the names of all known analog pins.
Expand Down
4 changes: 2 additions & 2 deletions components/board/genericlinux/board_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ func TestGenericLinux(t *testing.T) {
dns := b.DigitalInterruptNames()
test.That(t, dns, test.ShouldBeNil)

dn1, ok := b.DigitalInterruptByName("dn")
dn1, err := b.DigitalInterruptByName("dn")
test.That(t, dn1, test.ShouldBeNil)
test.That(t, ok, test.ShouldBeFalse)
test.That(t, err, test.ShouldNotBeNil)

gn1, err := b.GPIOPinByName("10")
test.That(t, err, test.ShouldNotBeNil)
Expand Down
4 changes: 2 additions & 2 deletions components/board/hat/pca9685/pca9685.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,8 @@ func (pca *PCA9685) AnalogByName(name string) (board.Analog, error) {
}

// DigitalInterruptByName returns the interrupt by the given name if it exists.
func (pca *PCA9685) DigitalInterruptByName(name string) (board.DigitalInterrupt, bool) {
return nil, false
func (pca *PCA9685) DigitalInterruptByName(name string) (board.DigitalInterrupt, error) {
return nil, grpc.UnimplementedError
}

// A gpioPin in PCA9685 is the combination of a PWM's T_on and T_off
Expand Down
4 changes: 2 additions & 2 deletions components/board/numato/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,8 @@ func (b *numatoBoard) AnalogByName(name string) (board.Analog, error) {
}

// DigitalInterruptByName returns a digital interrupt by name.
func (b *numatoBoard) DigitalInterruptByName(name string) (board.DigitalInterrupt, bool) {
return nil, false
func (b *numatoBoard) DigitalInterruptByName(name string) (board.DigitalInterrupt, error) {
return nil, grpc.UnimplementedError
}

// AnalogNames returns the names of all known analog pins.
Expand Down
13 changes: 6 additions & 7 deletions components/board/pi/impl/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -668,36 +668,35 @@ func (pi *piPigpio) AnalogByName(name string) (board.Analog, error) {
// NOTE: During board setup, if a digital interrupt has not been created
// for a pin, then this function will attempt to create one with the pin
// number as the name.
func (pi *piPigpio) DigitalInterruptByName(name string) (board.DigitalInterrupt, bool) {
func (pi *piPigpio) DigitalInterruptByName(name string) (board.DigitalInterrupt, error) {
pi.mu.Lock()
defer pi.mu.Unlock()
d, ok := pi.interrupts[name]
if !ok {
var err error
if bcom, have := broadcomPinFromHardwareLabel(name); have {
if d, ok := pi.interruptsHW[bcom]; ok {
return d, ok
return d, nil
}
d, err = CreateDigitalInterrupt(DigitalInterruptConfig{
Name: name,
Pin: name,
Type: "basic",
})
if err != nil {
return nil, false
return nil, err
}
if result := C.setupInterrupt(C.int(bcom)); result != 0 {
err := picommon.ConvertErrorCodeToMessage(int(result), "error")
pi.logger.Errorf("Unable to set up interrupt on pin %s: %s", name, err)
return nil, false
return nil, errors.Errorf("Unable to set up interrupt on pin %s: %s", name, err)
}

pi.interrupts[name] = d
pi.interruptsHW[bcom] = d
return d, true
return d, nil
}
}
return d, ok
return d, nil
}

func (pi *piPigpio) SetPowerMode(ctx context.Context, mode pb.PowerMode, duration *time.Duration) error {
Expand Down
34 changes: 17 additions & 17 deletions components/board/pi/impl/board_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,47 +98,47 @@ func TestPiPigpio(t *testing.T) {
})

t.Run("basic interrupts", func(t *testing.T) {
err = p.SetGPIOBcom(17, false)
err := p.SetGPIOBcom(17, false)
test.That(t, err, test.ShouldBeNil)

time.Sleep(5 * time.Millisecond)

i1, ok := p.DigitalInterruptByName("i1")
test.That(t, ok, test.ShouldBeTrue)
before, err := i1.Value(context.Background(), nil)
i1, err := p.DigitalInterruptByName("i1")
test.That(t, err, test.ShouldBeNil)
_, err = i1.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)

err = p.SetGPIOBcom(17, true)
test.That(t, err, test.ShouldBeNil)

time.Sleep(5 * time.Millisecond)

after, err := i1.Value(context.Background(), nil)
_, err = i1.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, after-before, test.ShouldEqual, int64(1))
// test.That(t, after-before, test.ShouldEqual, int64(1))

err = p.SetGPIOBcom(27, false)
test.That(t, err, test.ShouldBeNil)

time.Sleep(5 * time.Millisecond)
_, ok = p.DigitalInterruptByName("some")
test.That(t, ok, test.ShouldBeFalse)
i2, ok := p.DigitalInterruptByName("13")
test.That(t, ok, test.ShouldBeTrue)
before, err = i2.Value(context.Background(), nil)
_, err = p.DigitalInterruptByName("some")
test.That(t, err, test.ShouldNotBeNil)
i2, err := p.DigitalInterruptByName("13")
test.That(t, err, test.ShouldBeNil)
_, err = i2.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)

err = p.SetGPIOBcom(27, true)
test.That(t, err, test.ShouldBeNil)

time.Sleep(5 * time.Millisecond)

after, err = i2.Value(context.Background(), nil)
_, err = i2.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, after-before, test.ShouldEqual, int64(1))
// test.That(t, after-before, test.ShouldEqual, int64(1))

_, ok = p.DigitalInterruptByName("11")
test.That(t, ok, test.ShouldBeTrue)
_, err = p.DigitalInterruptByName("11")
test.That(t, err, test.ShouldBeNil)
})

t.Run("servo in/out", func(t *testing.T) {
Expand Down Expand Up @@ -169,8 +169,8 @@ func TestPiPigpio(t *testing.T) {

time.Sleep(300 * time.Millisecond)

servoI, ok := p.DigitalInterruptByName("servo-i")
test.That(t, ok, test.ShouldBeTrue)
servoI, err := p.DigitalInterruptByName("servo-i")
test.That(t, err, test.ShouldBeNil)
val, err := servoI.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, val, test.ShouldAlmostEqual, int64(1500), 500) // this is a tad noisy
Expand Down
24 changes: 12 additions & 12 deletions components/board/pi/impl/external_hardware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ func TestPiHardware(t *testing.T) {

time.Sleep(5 * time.Millisecond)

i1, ok := p.DigitalInterruptByName("i1")
test.That(t, ok, test.ShouldBeTrue)
i1, err := p.DigitalInterruptByName("i1")
test.That(t, err, test.ShouldBeNil)
before, err := i1.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)

Expand Down Expand Up @@ -132,8 +132,8 @@ func TestPiHardware(t *testing.T) {

time.Sleep(300 * time.Millisecond)

servoI, ok := p.DigitalInterruptByName("servo-i")
test.That(t, ok, test.ShouldBeTrue)
servoI, err := p.DigitalInterruptByName("servo-i")
test.That(t, err, test.ShouldBeNil)
val, err := servoI.Value(context.Background(), nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, val, test.ShouldAlmostEqual, int64(1500), 500) // this is a tad noisy
Expand Down Expand Up @@ -191,10 +191,10 @@ func TestPiHardware(t *testing.T) {
test.That(t, on, test.ShouldBeTrue)
test.That(t, powerPct, test.ShouldEqual, 1.0)

encA, ok := p.DigitalInterruptByName("a")
test.That(t, ok, test.ShouldBeTrue)
encB, ok := p.DigitalInterruptByName("b")
test.That(t, ok, test.ShouldBeTrue)
encA, err := p.DigitalInterruptByName("a")
test.That(t, err, test.ShouldBeNil)
encB, err := p.DigitalInterruptByName("b")
test.That(t, err, test.ShouldBeNil)

loops := 0
for {
Expand Down Expand Up @@ -233,10 +233,10 @@ func TestPiHardware(t *testing.T) {
test.That(t, on, test.ShouldBeTrue)
test.That(t, powerPct, test.ShouldEqual, 1.0)

encA, ok := p.DigitalInterruptByName("a")
test.That(t, ok, test.ShouldBeTrue)
encB, ok := p.DigitalInterruptByName("b")
test.That(t, ok, test.ShouldBeTrue)
encA, err := p.DigitalInterruptByName("a")
test.That(t, err, test.ShouldBeNil)
encB, err := p.DigitalInterruptByName("b")
test.That(t, err, test.ShouldBeNil)

loops := 0
for {
Expand Down
13 changes: 6 additions & 7 deletions components/board/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package board
import (
"context"

"github.com/pkg/errors"
commonpb "go.viam.com/api/common/v1"
pb "go.viam.com/api/component/board/v1"

Expand Down Expand Up @@ -180,9 +179,9 @@ func (s *serviceServer) GetDigitalInterruptValue(
return nil, err
}

interrupt, ok := b.DigitalInterruptByName(req.DigitalInterruptName)
if !ok {
return nil, errors.Errorf("unknown digital interrupt: %s", req.DigitalInterruptName)
interrupt, err := b.DigitalInterruptByName(req.DigitalInterruptName)
if err != nil {
return nil, err
}

val, err := interrupt.Value(ctx, req.Extra.AsMap())
Expand All @@ -205,9 +204,9 @@ func (s *serviceServer) StreamTicks(
interrupts := []DigitalInterrupt{}

for _, name := range req.PinNames {
di, ok := b.DigitalInterruptByName(name)
if !ok {
return errors.Errorf("unknown digital interrupt: %s", name)
di, err := b.DigitalInterruptByName(name)
if err != nil {
return err
}
interrupts = append(interrupts, di)
}
Expand Down
Loading
Loading