fix: invalid custom board option handling in FQBN

Closes #1588 Signed-off-by: Akos Kitta <[email protected]>
arduino · Feb 8, 2024 · 7924adf · 7924adf
1 parent 6e7b540
commit 7924adf
Show file tree

Hide file tree

Showing 10 changed files with 277 additions and 131 deletions.
diff --git a/arduino-ide-extension/package.json b/arduino-ide-extension/package.json
@@ -72,6 +72,7 @@
     "fast-json-stable-stringify": "^2.1.0",
     "fast-safe-stringify": "^2.1.1",
     "filename-reserved-regex": "^2.0.0",
+    "fqbn": "^1.0.3",
     "glob": "^7.1.6",
     "google-protobuf": "^3.20.1",
     "hash.js": "^1.1.7",

diff --git a/arduino-ide-extension/src/browser/boards/boards-data-store.ts b/arduino-ide-extension/src/browser/boards/boards-data-store.ts
@@ -12,6 +12,7 @@ import { ILogger } from '@theia/core/lib/common/logger';
 import { deepClone, deepFreeze } from '@theia/core/lib/common/objects';
 import type { Mutable } from '@theia/core/lib/common/types';
 import { inject, injectable, named } from '@theia/core/shared/inversify';
+import { FQBN } from 'fqbn';
 import {
   BoardDetails,
   BoardsService,
@@ -29,6 +30,14 @@ import type {
 import { NotificationCenter } from '../notification-center';
 import { BoardsServiceProvider } from './boards-service-provider';
 
+export interface SelectConfigOptionParams {
+  readonly fqbn: string;
+  readonly optionsToUpdate: readonly Readonly<{
+    option: string;
+    selectedValue: string;
+  }>[];
+}
+
 @injectable()
 export class BoardsDataStore
   implements
@@ -64,7 +73,12 @@ export class BoardsDataStore
     this.toDispose.pushAll([
       this.boardsServiceProvider.onBoardsConfigDidChange((event) => {
         if (isBoardIdentifierChangeEvent(event)) {
-          this.updateSelectedBoardData(event.selectedBoard?.fqbn);
+          this.updateSelectedBoardData(
+            event.selectedBoard?.fqbn,
+            // If the change event comes from toolbar and the FQBN contains custom board options, change the currently selected options
+            // https://github.com/arduino/arduino-ide/issues/1588
+            event.reason === 'toolbar'
+          );
         }
       }),
       this.notificationCenter.onPlatformDidInstall(async ({ item }) => {
@@ -125,9 +139,22 @@ export class BoardsDataStore
   }
 
   private async updateSelectedBoardData(
-    fqbn: string | undefined
+    fqbn: string | undefined,
+    updateConfigOptions = false
   ): Promise<void> {
     this._selectedBoardData = await this.getSelectedBoardData(fqbn);
+    if (fqbn && updateConfigOptions) {
+      const { options } = new FQBN(fqbn);
+      if (options) {
+        const optionsToUpdate = Object.entries(options).map(([key, value]) => ({
+          option: key,
+          selectedValue: value,
+        }));
+        const params = { fqbn, optionsToUpdate };
+        await this.selectConfigOption(params);
+        this._selectedBoardData = await this.getSelectedBoardData(fqbn); // reload the updated data
+      }
+    }
   }
 
   onStop(): void {
@@ -168,7 +195,7 @@ export class BoardsDataStore
       return undefined;
     }
     const { configOptions } = await this.getData(fqbn);
-    return ConfigOption.decorate(fqbn, configOptions);
+    return new FQBN(fqbn).withConfigOptions(...configOptions).toString();
   }
 
   async getData(fqbn: string | undefined): Promise<BoardsDataStore.Data> {
@@ -213,36 +240,41 @@ export class BoardsDataStore
     return true;
   }
 
-  async selectConfigOption({
-    fqbn,
-    option,
-    selectedValue,
-  }: {
-    fqbn: string;
-    option: string;
-    selectedValue: string;
-  }): Promise<boolean> {
-    const data = deepClone(await this.getData(fqbn));
-    const { configOptions } = data;
-    const configOption = configOptions.find((c) => c.option === option);
-    if (!configOption) {
+  async selectConfigOption(params: SelectConfigOptionParams): Promise<boolean> {
+    const { fqbn, optionsToUpdate } = params;
+    if (!optionsToUpdate.length) {
       return false;
     }
-    let updated = false;
-    for (const value of configOption.values) {
-      const mutable: Mutable<ConfigValue> = value;
-      if (mutable.value === selectedValue) {
-        mutable.selected = true;
-        updated = true;
-      } else {
-        mutable.selected = false;
+
+    const mutableData = deepClone(await this.getData(fqbn));
+    let didChange = false;
+
+    for (const { option, selectedValue } of optionsToUpdate) {
+      const { configOptions } = mutableData;
+      const configOption = configOptions.find((c) => c.option === option);
+      if (configOption) {
+        let updated = false;
+        for (const value of configOption.values) {
+          const mutable: Mutable<ConfigValue> = value;
+          if (mutable.value === selectedValue) {
+            mutable.selected = true;
+            updated = true;
+          } else {
+            mutable.selected = false;
+          }
+        }
+        if (updated) {
+          didChange = true;
+        }
       }
     }
-    if (!updated) {
+
+    if (!didChange) {
       return false;
     }
-    await this.setData({ fqbn, data });
-    this.fireChanged({ fqbn, data });
+
+    await this.setData({ fqbn, data: mutableData });
+    this.fireChanged({ fqbn, data: mutableData });
     return true;
   }
 

diff --git a/arduino-ide-extension/src/browser/boards/boards-service-provider.ts b/arduino-ide-extension/src/browser/boards/boards-service-provider.ts
@@ -136,6 +136,9 @@ export interface BoardListUIActions {
 }
 export type BoardListUI = BoardList & BoardListUIActions;
 
+export type BoardsConfigChangeEventUI = BoardsConfigChangeEvent &
+  Readonly<{ reason?: UpdateBoardsConfigReason }>;
+
 @injectable()
 export class BoardListDumper implements Disposable {
   @inject(OutputChannelManager)
@@ -190,7 +193,7 @@ export class BoardsServiceProvider
   private _ready = new Deferred<void>();
 
   private readonly boardsConfigDidChangeEmitter =
-    new Emitter<BoardsConfigChangeEvent>();
+    new Emitter<BoardsConfigChangeEventUI>();
   readonly onBoardsConfigDidChange = this.boardsConfigDidChangeEmitter.event;
 
   private readonly boardListDidChangeEmitter = new Emitter<BoardListUI>();
@@ -384,7 +387,7 @@ export class BoardsServiceProvider
       this._boardsConfig.selectedPort = event.selectedPort;
     }
 
-    this.boardsConfigDidChangeEmitter.fire(event);
+    this.boardsConfigDidChangeEmitter.fire(Object.assign(event, { reason }));
     this.refreshBoardList();
     this.saveState();
     return true;

diff --git a/arduino-ide-extension/src/browser/contributions/boards-data-menu-updater.ts b/arduino-ide-extension/src/browser/contributions/boards-data-menu-updater.ts
@@ -87,8 +87,7 @@ export class BoardsDataMenuUpdater extends Contribution {
                   execute: () =>
                     this.boardsDataStore.selectConfigOption({
                       fqbn,
-                      option,
-                      selectedValue: value.value,
+                      optionsToUpdate: [{ option, selectedValue: value.value }],
                     }),
                   isToggled: () => value.selected,
                 };

diff --git a/arduino-ide-extension/src/browser/contributions/ino-language.ts b/arduino-ide-extension/src/browser/contributions/ino-language.ts
@@ -9,10 +9,9 @@ import {
   BoardIdentifier,
   BoardsService,
   ExecutableService,
-  assertSanitizedFqbn,
   isBoardIdentifierChangeEvent,
-  sanitizeFqbn,
 } from '../../common/protocol';
+import { FQBN } from 'fqbn';
 import {
   defaultAsyncWorkers,
   maxAsyncWorkers,
@@ -159,14 +158,11 @@ export class InoLanguage extends SketchContribution {
       this.notificationCenter.onDidReinitialize(() => forceRestart()),
       this.boardDataStore.onDidChange((event) => {
         if (this.languageServerFqbn) {
-          const sanitizedFqbn = sanitizeFqbn(this.languageServerFqbn);
-          if (!sanitizeFqbn) {
-            throw new Error(
-              `Failed to sanitize the FQBN of the running language server. FQBN with the board settings was: ${this.languageServerFqbn}`
-            );
-          }
-          const matchingChange = event.changes.find(
-            (change) => change.fqbn === sanitizedFqbn
+          const sanitizedFqbn = new FQBN(this.languageServerFqbn).sanitize();
+          // The incoming FQBNs might contain custom boards configs, sanitize them before the comparison.
+          // https://github.com/arduino/arduino-ide/pull/2113#pullrequestreview-1499998328
+          const matchingChange = event.changes.find((change) =>
+            new FQBN(change.fqbn).sanitize().equals(sanitizedFqbn)
           );
           const { boardsConfig } = this.boardsServiceProvider;
           if (
@@ -228,7 +224,6 @@ export class InoLanguage extends SketchContribution {
         }
         return;
       }
-      assertSanitizedFqbn(fqbn);
       const fqbnWithConfig = await this.boardDataStore.appendConfigToFqbn(fqbn);
       if (!fqbnWithConfig) {
         throw new Error(

diff --git a/arduino-ide-extension/src/browser/contributions/upload-sketch.ts b/arduino-ide-extension/src/browser/contributions/upload-sketch.ts
@@ -1,7 +1,8 @@
 import { Emitter } from '@theia/core/lib/common/event';
 import { nls } from '@theia/core/lib/common/nls';
 import { inject, injectable } from '@theia/core/shared/inversify';
-import { CoreService, sanitizeFqbn } from '../../common/protocol';
+import { FQBN } from 'fqbn';
+import { CoreService } from '../../common/protocol';
 import { ArduinoMenus } from '../menu/arduino-menus';
 import { CurrentSketch } from '../sketches-service-client-impl';
 import { ArduinoToolbar } from '../toolbar/arduino-toolbar';
@@ -173,7 +174,11 @@ export class UploadSketch extends CoreServiceContribution {
     const [fqbn, { selectedProgrammer: programmer }, verify, verbose] =
       await Promise.all([
         verifyOptions.fqbn, // already decorated FQBN
-        this.boardsDataStore.getData(sanitizeFqbn(verifyOptions.fqbn)),
+        this.boardsDataStore.getData(
+          verifyOptions.fqbn
+            ? new FQBN(verifyOptions.fqbn).toString(true)
+            : undefined
+        ),
         this.preferences.get('arduino.upload.verify'),
         this.preferences.get('arduino.upload.verbose'),
       ]);

diff --git a/arduino-ide-extension/src/common/protocol/boards-service.ts b/arduino-ide-extension/src/common/protocol/boards-service.ts
@@ -1,4 +1,5 @@
 import { nls } from '@theia/core/lib/common/nls';
+import { FQBN } from 'fqbn';
 import type { MaybePromise } from '@theia/core/lib/common/types';
 import type URI from '@theia/core/lib/common/uri';
 import {
@@ -367,40 +368,6 @@ export interface ConfigOption {
   readonly values: ConfigValue[];
 }
 export namespace ConfigOption {
-  /**
-   * Appends the configuration options to the `fqbn` argument.
-   * Throws an error if the `fqbn` does not have the `segment(':'segment)*` format.
-   * The provided output format is always segment(':'segment)*(':'option'='value(','option'='value)*)?
-   */
-  export function decorate(
-    fqbn: string,
-    configOptions: ConfigOption[]
-  ): string {
-    if (!configOptions.length) {
-      return fqbn;
-    }
-
-    const toValue = (values: ConfigValue[]) => {
-      const selectedValue = values.find(({ selected }) => selected);
-      if (!selectedValue) {
-        console.warn(
-          `None of the config values was selected. Values were: ${JSON.stringify(
-            values
-          )}`
-        );
-        return undefined;
-      }
-      return selectedValue.value;
-    };
-    const options = configOptions
-      .map(({ option, values }) => [option, toValue(values)])
-      .filter(([, value]) => !!value)
-      .map(([option, value]) => `${option}=${value}`)
-      .join(',');
-
-    return `${fqbn}:${options}`;
-  }
-
   export class ConfigOptionError extends Error {
     constructor(message: string) {
       super(message);
@@ -574,30 +541,6 @@ export namespace Board {
   }
 }
 
-/**
- * Throws an error if the `fqbn` argument is not sanitized. A sanitized FQBN has the `VENDOR:ARCHITECTURE:BOARD_ID` construct.
- */
-export function assertSanitizedFqbn(fqbn: string): void {
-  if (fqbn.split(':').length !== 3) {
-    throw new Error(
-      `Expected a sanitized FQBN with three segments in the following format: 'VENDOR:ARCHITECTURE:BOARD_ID'. Got ${fqbn} instead.`
-    );
-  }
-}
-
-/**
- * Converts the `VENDOR:ARCHITECTURE:BOARD_ID[:MENU_ID=OPTION_ID[,MENU2_ID=OPTION_ID ...]]` FQBN to
- * `VENDOR:ARCHITECTURE:BOARD_ID` format.
- * See the details of the `{build.fqbn}` entry in the [specs](https://arduino.github.io/arduino-cli/latest/platform-specification/#global-predefined-properties).
- */
-export function sanitizeFqbn(fqbn: string | undefined): string | undefined {
-  if (!fqbn) {
-    return undefined;
-  }
-  const [vendor, arch, id] = fqbn.split(':');
-  return `${vendor}:${arch}:${id}`;
-}
-
 export type PlatformIdentifier = Readonly<{ vendorId: string; arch: string }>;
 export function createPlatformIdentifier(
   board: BoardWithPackage
@@ -752,8 +695,8 @@ export function boardIdentifierEquals(
     return false; // TODO: This a strict now. Maybe compare name in the future.
   }
   if (left.fqbn && right.fqbn) {
-    const leftFqbn = options.looseFqbn ? sanitizeFqbn(left.fqbn) : left.fqbn;
-    const rightFqbn = options.looseFqbn ? sanitizeFqbn(right.fqbn) : right.fqbn;
+    const leftFqbn = new FQBN(left.fqbn).toString(options.looseFqbn);
+    const rightFqbn = new FQBN(right.fqbn).toString(options.looseFqbn);
     return leftFqbn === rightFqbn;
   }
   // No more Genuino hack.

diff --git a/arduino-ide-extension/src/node/board-discovery.ts b/arduino-ide-extension/src/node/board-discovery.ts
@@ -267,24 +267,12 @@ export class BoardDiscovery
       const { port, boards } = detectedPort;
       const key = Port.keyOf(port);
       if (eventType === EventType.Add) {
-        const alreadyDetectedPort = newState[key];
-        if (alreadyDetectedPort) {
-          console.warn(
-            `Detected a new port that has been already discovered. The old value will be overridden. Old value: ${JSON.stringify(
-              alreadyDetectedPort
-            )}, new value: ${JSON.stringify(detectedPort)}`
-          );
-        }
+        // Note that, the serial discovery might detect port details (such as addressLabel) in chunks.
+        // For example, first, the Teensy 4.1 port is detected with the `[no_device] Triple Serial` address label,
+        // Then, when more refinements are available, the same port is detected with `/dev/cu.usbmodem127902301 Triple Serial` address label.
+        // In such cases, an `add` event is received from the CLI, and the the detected port is overridden in the state.
         newState[key] = { port, boards };
       } else if (eventType === EventType.Remove) {
-        const alreadyDetectedPort = newState[key];
-        if (!alreadyDetectedPort) {
-          console.warn(
-            `Detected a port removal but it has not been discovered. This is most likely a bug! Detected port was: ${JSON.stringify(
-              detectedPort
-            )}`
-          );
-        }
         delete newState[key];
       }
     }