fix: update monitor settings only if it's changed

Closes #375

Signed-off-by: Akos Kitta <a.kitta@arduino.cc>

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",

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,16 @@ 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
+        }: ${JSON.stringify(this.currentPortConfigSnapshot)}`
+      );
       this.startMessagesHandlers();
       this.logger.info(
         `started monitor to ${this.port?.address} using ${this.port?.protocol}`
@@ -518,16 +538,120 @@ 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 ${JSON.stringify(curr)}.`
+        );
+      }
+      const { path, value } = curr;
+      const [, index] = path;
+      if (!this.currentPortConfigSnapshot?.settingsList) {
+        throw new Error(
+          `'settingsList' is missing from current port config snapshot: ${JSON.stringify(
+            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: 'replace';
+    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

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"