fix: invalid custom board option handling in FQBN

Closes #1588

Signed-off-by: Akos Kitta <[email protected]>

Author: Akos Kitta

Diff for: 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.5",
     "glob": "^7.1.6",
     "google-protobuf": "^3.20.1",
     "hash.js": "^1.1.7",

Diff for: 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,
@@ -20,6 +21,7 @@ import {
   Programmer,
   isBoardIdentifierChangeEvent,
   isProgrammer,
+  sanitizeFqbn,
 } from '../../common/protocol';
 import { notEmpty } from '../../common/utils';
 import type {
@@ -29,6 +31,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 +74,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 }) => {
@@ -116,7 +131,7 @@ export class BoardsDataStore
     if (!fqbn) {
       return undefined;
     } else {
-      const data = await this.getData(fqbn);
+      const data = await this.getData(sanitizeFqbn(fqbn));
       if (data === BoardsDataStore.Data.EMPTY) {
         return undefined;
       }
@@ -125,9 +140,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 +196,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> {
@@ -201,48 +229,63 @@ export class BoardsDataStore
     fqbn: string;
     selectedProgrammer: Programmer;
   }): Promise<boolean> {
-    const storedData = deepClone(await this.getData(fqbn));
+    const sanitizedFQBN = sanitizeFqbn(fqbn);
+    const storedData = deepClone(await this.getData(sanitizedFQBN));
     const { programmers } = storedData;
     if (!programmers.find((p) => Programmer.equals(selectedProgrammer, p))) {
       return false;
     }
 
-    const data = { ...storedData, selectedProgrammer };
-    await this.setData({ fqbn, data });
-    this.fireChanged({ fqbn, data });
+    const change: BoardsDataStoreChange = {
+      fqbn: sanitizedFQBN,
+      data: { ...storedData, selectedProgrammer },
+    };
+    await this.setData(change);
+    this.fireChanged(change);
     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 sanitizedFQBN = sanitizeFqbn(fqbn);
+    const mutableData = deepClone(await this.getData(sanitizedFQBN));
+    let didChange = false;
+
+    for (const { option, selectedValue } of optionsToUpdate) {
+      const { configOptions } = mutableData;
+      const configOption = configOptions.find((c) => c.option === option);
+      if (configOption) {
+        const configOptionValueIndex = configOption.values.findIndex(
+          (configOptionValue) => configOptionValue.value === selectedValue
+        );
+        if (configOptionValueIndex >= 0) {
+          // unselect all
+          configOption.values
+            .map((value) => value as Mutable<ConfigValue>)
+            .forEach((value) => (value.selected = false));
+          const mutableConfigValue: Mutable<ConfigValue> =
+            configOption.values[configOptionValueIndex];
+          // make the new value `selected`
+          mutableConfigValue.selected = true;
+          didChange = true;
+        }
       }
     }
-    if (!updated) {
+
+    if (!didChange) {
       return false;
     }
-    await this.setData({ fqbn, data });
-    this.fireChanged({ fqbn, data });
+
+    const change: BoardsDataStoreChange = {
+      fqbn: sanitizedFQBN,
+      data: mutableData,
+    };
+    await this.setData(change);
+    this.fireChanged(change);
     return true;
   }
 

Diff for: arduino-ide-extension/src/browser/boards/boards-service-provider.ts

@@ -12,6 +12,7 @@ import { Emitter } from '@theia/core/lib/common/event';
 import { ILogger } from '@theia/core/lib/common/logger';
 import { MessageService } from '@theia/core/lib/common/message-service';
 import { nls } from '@theia/core/lib/common/nls';
+import { deepClone } from '@theia/core/lib/common/objects';
 import { Deferred } from '@theia/core/lib/common/promise-util';
 import type { Mutable } from '@theia/core/lib/common/types';
 import { inject, injectable, optional } from '@theia/core/shared/inversify';
@@ -21,31 +22,32 @@ import {
 } from '@theia/output/lib/browser/output-channel';
 import {
   BoardIdentifier,
-  boardIdentifierEquals,
+  BoardUserField,
+  BoardWithPackage,
   BoardsConfig,
   BoardsConfigChangeEvent,
   BoardsPackage,
   BoardsService,
-  BoardUserField,
-  BoardWithPackage,
   DetectedPorts,
+  Port,
+  PortIdentifier,
+  boardIdentifierEquals,
   emptyBoardsConfig,
   isBoardIdentifier,
   isBoardIdentifierChangeEvent,
   isPortIdentifier,
   isPortIdentifierChangeEvent,
-  Port,
-  PortIdentifier,
   portIdentifierEquals,
+  sanitizeFqbn,
   serializePlatformIdentifier,
 } from '../../common/protocol';
 import {
   BoardList,
   BoardListHistory,
-  createBoardList,
   EditBoardsConfigActionParams,
-  isBoardListHistory,
   SelectBoardsConfigActionParams,
+  createBoardList,
+  isBoardListHistory,
 } from '../../common/protocol/board-list';
 import type { Defined } from '../../common/types';
 import type {
@@ -104,6 +106,21 @@ type BoardListHistoryUpdateResult =
 type BoardToSelect = BoardIdentifier | undefined | 'ignore-board';
 type PortToSelect = PortIdentifier | undefined | 'ignore-port';
 
+function sanitizeBoardToSelectFQBN(board: BoardToSelect): BoardToSelect {
+  if (isBoardIdentifier(board)) {
+    return sanitizeBoardIdentifierFQBN(board);
+  }
+  return board;
+}
+function sanitizeBoardIdentifierFQBN(board: BoardIdentifier): BoardIdentifier {
+  if (board.fqbn) {
+    const copy: Mutable<BoardIdentifier> = deepClone(board);
+    copy.fqbn = sanitizeFqbn(board.fqbn);
+    return copy;
+  }
+  return board;
+}
+
 interface UpdateBoardListHistoryParams {
   readonly portToSelect: PortToSelect;
   readonly boardToSelect: BoardToSelect;
@@ -136,6 +153,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 +210,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>();
@@ -353,7 +373,8 @@ export class BoardsServiceProvider
       portToSelect !== 'ignore-port' &&
       !portIdentifierEquals(portToSelect, previousSelectedPort);
     const boardDidChangeEvent = boardDidChange
-      ? { selectedBoard: boardToSelect, previousSelectedBoard }
+      ? // The change event must always contain any custom board options. Hence the board to select is not sanitized.
+        { selectedBoard: boardToSelect, previousSelectedBoard }
       : undefined;
     const portDidChangeEvent = portDidChange
       ? { selectedPort: portToSelect, previousSelectedPort }
@@ -374,16 +395,31 @@ export class BoardsServiceProvider
       return false;
     }
 
-    this.maybeUpdateBoardListHistory({ portToSelect, boardToSelect });
-    this.maybeUpdateBoardsData({ boardToSelect, reason });
+    // unlike for the board change event, every persistent state must not contain custom board config options in the FQBN
+    const sanitizedBoardToSelect = sanitizeBoardToSelectFQBN(boardToSelect);
+
+    this.maybeUpdateBoardListHistory({
+      portToSelect,
+      boardToSelect: sanitizedBoardToSelect,
+    });
+    this.maybeUpdateBoardsData({
+      boardToSelect: sanitizedBoardToSelect,
+      reason,
+    });
 
     if (isBoardIdentifierChangeEvent(event)) {
-      this._boardsConfig.selectedBoard = event.selectedBoard;
+      this._boardsConfig.selectedBoard = event.selectedBoard
+        ? sanitizeBoardIdentifierFQBN(event.selectedBoard)
+        : event.selectedBoard;
     }
     if (isPortIdentifierChangeEvent(event)) {
       this._boardsConfig.selectedPort = event.selectedPort;
     }
 
+    if (reason) {
+      event = Object.assign(event, { reason });
+    }
+
     this.boardsConfigDidChangeEmitter.fire(event);
     this.refreshBoardList();
     this.saveState();

Diff for: 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 for: arduino-ide-extension/src/browser/contributions/ino-language.ts

@@ -9,7 +9,6 @@ import {
   BoardIdentifier,
   BoardsService,
   ExecutableService,
-  assertSanitizedFqbn,
   isBoardIdentifierChangeEvent,
   sanitizeFqbn,
 } from '../../common/protocol';
@@ -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 sanitizedFQBN = sanitizeFqbn(this.languageServerFqbn);
+          // 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) => change.fqbn === sanitizedFqbn
+            (change) => sanitizedFQBN === sanitizeFqbn(change.fqbn)
           );
           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 for: 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'),
       ]);