[RSDK-7335] Remove Close() from and Add Write() to Analog interface in board #3809
Conversation
viambot
added
the
safe to test
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
randhid
changed the title
viambot
added
safe to test
| @@ -360,16 +360,16 @@ type analog struct { | |||
| pin string | |||
| } | |||
|
|
|||
| func (ar *analog) Read(ctx context.Context, extra map[string]interface{}) (int, error) { | |||
| res, err := ar.b.doSendReceive(ctx, fmt.Sprintf("adc read %s", ar.pin)) | |||
| func (a *analog) Read(ctx context.Context, extra map[string]interface{}) (int, error) { | |||
There was a problem hiding this comment.
I think the numato code can either not use the analog smoother or implement a Close more gracefully. We'll handle this driver in our next bug bash.
| @@ -65,3 +66,7 @@ func (mar *MCP3008AnalogReader) Read(ctx context.Context, extra map[string]inter | |||
| func (mar *MCP3008AnalogReader) Close(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (mar *MCP3008AnalogReader) Write(ctx context.Context, value int, extra map[string]interface{}) error { | |||
There was a problem hiding this comment.
The current flow for the pi/generic linux:
- make a MCP3008 analog helper,
- pass that into AnalogSMoother, it needs Close function implemented.
- use that analog smoother to fulfill the Read and Write methods in analog for board's Analog struct.
This can probably be better. Probably by obliterating it.
randhid
changed the title
viambot
added
safe to test
testutils/inject/analog.go
Outdated
|
|
||
| // Write calls the injected Write or the real version. | ||
| func (a *Analog) Write(ctx context.Context, value int, extra map[string]interface{}) error { | ||
| a.readCap = []interface{}{ctx} |
There was a problem hiding this comment.
Looking at the documentation you left for WriteCap, I suspect we should also store the value here.
...though TBH I don't understand when you'd ever need to call WriteCap() or ReadCap() and don't really get why this code exists at all. The one use of ReadCap in our codebase is used to assert that the context passed into a function really is the context passed in, which seems like kind of a useless test.
There was a problem hiding this comment.
I'm not going to change the pattern of inject implementations as part of this pr. They could probably be better, but they do their job of injecting interfaces for testing, and I'm happy with that.
There was a problem hiding this comment.
I did not mean to change the pattern; I don't think you're following the pre-existing pattern here. For example, in testutils/inject/gpio_pin.go, the captures store all of the arguments except extra, which would include storing value here.
I wonder if there's some other reason not to store value and to set things up the way you did, but I don't see that reason myself, partly because I don't understand why the pattern has been set up this way in the first place.
There was a problem hiding this comment.
Ah I see, no you're right, we should store value here. tbh I do think our injects could be better. The Cap functions are usually to capture the input arguments to the injected function to be checked against in server-client code tests. But to be frank our team almost never has a need to use the Cap functions in testing the drivers, because we care about testing the behaviour of inputs not validating that we've sent the inputs. The Cap functions are usually used in server tests for resource APIs.
... the inject package could be better and lessconfusing overall, maybe one day.
viambot
added
safe to test
viambot
added
safe to test
Removes the Close function from the analog interface, changes implementation of the raspberry pi to directly use the analog smoother rather than store it as a board.Analog in its struct.
Note: I have only implemented the
Writemethod on the interface in the analog struct in board.go. This function does not yet go through the server or client in th main board file, which was ticketed separately. That will need to go over the wire using theWriteAnalogrpc.