RSDK-7152 Analog Reader API rdk changes

[oliviamiller]

Numato was weird due to the driver being able to support multiple numato versions that all have different ADCs. the driver will search for the product id and if its one of the 4 that we "officially" support we return the correct values, otherwise we just return zeros.

[github-actions]

Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!

component	function
base	IsMoving
board	GPIOPinByName
camera	Properties
encoder	Properties
motor	IsMoving
sensor	Readings
servo	Position
arm	EndPosition
audio	MediaProperties
gantry	Lengths
gripper	IsMoving
input_controller	Controls
movement_sensor	LinearAcceleration
power_sensor	Power
pose_tracker	Poses
motion	GetPose
vision	ClassificationsFromCamera

[penguinland]

A couple small but important bugs, and then a whole bunch of optional suggestions and discussion questions that don't have obvious answers (so, maybe ignore those and leave things as-is).

[penguinland]

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.

[oliviamiller]

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

[penguinland]

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. 🤷

[oliviamiller]

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

[penguinland]

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.

[oliviamiller]

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.

[penguinland]

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

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

[penguinland]

...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).

[oliviamiller]

yeah thats fair ill just keep it in volts/bit

[penguinland]

All my feedback is either optional or trivial. LGTM!

[penguinland]

👍 Thanks for this comment!

[penguinland]

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.

[oliviamiller]

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

[penguinland]

Change is to are.

[penguinland]

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.

[penguinland]

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

[penguinland]

This makes it look like the point of the mutex is to protect analogVal. If that's the case, we should also lock the mutex before line 65. I wonder if lines 70-71 (locking and deferring unlocking the mutex) should just be moved to the top of the function for simplicity.

Another option is to change Read to never return as.analogVal itself, but always to create a new board.AnalogValue struct instead and copy data into that. Then, as.analogVal is constant after this line, and we don't need the mutex at all.

[oliviamiller]

did the second option and removed the mutex

[penguinland]

I think there's a race condition here: if someone calls Read() before the background goroutine has gotten to here, the min/max/stepsize will all be 0. I suspect this paragraph should happen in SmoothAnalogReader() instead of here, so the entire struct is initialized before it can be used.