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-7152 Analog Reader API rdk changes #3946

Merged
merged 20 commits into from
May 16, 2024
13 changes: 12 additions & 1 deletion components/board/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,23 @@ type Board interface {
// An Analog represents an analog pin that resides on a board.
type Analog interface {
// Read reads off the current value.
Read(ctx context.Context, extra map[string]interface{}) (int, error)
Read(ctx context.Context, extra map[string]interface{}) (AnalogValue, error)

// Write writes a value to the analog pin.
Write(ctx context.Context, value int, extra map[string]interface{}) error
}

// AnalogValue contains all info about the analog reading.
// Value represents the reading in bits.
// Min and Max represent the range of raw analog values.
// StepSize is the precision per bit of the reading.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks for this comment!

type AnalogValue struct {
Value int
Min float32
Max float32
StepSize float32
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
}

// FromDependencies is a helper for getting the named board from a collection of
// dependencies.
func FromDependencies(deps resource.Dependencies, name string) (Board, error) {
Expand Down
8 changes: 4 additions & 4 deletions components/board/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,10 @@ type analogClient struct {
analogName string
}

func (ac *analogClient) Read(ctx context.Context, extra map[string]interface{}) (int, error) {
func (ac *analogClient) Read(ctx context.Context, extra map[string]interface{}) (AnalogValue, error) {
ext, err := protoutils.StructToStructPb(extra)
if err != nil {
return 0, err
return AnalogValue{}, err
}
// the api method is named ReadAnalogReader, it is named differently than
// the board interface functions.
Expand All @@ -147,9 +147,9 @@ func (ac *analogClient) Read(ctx context.Context, extra map[string]interface{})
Extra: ext,
})
if err != nil {
return 0, err
return AnalogValue{}, err
}
return int(resp.Value), nil
return AnalogValue{Value: int(resp.Value), Min: resp.MinRange, Max: resp.MaxRange, StepSize: resp.StepSize}, nil
}

func (ac *analogClient) Write(ctx context.Context, value int, extra map[string]interface{}) error {
Expand Down
11 changes: 7 additions & 4 deletions components/board/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,16 @@ func TestWorkingClient(t *testing.T) {
test.That(t, injectBoard.AnalogByNameCap(), test.ShouldResemble, []interface{}{"analog1"})

// Analog: Read
injectAnalog.ReadFunc = func(ctx context.Context, extra map[string]interface{}) (int, error) {
injectAnalog.ReadFunc = func(ctx context.Context, extra map[string]interface{}) (board.AnalogValue, error) {
actualExtra = extra
return 6, nil
return board.AnalogValue{Value: 6, Min: 0, Max: 10, StepSize: 0.1}, nil
}
readVal, err := analog1.Read(context.Background(), expectedExtra)
analogVal, err := analog1.Read(context.Background(), expectedExtra)
test.That(t, err, test.ShouldBeNil)
test.That(t, readVal, test.ShouldEqual, 6)
test.That(t, analogVal.Value, test.ShouldEqual, 6)
test.That(t, analogVal.Min, test.ShouldEqual, 0)
test.That(t, analogVal.Max, test.ShouldEqual, 10)
test.That(t, analogVal.StepSize, test.ShouldEqual, 0.1)
test.That(t, actualExtra, test.ShouldResemble, expectedExtra)
actualExtra = nil

Expand Down
9 changes: 6 additions & 3 deletions components/board/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@ func newAnalogCollector(resource interface{}, params data.CollectorParams) (data
}

cFunc := data.CaptureFunc(func(ctx context.Context, arg map[string]*anypb.Any) (interface{}, error) {
var value int
var analogValue AnalogValue
if _, ok := arg[analogReaderNameKey]; !ok {
return nil, data.FailedToReadErr(params.ComponentName, analogs.String(),
errors.New("Must supply reader_name in additional_params for analog collector"))
}
if reader, err := board.AnalogByName(arg[analogReaderNameKey].String()); err == nil {
value, err = reader.Read(ctx, data.FromDMExtraMap)
analogValue, err = reader.Read(ctx, data.FromDMExtraMap)
if err != nil {
// A modular filter component can be created to filter the readings from a component. The error ErrNoCaptureToStore
// is used in the datamanager to exclude readings from being captured and stored.
Expand All @@ -57,7 +57,10 @@ func newAnalogCollector(resource interface{}, params data.CollectorParams) (data
}
}
return pb.ReadAnalogReaderResponse{
Value: int32(value),
Value: int32(analogValue.Value),
MinRange: analogValue.Min,
MaxRange: analogValue.Max,
StepSize: analogValue.StepSize,
}, nil
})
return data.NewCollector(cFunc, params)
Expand Down
9 changes: 6 additions & 3 deletions components/board/collectors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ func TestCollectors(t *testing.T) {
},
collector: board.NewAnalogCollector,
expected: tu.ToProtoMapIgnoreOmitEmpty(pb.ReadAnalogReaderResponse{
Value: 1,
Value: 1,
MinRange: 0,
MaxRange: 10,
StepSize: 0.1,
}),
shouldError: false,
},
Expand Down Expand Up @@ -96,8 +99,8 @@ func TestCollectors(t *testing.T) {
func newBoard() board.Board {
b := &inject.Board{}
analog := &inject.Analog{}
analog.ReadFunc = func(ctx context.Context, extra map[string]interface{}) (int, error) {
return 1, nil
analog.ReadFunc = func(ctx context.Context, extra map[string]interface{}) (board.AnalogValue, error) {
return board.AnalogValue{Value: 1, Min: 0, Max: 10, StepSize: 0.1}, nil
}
b.AnalogByNameFunc = func(name string) (board.Analog, error) {
return analog, nil
Expand Down
7 changes: 4 additions & 3 deletions components/board/fake/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,15 @@ func (a *Analog) reset(pin string) {
a.Mu.Unlock()
}

func (a *Analog) Read(ctx context.Context, extra map[string]interface{}) (int, error) {
func (a *Analog) Read(ctx context.Context, extra map[string]interface{}) (board.AnalogValue, error) {
a.Mu.RLock()
defer a.Mu.RUnlock()
if a.pin != analogTestPin {
a.Value = a.fakeValue
a.fakeValue++
a.fakeValue %= 1001
a.Value = a.fakeValue
}
return a.Value, nil
return board.AnalogValue{Value: a.Value, Min: 0, Max: 1000, StepSize: 1}, nil
}

func (a *Analog) Write(ctx context.Context, value int, extra map[string]interface{}) error {
Expand Down
4 changes: 2 additions & 2 deletions components/board/genericlinux/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -351,11 +351,11 @@ func newWrappedAnalogReader(ctx context.Context, chipSelect string, reader *pinw
return &wrapped
}

func (a *wrappedAnalogReader) Read(ctx context.Context, extra map[string]interface{}) (int, error) {
func (a *wrappedAnalogReader) Read(ctx context.Context, extra map[string]interface{}) (board.AnalogValue, error) {
a.mu.RLock()
defer a.mu.RUnlock()
if a.reader == nil {
return 0, errors.New("closed")
return board.AnalogValue{}, errors.New("closed")
}
return a.reader.Read(ctx, extra)
}
Expand Down
12 changes: 8 additions & 4 deletions components/board/mcp3008helper/mcp3008.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

"go.uber.org/multierr"

"go.viam.com/rdk/components/board"
"go.viam.com/rdk/components/board/genericlinux/buses"
"go.viam.com/rdk/grpc"
"go.viam.com/rdk/resource"
Expand Down Expand Up @@ -37,29 +38,32 @@ func (config *MCP3008AnalogConfig) Validate(path string) error {
return nil
}

func (mar *MCP3008AnalogReader) Read(ctx context.Context, extra map[string]interface{}) (value int, err error) {
func (mar *MCP3008AnalogReader) Read(ctx context.Context, extra map[string]interface{}) (
analogVal board.AnalogValue, err error,
) {
var tx [3]byte
tx[0] = 1 // start bit
tx[1] = byte((8 + mar.Channel) << 4) // single-ended
tx[2] = 0 // extra clocks to receive full 10 bits of data

bus, err := mar.Bus.OpenHandle()
if err != nil {
return 0, err
return board.AnalogValue{}, err
}
defer func() {
err = multierr.Combine(err, bus.Close())
}()

rx, err := bus.Xfer(ctx, 1000000, mar.Chip, 0, tx[:])
if err != nil {
return 0, err
return board.AnalogValue{}, err
}
// Reassemble the 10-bit value. Do not include bits before the final 10, because they contain
// garbage and might be non-zero.
val := 0x03FF & ((int(rx[1]) << 8) | int(rx[2]))

return val, nil
// returning no analog range since mcp3008 will be removed soon.
return board.AnalogValue{Value: val}, nil
}

// Close does nothing.
Expand Down
68 changes: 59 additions & 9 deletions components/board/numato/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,8 @@ type numatoBoard struct {
sent map[string]bool
sentMu sync.Mutex
workers rdkutils.StoppableWorkers

productID int
}

func (b *numatoBoard) addToSent(msg string) {
Expand Down Expand Up @@ -354,12 +356,43 @@ type analog struct {
pin string
}

func (a *analog) Read(ctx context.Context, extra map[string]interface{}) (int, error) {
// Read returns the analog value with the range and step size in V/bit.
func (a *analog) Read(ctx context.Context, extra map[string]interface{}) (board.AnalogValue, error) {
res, err := a.b.doSendReceive(ctx, fmt.Sprintf("adc read %s", a.pin))
if err != nil {
return 0, err
return board.AnalogValue{}, err
}
reading, err := strconv.Atoi(res)
if err != nil {
return board.AnalogValue{}, err
}
return strconv.Atoi(res)
var max float32
var stepSize float32
switch a.b.productID {
case 0x805:
// 128 channel usb numato has 12 bit resolution
max = 3.3
stepSize = max / 4096
case 0x802:
// 32 channel usb numato has 10 bit resolution
max = 3.3
stepSize = max / 1024
case 0x800:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mild preference to sort the cases in the switch statement, so it's easier to find the one you care about.

// 8 and 16 pin usb versions have the same product ID but different voltage ranges
// both have 10 bit resolution
if a.b.pins == 8 {
max = 5.0
} else if a.b.pins == 16 {
max = 3.3
}
stepSize = max / 1024
case 0xC05:
// 1 channel usb relay module numato - 10 bit resolution
max = 5.0
stepSize = max / 1024
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion question: none of these ever change, right? Would it be simpler to calculate them once on initialization, and then just use the stored values within Read()? I'm unsure: we don't really care about them until here, but it feels wasteful to have a switch statement (which contains a memory fence, which slows everything down) every time we read the value. and yet, such an optimization is probably small enough it's not even worth worrying over, and this version is just fine. I dunno; what do you think? Maybe leave as-is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that makes sense to me, ill do this at the start instead.

default:
}
return board.AnalogValue{Value: reading, Min: 0, Max: max, StepSize: stepSize}, nil
}

func (a *analog) Write(ctx context.Context, value int, extra map[string]interface{}) error {
Expand All @@ -384,6 +417,24 @@ func connect(ctx context.Context, name resource.Name, conf *Config, logger loggi
path = devs[0].Path
}

// Find the numato board's productid
products := getSerialDevices()

var productID int
for _, product := range products {
if product.ID.Vendor != 0x2a19 {
continue
}
// we can safely get the first numato productID we find because
// we only support one board being used at a time
productID = product.ID.Product
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
break
}

if productID != 0x800 && productID != 0x802 && productID != 0x805 && productID != 0xC05 {
logger.Warnf("analog range and step size is not supported for numato with product id %d", productID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is to are.

}

options := goserial.OpenOptions{
PortName: path,
BaudRate: 19200,
Expand All @@ -396,12 +447,12 @@ func connect(ctx context.Context, name resource.Name, conf *Config, logger loggi
if err != nil {
return nil, err
}

b := &numatoBoard{
Named: name.AsNamed(),
pins: pins,
port: device,
logger: logger,
Named: name.AsNamed(),
pins: pins,
port: device,
logger: logger,
productID: productID,
}

b.analogs = map[string]*pinwrappers.AnalogSmoother{}
Expand All @@ -419,6 +470,5 @@ func connect(ctx context.Context, name resource.Name, conf *Config, logger loggi
return nil, multierr.Combine(b.Close(ctx), err)
}
b.logger.CDebugw(ctx, "numato startup", "version", ver)

return b, nil
}
10 changes: 7 additions & 3 deletions components/board/numato/board_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,21 +93,25 @@ func TestNumato1(t *testing.T) {

res2, err := ar.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, res2, test.ShouldBeLessThan, 100)
test.That(t, res2.Value, test.ShouldBeLessThan, 100)

// Only testing these since the values depend on what board version is connected.
test.That(t, res2.Min, test.ShouldEqual, 0)
test.That(t, res2.Max, test.ShouldBeGreaterThanOrEqualTo, 3.3)

err = zeroPin.Set(context.Background(), true, nil)
test.That(t, err, test.ShouldBeNil)

res2, err = ar.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, res2, test.ShouldBeGreaterThan, 1000)
test.That(t, res2.Value, test.ShouldBeGreaterThan, 1000)

err = zeroPin.Set(context.Background(), false, nil)
test.That(t, err, test.ShouldBeNil)

res2, err = ar.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, res2, test.ShouldBeLessThan, 100)
test.That(t, res2.Value, test.ShouldBeLessThan, 100)
}

func TestConfigValidate(t *testing.T) {
Expand Down
12 changes: 12 additions & 0 deletions components/board/numato/darwin_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//go:build darwin

package numato

import "go.viam.com/utils/usb"

// getSerialDevices returns all devices connected by USB on a mac.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment doesn't really match code: I think this returns descriptions of devices, not devices themselves. and yet, I can get that from the function signature, so it doesn't necessarily need to be in the comment, too. but I still don't easily see what this function is for. I think the point is that we use it to find the productID of the numato board plugged in, and use that to figure out what capabilities the board has? That's the part that really belongs in a comment: supply the "why" that isn't easily gotten from the code (which supplies the "what").

Same suggestion in linux_utils.go, too.

func getSerialDevices() []usb.Description {
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
return usb.Search(usb.NewSearchFilter("AppleUSBACMData", "usbmodem"), func(vendorID, productID int) bool {
return true
})
}
12 changes: 12 additions & 0 deletions components/board/numato/linux_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
//go:build linux

package numato

import "go.viam.com/utils/usb"

// getSerialDevices returns all devices connected by USB on a linux machine.
func getSerialDevices() []usb.Description {
return usb.Search(usb.SearchFilter{}, func(vendorID, productID int) bool {
return true
})
}
6 changes: 3 additions & 3 deletions components/board/pi/impl/external_hardware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,23 +67,23 @@ func TestPiHardware(t *testing.T) {

v, err := reader.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, v, test.ShouldAlmostEqual, 0, 150)
test.That(t, v.Value, test.ShouldAlmostEqual, 0, 150)

// try to set high
err = p.SetGPIOBcom(26, true)
test.That(t, err, test.ShouldBeNil)

v, err = reader.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, v, test.ShouldAlmostEqual, 1023, 150)
test.That(t, v.Value, test.ShouldAlmostEqual, 1023, 150)

// back to low
err = p.SetGPIOBcom(26, false)
test.That(t, err, test.ShouldBeNil)

v, err = reader.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, v, test.ShouldAlmostEqual, 0, 150)
test.That(t, v.Value, test.ShouldAlmostEqual, 0, 150)
})

t.Run("basic interrupts", func(t *testing.T) {
Expand Down
Loading
Loading