-
Notifications
You must be signed in to change notification settings - Fork 22
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-7151] Update analog reader responses to include accuracy data #237
[RSDK-7151] Update analog reader responses to include accuracy data #237
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wandering by, do with these comments what you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drew's comments can clean up the code a bit, but otherwise, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've resolved my tension between statuses holding just an int32 while the analog responses now hold 4 pieces of data: keep statuses as just an int32, and only change the return type when reading analogs! This got rid of half the things Drew suggested needed fixing, and I like the design better. Take another look!
@@ -103,8 +103,11 @@ ::grpc::Status BoardServer::ReadAnalogReader( | |||
extra = struct_to_map(request->extra()); | |||
} | |||
|
|||
const Board::analog_value result = board->read_analog(request->analog_reader_name(), extra); | |||
response->set_value(result); | |||
const Board::analog_response result = board->read_analog(request->analog_reader_name(), extra); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I named the type analog_response
, but named all the variables result
. Should the type be analog_result
instead? I'm unsure...
The CI build is failing with |
@penguinland not sure what's going on with this but it's not a problem on your end; it's popping up in other PRs as well. I'll investigate. Edit: okay I'm pretty sure I have a fix, confirming now then will merge it into main. |
276bfe2
to
2d655c8
Compare
Yup, I rebased off of |
Surprisingly, we don't need a protobuf update (from viamrobotics/api#492) because we're already using the new protobufs and have just been ignoring the extra data they hold!
Everything compiles and the tests all pass, though I haven't tried it out more than that.