RSDK-1979 use new motion service wrapper #2235
Merged
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
viambot
added
safe to test
maximpertsov
requested review from
nicksanford
and removed request for
micheal-parks and
nicksanford
nicksanford
requested changes
Apr 18, 2023
web/frontend/src/components/slam.vue
Outdated
| code: status.code, | ||
| metadata: status.metadata, | ||
| }; | ||
| getPointCloudMap.on('status', (status?) => { |
There was a problem hiding this comment.
- what does it mean if
statusis undefined? - why is this changing?
There was a problem hiding this comment.
this is no longer changing - I think I simply made a mistake here, I can't reproduce the linting error I got before that led me to make this change.
There was a problem hiding this comment.
repeated my smoke test from before and saw the same behavior after reverting this change.
| reject(error); | ||
| } | ||
| }); | ||
| getPointCloudMap.on('end', (end: { code: number }) => { | ||
| getPointCloudMap.on('end', (end?: { code: number }) => { |
There was a problem hiding this comment.
- what does it mean if
endis undefined? - why is this changing?
There was a problem hiding this comment.
- what does it mean if end is undefined?
This means the response was closed without a valid grpc-status. This can happen if the server shuts down or receives a malformed request.
- why is this changing?
My best guess is that types were mis-configured on the typescript side - we recently made a change to address this. In this case, we were properly handing status being undefined, but just did not have the correct type signature to reflect this.
EDIT: this signature is from generated pb service code - I can't find any record of the signature being different.
| @@ -84,7 +81,7 @@ const fetchSLAMPose = (name: string): Promise<commonApi.Pose> => { | |||
| props.client.slamService.getPosition( | |||
| req, | |||
| new grpc.Metadata(), | |||
| (error: ServiceError, res: slamApi.GetPositionResponse): void => { | |||
| (error: ServiceError | null, res: slamApi.GetPositionResponse | null): void => { | |||
There was a problem hiding this comment.
is it fair to say that one & only one of error and res will be defined?
There was a problem hiding this comment.
this is unclear
in the logic below it looks like we are handling this case a "successful" response is null. (i.e. we are calling resp! instead of just resp).
viambot
added
safe to test
maximpertsov
force-pushed
the
RSDK-1979-motion
branch
from
91b5452 to
cd00be2
Compare
viambot
added
safe to test
maximpertsov
commented
Apr 18, 2023
| @@ -56,7 +56,7 @@ const fetchSLAMMap = (name: string): Promise<Uint8Array> => { | |||
| const chunk = res.getPointCloudPcdChunk_asU8(); | |||
| chunks.push(chunk); | |||
| }); | |||
| getPointCloudMap.on('status', (status: { code: number, details: string, metadata: string }) => { | |||
| getPointCloudMap.on('status', (status: { code: number, details: string, metadata: grpc.Metadata }) => { | |||
There was a problem hiding this comment.
okay, now this is just a typing change - no logic changes. see #2235 (comment) for more context
|
@nicksanford |
In that case I have no objections & can be removed as a reviewer. Thanks for checking in! |
viambot
added
safe to test
maximpertsov
requested review from
cheukt and
nicksanford
and removed request for
nicksanford
Cool, though now need to approve since you requested changes earlier - I can't take you off once you do that. |
|
nicksanford
approved these changes
Apr 18, 2023
There was a problem hiding this comment.
Approving given my review is no longer required
#2235 (comment)
micheal-parks
approved these changes
Apr 20, 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.
Depends on viamrobotics/viam-typescript-sdk#55
Supersedes: #2189 - go there to see discussions