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.

Sign up for GitHub

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

Jump to bottom

RSDK-6171-add-client-api-for-debugging-grpc-calls #384

Merged
merged 10 commits into from
Oct 14, 2024
Merged

RSDK-6171-add-client-api-for-debugging-grpc-calls #384

merged 10 commits into from
Oct 14, 2024

Conversation

purplenicole730
Copy link
Member

Working example:

  await motor1Client.setPower(0.5);
  let opts = VIAM.enableDebugLogging('this-is-nicole');
  await motor1Client.setPower(0.75, {}, opts);
  motor1Client.callOptions = VIAM.enableDebugLogging('motor-opts');
  await motor1Client.setPower(1);
  await motor1Client.setPower(0.25, {}, opts);
  VIAM.disableDebugLogging(motor1Client.callOptions);
  await motor1Client.setPower(0);
  await motor1Client.setPower(0.3, {}, opts);

debug rdk.resource_manager.rdk:component:motor/motor1 fake/motor.go:212 Motor SetPower 0.300000 traceKey this-is-nicole
debug rdk.resource_manager.rdk:component:motor/motor1 fake/motor.go:212 Motor SetPower 0.250000 traceKey this-is-nicole
debug rdk.resource_manager.rdk:component:motor/motor1 fake/motor.go:212 Motor SetPower 1.000000 traceKey motor-opts
debug rdk.resource_manager.rdk:component:motor/motor1 fake/motor.go:212 Motor SetPower 0.750000 traceKey this-is-nicole

purplenicole730
purplenicole730 commented Oct 10, 2024
View reviewed changes
src/components/base/base.ts
@@ -51,7 +51,7 @@ export interface Base extends Resource {
stop(extra?: Struct): Promise<void>;

/** Return true if the base is in motion. */
isMoving(extra?: Struct): Promise<boolean>;
isMoving(): Promise<boolean>;
Copy link
Member Author

@purplenicole730 purplenicole730 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is technically a breaking change. None of the other isMoving functions have an extra parameter and BaseClient.isMoving was not using it as well, leading me to believe it was mistakenly included. I thought to remove it, but let me know if I should not do this.

@purplenicole730 purplenicole730 marked this pull request as ready for review October 10, 2024 16:31
@purplenicole730 purplenicole730 requested a review from a team as a code owner October 10, 2024 16:31
njooma
njooma reviewed Oct 11, 2024
View reviewed changes
src/utils.ts
Comment on lines +71 to +88
export const addMetadata = (
key: string,
value: string,
opts?: CallOptions
): CallOptions => {
const finalOpts =
opts ?? ({ headers: {} as Record<string, string> } as CallOptions);
(finalOpts.headers as Record<string, string>)[key] = value;
return finalOpts;
};

export const deleteMetadata = (opts: CallOptions, key: string): void => {
const { [key]: _, ...remainingHeaders } = opts.headers as Record<
string,
string
>;
opts.headers = remainingHeaders;
};
Copy link
Member

Choose a reason for hiding this comment

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

Are these helpers used? Or just for potential users ?

Copy link
Member Author

@purplenicole730 purplenicole730 Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These two helpers not being used right now, but these would be good to include for potential usage. This was also introduced in the Python SDK, so I thought it would be good to keep things similar. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave it in personally. We're now offering users access to passing metadata so we might as well make it easy for them to do so.

Copy link
Member

Choose a reason for hiding this comment

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

we can leave in. i was just wondering if i missed something

@purplenicole730 purplenicole730 merged commit 1f8e7f4 into viamrobotics:main Oct 14, 2024
3 checks passed
@purplenicole730 purplenicole730 deleted the RSDK-6171-add-client-api-for-debugging-grpc-calls branch October 14, 2024 18:41
@purplenicole730 purplenicole730 restored the RSDK-6171-add-client-api-for-debugging-grpc-calls branch October 28, 2024 15:46
@purplenicole730 purplenicole730 deleted the RSDK-6171-add-client-api-for-debugging-grpc-calls branch October 28, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuqdog stuqdog stuqdog approved these changes

@njooma njooma njooma approved these changes

@gloriacai01 gloriacai01 Awaiting requested review from gloriacai01 gloriacai01 is a code owner automatically assigned from viamrobotics/sdk-netcode

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@purplenicole730 @njooma @stuqdog