RSDK-2692 - Add collectors for MovementSensor LinearAcceleration and Orientation #2258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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!
|
|
AaronCasas
reviewed
Apr 21, 2023
| return v, err | ||
| }) | ||
| registerCollector("Orientation", func(ctx context.Context, ms MovementSensor) (interface{}, error) { | ||
| v, err := ms.Orientation(ctx, make(map[string]interface{})) |
There was a problem hiding this comment.
Since Orientation returns an interface, and these are persisted by converting them to a proto struct using this function, which requires the input to be a struct, this unfortunately won't work. This output will need to wrapped in/converted to some concrete type. The proto defined OrientationResponse type here should work
There was a problem hiding this comment.
This did seem to work. Orientation output looked like this:
{"Imag":0,"Jmag":0,"Kmag":0,"MetadataIndex":3,"Real":1,"TimeReceived":{"seconds":1682100838,"nanos":271460000},"TimeRequested":{"seconds":1682100838,"nanos":271455000}}
There was a problem hiding this comment.
Interesting - I must be mis-remembering. Looks like the concrete type it's using is a quat.Number then. I would still recommend wrapping in that proto type so that this is consistent (should be an easy mapping using the orientations .AxisAngles(), which seems to be the representation the proto interface uses)
There was a problem hiding this comment.
Though actually I see Readings uses this quat value too. I think this is definitely fine for now, but as a team we should revisit the pattern here soon (put time on Thursday to discuss)
There was a problem hiding this comment.
Thanks for the explanation, and sounds good!
AaronCasas
approved these changes
Apr 21, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I tested this manually by creating a robot with a fake movement sensor and capturing Position, LinearAcceleration, and Orientation. I didn't see a place to add unit test coverage.
Here is the exported sensor data.
sensor_data.zip
Here are the corresponding app changes: https://github.com/viamrobotics/app/pull/1503