Skip to content

RSDK-4790: Add Readings to TypeScript SDK #191

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

Merged
merged 6 commits into from
Nov 8, 2023
Merged

RSDK-4790: Add Readings to TypeScript SDK #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f931e4
add getReadings to movement and power sensor
martha-johnston Nov 7, 2023
4d14f8e
fix tests
martha-johnston Nov 7, 2023
7e45756
remove unused value
martha-johnston Nov 7, 2023
26724a2
lint
martha-johnston Nov 7, 2023
dd822dd
address PR comments
martha-johnston Nov 7, 2023
e704f58
remove movementsensorreadings
martha-johnston Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions buf.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ deps:
- remote: buf.build
owner: viamrobotics
repository: api
commit: 9196e89cb02f40d6b38344d637fdb3fc
digest: shake256:45153f9922a876347f1b3cf7db9d35a76ff045e5a72b83fc7d6e89d1e899ff53b2d3930c36f7ce8aac704fb57f60dd9cbdba61224c8df51b1044a1ff9882ba15
commit: db4372e839fc4ddab817a38a397686fb
digest: shake256:7983589dddbde5534f0f04a4c43e1cb94f185b11f1ff6d0fe8ba50fa5fdb22711a60c009b5d428b20fd251b497e2259605a065556013a4fb9daa1f2ce6b10168
- remote: buf.build
owner: viamrobotics
repository: goutils
Expand Down
14 changes: 7 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"grpc-web": "^1.4.2",
"happy-dom": "^8.2.6",
"npm-check": "^6.0.1",
"prettier": "^2.8.1",
"prettier": "^2.8.8",
"prettier-plugin-jsdoc": "^0.4.2",
"protoc-gen-js": "^3.21.2",
"ts-protoc-gen": "^0.15.0",
Expand Down
46 changes: 20 additions & 26 deletions src/components/movement-sensor/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,29 @@ import { MovementSensorClient } from './client';

let sensor: MovementSensorClient;

const mapVals = {
toJavaScript: () => {
return 'readings';
},
};

beforeEach(() => {
RobotClient.prototype.createServiceClient = vi
.fn()
.mockImplementation(() => new MovementSensorServiceClient('mysensor'));

MovementSensorServiceClient.prototype.getReadings = vi
.fn()
.mockImplementation((_req, _md, cb) => {
cb(null, {
getReadingsMap: () => {
return {
entries: () => new Map<string, unknown>([['readings', mapVals]]),
};
},
});
});

MovementSensorServiceClient.prototype.getPosition = vi
.fn()
.mockImplementation((_req, _md, cb) => {
Expand Down Expand Up @@ -88,12 +106,7 @@ test('individual readings', async () => {

test('get readings', async () => {
await expect(sensor.getReadings()).resolves.toStrictEqual({
position: 'pos',
linearVelocity: 'vel',
linearAcceleration: 'acc',
angularVelocity: 'ang',
compassHeading: 'comp',
orientation: 'ori',
readings: 'readings',
});
});

Expand All @@ -110,25 +123,6 @@ test('get readings returns without unimplemented fields', async () => {
unimplementedError
);
await expect(sensor.getReadings()).resolves.toStrictEqual({
position: 'pos',
linearAcceleration: 'acc',
angularVelocity: 'ang',
compassHeading: 'comp',
orientation: 'ori',
readings: 'readings',
});
});

test('get readings fails on other errors', async () => {
const unexpectedError = new Error('Jank!');

MovementSensorServiceClient.prototype.getLinearVelocity = vi
.fn()
.mockImplementation((_req, _md, cb) => {
cb(unexpectedError, null);
});

await expect(sensor.getLinearVelocity()).rejects.toStrictEqual(
unexpectedError
);
await expect(sensor.getReadings()).rejects.toStrictEqual(unexpectedError);
});
43 changes: 20 additions & 23 deletions src/components/movement-sensor/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import { MovementSensorServiceClient } from '../../gen/component/movementsensor/
import type { Options, StructType } from '../../types';
import pb from '../../gen/component/movementsensor/v1/movementsensor_pb';
import { promisify, doCommandFromClient } from '../../utils';
import type { MovementSensor, MovementSensorReadings } from './movement-sensor';
import {
GetReadingsRequest,
GetReadingsResponse,
} from '../../gen/common/v1/common_pb';
import type { MovementSensor } from './movement-sensor';

/**
* A gRPC-web client for the MovementSensor component.
Expand Down Expand Up @@ -195,30 +199,23 @@ export class MovementSensorClient implements MovementSensor {
}

async getReadings(extra = {}) {
const readingFunctions = {
position: this.getPosition.bind(this),
linearVelocity: this.getLinearVelocity.bind(this),
angularVelocity: this.getAngularVelocity.bind(this),
linearAcceleration: this.getLinearAcceleration.bind(this),
compassHeading: this.getCompassHeading.bind(this),
orientation: this.getOrientation.bind(this),
};

const tasks = Object.entries(readingFunctions).flatMap(([field, func]) =>
(async () => {
try {
return [[field, await func(extra)]];
} catch (error) {
if (!(error as Error).message.includes('Unimplemented')) {
throw error;
}
return [];
}
})()
const { movementsensorService } = this;
const request = new GetReadingsRequest();
request.setName(this.name);
request.setExtra(Struct.fromJavaScript(extra));

this.options.requestLogger?.(request);

const response = await promisify<GetReadingsRequest, GetReadingsResponse>(
movementsensorService.getReadings.bind(movementsensorService),
request
);
const entries = await Promise.all(tasks);

return Object.fromEntries(entries.flat()) as MovementSensorReadings;
const result: Record<string, unknown> = {};
for (const [key, value] of response.getReadingsMap().entries()) {
result[key] = value.toJavaScript();
}
return result;
}

async doCommand(command: StructType): Promise<StructType> {
Expand Down
19 changes: 5 additions & 14 deletions src/components/movement-sensor/movement-sensor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,14 @@
import type { Orientation, StructType, Vector3 } from '../../types';
import type { Sensor } from '../sensor';
import type { Resource, Orientation, StructType, Vector3 } from '../../types';
import pb from '../../gen/component/movementsensor/v1/movementsensor_pb';

export type MovementSensorPosition = pb.GetPositionResponse.AsObject;
export type MovementSensorProperties = pb.GetPropertiesResponse.AsObject;

// https://github.com/microsoft/TypeScript/issues/15300
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export type MovementSensorReadings = {
position?: MovementSensorPosition;
linearVelocity?: Vector3;
angularVelocity?: Vector3;
linearAcceleration?: Vector3;
compassHeading?: number;
orientation?: Orientation;
};

/**
* Represents any sensor that reports information about the robot's direction,
* position, and/or speed.
*/
export interface MovementSensor extends Sensor {
export interface MovementSensor extends Resource {
/** Get linear velocity across x/y/z axes */
getLinearVelocity(extra?: StructType): Promise<Vector3>;

Expand Down Expand Up @@ -50,4 +38,7 @@ export interface MovementSensor extends Sensor {

/** Get linear acceleration across x/y/z axes */
getLinearAcceleration(extra?: StructType): Promise<Vector3>;

/** Return the readings of a sensor. */
getReadings(extra?: StructType): Promise<Record<string, unknown>>;
}
1 change: 0 additions & 1 deletion src/components/movementsensor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
export type {
MovementSensorPosition,
MovementSensorProperties,
MovementSensorReadings,
MovementSensor,
} from './movement-sensor/movement-sensor';
export { MovementSensorClient } from './movement-sensor/client';
40 changes: 20 additions & 20 deletions src/components/power-sensor/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ const testVoltage = 1.5;
const testCurrent = 1;
const testIsAc = true;

const mapVals = {
toJavaScript: () => {
return 'readings';
},
};

beforeEach(() => {
RobotClient.prototype.createServiceClient = vi
.fn()
Expand Down Expand Up @@ -42,6 +48,18 @@ beforeEach(() => {
});
});

PowerSensorServiceClient.prototype.getReadings = vi
.fn()
.mockImplementation((_req, _md, cb) => {
cb(null, {
getReadingsMap: () => {
return {
entries: () => new Map<string, unknown>([['readings', mapVals]]),
};
},
});
});

sensor = new PowerSensorClient(new RobotClient('host'), 'test-sensor');
});

Expand All @@ -63,10 +81,7 @@ test('individual readings', async () => {

test('get readings', async () => {
await expect(sensor.getReadings()).resolves.toStrictEqual({
voltage: testVoltage,
current: testCurrent,
isAc: testIsAc,
power: testPower,
readings: 'readings',
});
});

Expand All @@ -81,21 +96,6 @@ test('get readings returns without unimplemented fields', async () => {

await expect(sensor.getVoltage()).rejects.toStrictEqual(unimplementedError);
await expect(sensor.getReadings()).resolves.toStrictEqual({
current: testCurrent,
isAc: testIsAc,
power: testPower,
readings: 'readings',
});
});

test('get readings fails on other errors', async () => {
const unexpectedError = new Error('Jank!');

PowerSensorServiceClient.prototype.getPower = vi
.fn()
.mockImplementation((_req, _md, cb) => {
cb(unexpectedError, null);
});

await expect(sensor.getPower()).rejects.toStrictEqual(unexpectedError);
await expect(sensor.getReadings()).rejects.toStrictEqual(unexpectedError);
});
46 changes: 22 additions & 24 deletions src/components/power-sensor/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,14 @@ import { PowerSensorServiceClient } from '../../gen/component/powersensor/v1/pow
import type { Options, StructType } from '../../types';
import pb from '../../gen/component/powersensor/v1/powersensor_pb';
import { promisify, doCommandFromClient } from '../../utils';
import type { PowerSensor, PowerSensorReadings } from './power-sensor';
import {
GetReadingsRequest,
GetReadingsResponse,
} from '../../gen/common/v1/common_pb';
import type { PowerSensor } from './power-sensor';

/**
* A gRPC-web client for the MovementSensor component.
* A gRPC-web client for the PowerSensor component.
*
* @group Clients
*/
Expand Down Expand Up @@ -76,29 +80,23 @@ export class PowerSensorClient implements PowerSensor {
}

async getReadings(extra = {}) {
const readings: Record<string, any> = {};
try {
[readings['voltage'], readings['isAc']] = await this.getVoltage(extra);
} catch (error) {
if (!(error as Error).message.includes('Unimplemented')) {
throw error;
}
}
try {
[readings['current'], readings['isAc']] = await this.getCurrent(extra);
} catch (error) {
if (!(error as Error).message.includes('Unimplemented')) {
throw error;
}
}
try {
readings['power'] = await this.getPower(extra);
} catch (error) {
if (!(error as Error).message.includes('Unimplemented')) {
throw error;
}
const { powersensorService } = this;
const request = new GetReadingsRequest();
request.setName(this.name);
request.setExtra(Struct.fromJavaScript(extra));

this.options.requestLogger?.(request);

const response = await promisify<GetReadingsRequest, GetReadingsResponse>(
powersensorService.getReadings.bind(powersensorService),
request
);

const result: Record<string, unknown> = {};
for (const [key, value] of response.getReadingsMap().entries()) {
result[key] = value.toJavaScript();
}
return readings as PowerSensorReadings;
return result;
Copy link
Member

@DTCurrie DTCurrie Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we using Record<string, unknown> here instead of the PowerSensorReadings type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I thought PowerSensorReadings was a type we were using before there was an API for powersensor.GetReadings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since sensor did not have that and now sensors and power sensor are using the same api

Copy link
Member

@DTCurrie DTCurrie Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We relied on those types in the RC, so now we see these errors:

Error: Type 'unknown' is not assignable to type 'number | undefined'. (ts)
    const readings = await powerSensorClient.getReadings();
    voltageValue = readings.voltage;
    currentValue = readings.current;


/__w/rdk/rdk/web/frontend/src/components/power-sensor/index.svelte:28:5
Error: Type 'unknown' is not assignable to type 'number | undefined'. (ts)
    voltageValue = readings.voltage;
    currentValue = readings.current;
    powerValue = readings.power;


/__w/rdk/rdk/web/frontend/src/components/power-sensor/index.svelte:29:5
Error: Type 'unknown' is not assignable to type 'number | undefined'. (ts)
    currentValue = readings.current;
    powerValue = readings.power;
  } catch (error) {

With these changes we can no longer rely on the SDK to return us current, voltage, or power as a part of the readings response object. Also we no longer know if those fields exist on the response object, and since they are type to unknown we have no idea if they are numbers, strings, objects, etc...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh sorry I had no idea, are you seeing this issue with movement sensor as well? or is it still working

Copy link
Member

Choose a reason for hiding this comment

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

I haven't even tested anything out, the RC is failing it's build checks right now with an SDK version bump. These changes are not passing our standard checks.

}

async doCommand(command: StructType): Promise<StructType> {
Expand Down
9 changes: 2 additions & 7 deletions src/components/power-sensor/power-sensor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
import type { StructType } from '../../types';
import type { Sensor } from '../sensor';

export type PowerSensorReadings = {
voltage?: number;
current?: number;
isAc?: boolean;
power?: number;
};

/** Represents any sensor that reports voltage, current, and/or power */
export interface PowerSensor extends Sensor {
/** Get Voltage in volts and a boolean that returns true if AC */
Expand All @@ -16,4 +9,6 @@ export interface PowerSensor extends Sensor {
getCurrent(extra?: StructType): Promise<readonly [number, boolean]>;
/** Get Power in watts */
getPower(extra?: StructType): Promise<number>;
/** Return the readings of a sensor. */
getReadings(extra?: StructType): Promise<Record<string, unknown>>;
}
5 changes: 1 addition & 4 deletions src/components/powersensor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,2 @@
export type {
PowerSensorReadings,
PowerSensor,
} from './power-sensor/power-sensor';
export type { PowerSensor } from './power-sensor/power-sensor';
export { PowerSensorClient } from './power-sensor/client';
Loading