From 0a67591352621d72eb2f09573279f176227ab090 Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Tue, 22 Nov 2022 19:00:34 +0100 Subject: [PATCH] fix: update monitor settings only if it's changed Closes #375 Signed-off-by: Akos Kitta --- arduino-ide-extension/package.json | 1 + .../src/node/monitor-service.ts | 122 +++++++++++++++++- yarn.lock | 5 + 3 files changed, 127 insertions(+), 1 deletion(-) diff --git a/arduino-ide-extension/package.json b/arduino-ide-extension/package.json index 0daf84ced..6822ec4cf 100644 --- a/arduino-ide-extension/package.json +++ b/arduino-ide-extension/package.json @@ -73,6 +73,7 @@ "google-protobuf": "^3.20.1", "hash.js": "^1.1.7", "js-yaml": "^3.13.1", + "just-diff": "^5.1.1", "jwt-decode": "^3.1.2", "keytar": "7.2.0", "lodash.debounce": "^4.0.8", diff --git a/arduino-ide-extension/src/node/monitor-service.ts b/arduino-ide-extension/src/node/monitor-service.ts index 2e6b55a85..3b751512b 100644 --- a/arduino-ide-extension/src/node/monitor-service.ts +++ b/arduino-ide-extension/src/node/monitor-service.ts @@ -1,6 +1,7 @@ import { ClientDuplexStream } from '@grpc/grpc-js'; import { Disposable, Emitter, ILogger } from '@theia/core'; import { inject, named, postConstruct } from '@theia/core/shared/inversify'; +import { diff, Operation } from 'just-diff'; import { Board, Port, Status, Monitor } from '../common/protocol'; import { EnumerateMonitorPortSettingsRequest, @@ -80,6 +81,15 @@ export class MonitorService extends CoreClientAware implements Disposable { private readonly port: Port; private readonly monitorID: string; + /** + * The lightweight representation of the port configuration currently in use for the running monitor. + * IDE2 stores this object after starting the monitor. On every monitor settings change request, IDE2 compares + * the current config with the new settings, and only sends the diff as the new config to overcome https://github.com/arduino/arduino-ide/issues/375. + */ + private currentPortConfigSnapshot: + | MonitorPortConfiguration.AsObject + | undefined; + constructor( @inject(MonitorServiceFactoryOptions) options: MonitorServiceFactoryOptions ) { @@ -211,6 +221,14 @@ export class MonitorService extends CoreClientAware implements Disposable { monitorRequest ); if (wroteToStreamSuccessfully) { + // Only store the config, if the monitor has successfully started. + this.currentPortConfigSnapshot = MonitorPortConfiguration.toObject( + false, + config + ); + this.logger.info( + `Using port configuration for ${this.port.protocol}:${this.port.address}: ${this.currentPortConfigSnapshot}` + ); this.startMessagesHandlers(); this.logger.info( `started monitor to ${this.port?.address} using ${this.port?.protocol}` @@ -518,16 +536,118 @@ export class MonitorService extends CoreClientAware implements Disposable { if (!this.duplex) { return Status.NOT_CONNECTED; } + + const diffConfig = this.maybeUpdatePortConfigSnapshot(config); + if (!diffConfig) { + this.logger.info( + `No port configuration changes have been detected. No need to send configure commands to the running monitor ${this.port.protocol}:${this.port.address}.` + ); + return Status.OK; + } + const coreClient = await this.coreClient; const { instance } = coreClient; + this.logger.info( + `Sending monitor request with new port configuration: ${JSON.stringify( + MonitorPortConfiguration.toObject(false, diffConfig) + )}` + ); const req = new MonitorRequest(); req.setInstance(instance); - req.setPortConfiguration(config); + req.setPortConfiguration(diffConfig); this.duplex.write(req); return Status.OK; } + /** + * Function to calculate a diff between the `otherPortConfig` argument and the `currentPortConfigSnapshot`. + * + * If the current config snapshot and the snapshot derived from `otherPortConfig` are the same, no snapshot update happens, + * and the function returns with undefined. Otherwise, the current snapshot config value will be updated from the snapshot + * derived from the `otherPortConfig` argument, and this function returns with a `MonitorPortConfiguration` instance + * representing only the difference between the two snapshot configs to avoid sending unnecessary monitor to configure commands to the CLI. + * See [#1703 (comment)](https://github.com/arduino/arduino-ide/pull/1703#issuecomment-1327913005) for more details. + */ + private maybeUpdatePortConfigSnapshot( + otherPortConfig: MonitorPortConfiguration + ): MonitorPortConfiguration | undefined { + const otherPortConfigSnapshot = MonitorPortConfiguration.toObject( + false, + otherPortConfig + ); + if (!this.currentPortConfigSnapshot) { + throw new Error( + `The current port configuration object was undefined when tried to merge in ${JSON.stringify( + otherPortConfigSnapshot + )}.` + ); + } + + const snapshotDiff = diff( + this.currentPortConfigSnapshot, + otherPortConfigSnapshot + ); + if (!snapshotDiff.length) { + return undefined; + } + + const diffConfig = snapshotDiff.reduce((acc, curr) => { + if (!this.isValidMonitorPortSettingChange(curr)) { + throw new Error( + `Expected only 'replace' operation: a 'value' change in the 'settingsList'. Calculated diff a ${JSON.stringify( + snapshotDiff + )} between ${JSON.stringify( + this.currentPortConfigSnapshot + )} and ${JSON.stringify( + otherPortConfigSnapshot + )} snapshots. Current JSON-patch entry was ${curr}.` + ); + } + const { path, value } = curr; + const [, index] = path; + if (!this.currentPortConfigSnapshot?.settingsList) { + throw new Error( + `'settingsList' is missing from current port config snapshot: ${this.currentPortConfigSnapshot}` + ); + } + const changedSetting = this.currentPortConfigSnapshot.settingsList[index]; + const setting = new MonitorPortSetting(); + setting.setValue(value); + setting.setSettingId(changedSetting.settingId); + acc.addSettings(setting); + return acc; + }, new MonitorPortConfiguration()); + + this.currentPortConfigSnapshot = otherPortConfigSnapshot; + this.logger.info( + `Updated the port configuration for ${this.port.protocol}:${ + this.port.address + }: ${JSON.stringify(this.currentPortConfigSnapshot)}` + ); + return diffConfig; + } + + private isValidMonitorPortSettingChange(entry: { + op: Operation; + path: (string | number)[]; + value: unknown; + }): entry is { + op: 'add'; + path: ['settingsList', number, string]; + value: string; + } { + const { op, path, value } = entry; + return ( + op === 'replace' && + path.length === 3 && + path[0] === 'settingsList' && + typeof path[1] === 'number' && + path[2] === 'value' && + typeof value === 'string' + ); + } + /** * Starts the necessary handlers to send and receive * messages to and from the frontend and the running monitor diff --git a/yarn.lock b/yarn.lock index 25a47cf55..d6d879fb3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9391,6 +9391,11 @@ jsprim@^1.2.2: array-includes "^3.1.5" object.assign "^4.1.2" +just-diff@^5.1.1: + version "5.1.1" + resolved "https://registry.yarnpkg.com/just-diff/-/just-diff-5.1.1.tgz#8da6414342a5ed6d02ccd64f5586cbbed3146202" + integrity sha512-u8HXJ3HlNrTzY7zrYYKjNEfBlyjqhdBkoyTVdjtn7p02RJD5NvR8rIClzeGA7t+UYP1/7eAkWNLU0+P3QrEqKQ== + jwt-decode@^3.1.2: version "3.1.2" resolved "https://registry.yarnpkg.com/jwt-decode/-/jwt-decode-3.1.2.tgz#3fb319f3675a2df0c2895c8f5e9fa4b67b04ed59"