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
9 changes: 8 additions & 1 deletion components/board/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,19 @@ 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{}) (int, AnalogRange, error)
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: what are the pros and cons of this approach vs. making the new struct AnalogValue and having all 4 fields in it (the 3 currently in AnalogRange and the value itself)? I find the new function signature to Read surprising, because I'd expect it to return the data we read and an error, and having a third return value is unexpected. AnalogValue would also more closely match the protobuf structure (though matching the protobuf structure is not a goal in and of itself; it's just an unsurprising and natural thing to do).

I'm not opposed to this approach, I just think it's surprising, and am wondering if there's a reason you went with this choice instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can change to returning anAnalogValue if you prefer it. I thought that adding a new return would be less breaking since if you still just want the bit value only you can get it and do _ for the analogrange, but still a breaking change either way so probably doesn't matter that much

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong preference; I just think it's something to consider, in the hopes that on further thought one design is better than the other. Leave as-is if you want. 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

okay I changed it to one return value, prob better to just have one if theres not a great reason to have 2


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

// AnalogRange contains themin, max, and stepSize of the analog reader.
type AnalogRange struct {
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{}) (int, AnalogRange, error) {
ext, err := protoutils.StructToStructPb(extra)
if err != nil {
return 0, err
return 0, AnalogRange{}, 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 0, AnalogRange{}, err
}
return int(resp.Value), nil
return int(resp.Value), AnalogRange{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
9 changes: 6 additions & 3 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{}) (int, board.AnalogRange, error) {
actualExtra = extra
return 6, nil
return 6, board.AnalogRange{Min: 0, Max: 10, StepSize: 0.1}, nil
}
readVal, err := analog1.Read(context.Background(), expectedExtra)
readVal, analogRange, err := analog1.Read(context.Background(), expectedExtra)
test.That(t, err, test.ShouldBeNil)
test.That(t, readVal, test.ShouldEqual, 6)
test.That(t, analogRange.Min, test.ShouldEqual, 0)
test.That(t, analogRange.Max, test.ShouldEqual, 10)
test.That(t, analogRange.StepSize, test.ShouldEqual, 0.1)
test.That(t, actualExtra, test.ShouldResemble, expectedExtra)
actualExtra = nil

Expand Down
8 changes: 6 additions & 2 deletions components/board/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,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 analogRange AnalogRange
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)
value, analogRange, 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 +58,10 @@ func newAnalogCollector(resource interface{}, params data.CollectorParams) (data
}
}
return pb.ReadAnalogReaderResponse{
Value: int32(value),
Value: int32(value),
MinRange: analogRange.Min,
MaxRange: analogRange.Max,
StepSize: analogRange.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{}) (int, board.AnalogRange, error) {
return 1, board.AnalogRange{Min: 0, Max: 10, StepSize: 0.1}, nil
}
b.AnalogByNameFunc = func(name string) (board.Analog, error) {
return analog, nil
Expand Down
10 changes: 7 additions & 3 deletions components/board/fake/board.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,18 @@ 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{}) (int, board.AnalogRange, error) {
a.Mu.RLock()
defer a.Mu.RUnlock()
if a.pin != analogTestPin {
a.Value = a.fakeValue
a.fakeValue++
if a.fakeValue == 1000 {
a.fakeValue = 0
} else {
a.fakeValue++
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
}
}
return a.Value, nil
return a.Value, board.AnalogRange{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{}) (int, board.AnalogRange, error) {
a.mu.RLock()
defer a.mu.RUnlock()
if a.reader == nil {
return 0, errors.New("closed")
return 0, board.AnalogRange{}, 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{}) (
value int, analogRange board.AnalogRange, 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 0, board.AnalogRange{}, 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 0, board.AnalogRange{}, 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 blank analog range since mcp3008 will be removed soon.
return val, board.AnalogRange{}, nil
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
}

// Close does nothing.
Expand Down
66 changes: 57 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,44 @@ type analog struct {
pin string
}

func (a *analog) Read(ctx context.Context, extra map[string]interface{}) (int, error) {
// analog Read returns the analog value with the range and step size in mV/bit.
func (a *analog) Read(ctx context.Context, extra map[string]interface{}) (int, board.AnalogRange, error) {
res, err := a.b.doSendReceive(ctx, fmt.Sprintf("adc read %s", a.pin))
if err != nil {
return 0, err
return 0, board.AnalogRange{}, err
}
reading, err := strconv.Atoi(res)
if err != nil {
return 0, board.AnalogRange{}, 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:
}
stepSize *= 1000
Copy link
Member

Choose a reason for hiding this comment

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

Why are we multiplying stepSize by a thousand? The whole idea of "maximum voltage divided by maximum bit size" seemed like a good idea to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

the scope doc converted the example calculation to mV/bit, I think it makes it more readable otherwise its very small values but willing to discuss changing.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense! Put a comment here pointing out the right units.

(apologies for the confusion where I remarked on this in the test)

Copy link
Member

Choose a reason for hiding this comment

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

...wait, so Min and Max are in volts, but StepSize is in millivolts? Put a comment in the definition of the struct explaining that.

...or rather, it seems surprising that they have different units, and perhaps the way to make it less surprising, instead of documenting the surprise, is to change the units so they're either all volts or all millivolts. I checked the scope doc, but it doesn't explicitly mention the units on any of these values (two examples mention mV and rejected alternative 3 mentions units, but I don't think this was ever laid out clearly).

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 thats fair ill just keep it in volts/bit

return reading, board.AnalogRange{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 +418,21 @@ 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
}
productID = product.ID.Product
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
}

if productID != 0x805|0x802|0x800|0x0C05 {
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved
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 +445,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 +468,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 @@ -91,21 +91,25 @@ func TestNumato1(t *testing.T) {
ar, err := b.AnalogByName("foo")
test.That(t, err, test.ShouldBeNil, true)

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

test.That(t, analogRange.Min, test.ShouldEqual, 0)
test.That(t, analogRange.Max, test.ShouldEqual, 5)
test.That(t, analogRange.StepSize, test.ShouldEqual, 4.88)
oliviamiller marked this conversation as resolved.
Show resolved Hide resolved

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

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

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

res2, err = ar.Read(ctx, nil)
res2, _, err = ar.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, res2, test.ShouldBeLessThan, 100)
}
Expand Down
11 changes: 11 additions & 0 deletions components/board/numato/darwin_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//go:build darwin

package numato

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

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
})
}
11 changes: 11 additions & 0 deletions components/board/numato/linux_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//go:build linux

package numato

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

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 @@ -65,23 +65,23 @@ func TestPiHardware(t *testing.T) {
err = p.SetGPIOBcom(26, false)
test.That(t, err, test.ShouldBeNil)

v, err := reader.Read(ctx, nil)
v, _, err := reader.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, v, 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)
v, _, err = reader.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, v, test.ShouldAlmostEqual, 1023, 150)

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

v, err = reader.Read(ctx, nil)
v, _, err = reader.Read(ctx, nil)
test.That(t, err, test.ShouldBeNil)
test.That(t, v, test.ShouldAlmostEqual, 0, 150)
})
Expand Down
Loading
Loading