Skip to content
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-6631 - Change RC card to use MoveOnMap typescript SDK wrapper #3574

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nicksanford
Copy link
Member

@nicksanford nicksanford commented Feb 12, 2024

Ticket

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Feb 12, 2024
@@ -1,73 +0,0 @@
import { type Client, commonApi, motionApi } from '@viamrobotics/sdk';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bye bye boilerplate 👋

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oo la la

base,
destination!.x,
destination!.y
{ planDeviationM: 0.5 },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing a lint error despite working in the browser:
https://github.com/viamrobotics/rdk/actions/runs/7873963113/job/21482471651?pr=3574#step:5:2176

Also, the typescript SDK sets this value in the MotionConfiguration type: https://github.com/viamrobotics/viam-typescript-sdk/blob/main/src/services/motion/types.ts#L88

Do I need to do something in the typescript SDK to placate the type checker?

@micheal-parks @ethanlook

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick glance it appears that those addtional props (obstacleDetectorsList, positionPollingFrequencyHz) are not marked as optional and should be? Something like:

interface MoveOnMapOptions {
  planDeviationM: number // if required
  obstacleDetectorsList?: number
  positionPollingFrequencyHz?: number
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't see the text string MoveOnMapOptions in the typescript SDK nor in the RC web app
  2. The type in question is
import pb from '../../gen/service/motion/v1/motion_pb';
...
export type MotionConfiguration = pb.MotionConfiguration.AsObject;

https://github.com/viamrobotics/viam-typescript-sdk/blob/main/src/services/motion/types.ts#L9

That object (at least in vanilla js) knows about planDeviationM: https://github.com/viamrobotics/api/blob/main/gen/js/service/motion/v1/motion_pb.js#L1956 How do I get typescript to believe that that field is a member of the MotionConfiguration type?

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Feb 12, 2024
@nicksanford nicksanford marked this pull request as ready for review February 12, 2024 17:03
Copy link
Contributor

@ethanlook ethanlook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really appreciate you coming back to do the cleanup!

Comment on lines 281 to 305
const lastPose = await slamClient.getPosition();
const destinationMM = lastPose.pose!;
destinationMM.x = destination!.x * 1000;
destinationMM.y = destination!.y * 1000;
const base = bases[0]!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor stuff] Could we change this to be a little more defensive? In general, we should avoid using the assertion operator ! (in App we have linters against this) because it's unsafe. Maybe we could use the nullish coalescing operator ?? to provide a reasonable default? Or return and display an error?

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Mar 4, 2024
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
4 participants