Fixed handling of invalid manual configurations. Improved code locality for board configuration manipulation and fixed a race condition with fine grained event handling. Details:

elektronikworkshop · adiazulay · commit 2d52e526b85b · 2021-01-19T15:08:52.000-08:00
Added missing checks when board configurations are loaded from the configuration file:
* Up to now vscode-arduino blindly loaded any board configuration from `arduino.json` even if this would result in invalid board configurations which in turn lead to compilation (verify, upload) failure.
* Up to now this state couldn't be recovered by simply removing the offending configuration from the configuration file. Even worse: it stored the wrong configuration in between board changes.

To reproduce the bug in 0.2.29

1. Select Arduino Nano with the *Arduino Board Configuration*
2. Set configuration in `arduino.json` to `cpu=cray2` and save
3. Verify -&gt; fails
4. Switch board to Arduino Uno
5. Switch back to Arduino Nano: The wrong configuration is back and now the user can't even select another (correct) configuration from the *Arduino Board Configuration* window
6. Delete the wrong configuration and save -&gt; verify still fails

`vscode-arduino` does not fall back to a default configuration. The user has now two options: find the correct configuration by himself and set it within arduino.json. Very experienced users could probably accomplish that. Everone else can just restart vscode.

I corrected that by enhancing IBoard.loadConfig and IBoard.updateConfig member functions to
* check for proper formatting of the config string loaded from `arduino.json`
* check if the configuration IDs and the option IDs are valid

If any of the above fails, the functions bail out and return the error. The board manager then loads a default configuration and issues a warning, that the configuration is invalid. This way the user gets the chance to fix her/his configuration but gets informed at the same time, that a different configuration than the intended is loaded (prevents surprises).

This situation is only relevant, when users start fiddling with the configuration values in `arduino.json`. As long as they just set the board and the configurations from within the *Arduino Board Configuration Window* nothing bad can happen. But now custom configurations are handled in a cleaner way.

The DeviceContext's board configuration was set in board.ts and boardManager.ts in different places - even when it was loaded after a DeviceContext's configuration update event which is prone to infinite loops. This has been resolved and it's not re-written/re-set during loading a configuration on change.

This is valid for board manager's updateStatusBar function which fiddled with the board and the configuration. Now updateStatusBar really just updates the status bar. And it isn't necessary to call it from outside the board manager anymore due to proper event handling which identifies the situations during which the status bar has to be updated. Therefore this member is now private.

In board manager itself operations that affect device context and current board now happen only within doChangeBoardType and the event handlers of DeviceContext callbacks onDeviceContextConfigurationChange and onDeviceContextBoardChange. This prevents the accidental creation of infinite event loops, makes the code more understandable, maintainable and therefore resilient against future bugs.
diff --git a/src/arduino/arduinoContentProvider.ts b/src/arduino/arduinoContentProvider.ts
@@ -8,6 +8,7 @@ import ArduinoActivator from "../arduinoActivator";
 import ArduinoContext from "../arduinoContext";
 import * as Constants from "../common/constants";
 import * as JSONHelper from "../common/cycle";
+import { DeviceContext } from "../deviceContext";
 import * as Logger from "../logger/logger";
 import LocalWebServer from "./localWebServer";
 
@@ -262,6 +263,8 @@ export class ArduinoContentProvider implements vscode.TextDocumentContentProvide
         } else {
             try {
                 ArduinoContext.boardManager.currentBoard.updateConfig(req.body.configId, req.body.optionId);
+                const dc = DeviceContext.getInstance();
+                dc.configuration = ArduinoContext.boardManager.currentBoard.customConfig;
                 return res.json({
                     status: "OK",
                 });
diff --git a/src/arduino/board.ts b/src/arduino/board.ts
@@ -1,8 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license.
 
-import { DeviceContext } from "../deviceContext";
-import { IBoard, IBoardConfigItem, IPlatform } from "./package";
+import { BoardConfigResult, IBoard, IBoardConfigItem, IPlatform } from "./package";
 
 export function parseBoardDescriptor(boardDescriptor: string, plat: IPlatform): Map<string, IBoard> {
     const boardLineRegex = /([^\.]+)\.(\S+)=(.+)/;
@@ -52,15 +51,15 @@ export class Board implements IBoard {
         this._configItems = [];
     }
 
-    public get board(): string {
+    public get board() {
         return this._board;
     }
 
-    public get platform(): IPlatform {
+    public get platform() {
         return this._platform;
     }
 
-    public addParameter(key: string, value: string): void {
+    public addParameter(key: string, value: string) {
         const match = key.match(MENU_REGEX);
         if (match) {
             const existingItem = this._configItems.find((item) => item.id === match[1]);
@@ -82,8 +81,7 @@ export class Board implements IBoard {
             }
         }
     }
-
-    public getBuildConfig(): string {
+    public getBuildConfig() {
         return `${this.getPackageName()}:${this.platform.architecture}:${this.board}${this.customConfig ? ":" + this.customConfig : ""}`;
     }
 
@@ -100,36 +98,86 @@ export class Board implements IBoard {
         }
     }
 
-    public get configItems(): IBoardConfigItem[] {
+    public get configItems() {
         return this._configItems;
     }
 
-    public loadConfig(configString: string): void {
+    public loadConfig(configString: string) {
+        // An empty or undefined config string resets the configuration
+        if (!configString) {
+            this.resetConfig();
+            return BoardConfigResult.Success;
+        }
         const configSections = configString.split(",");
         const keyValueRegex = /(\S+)=(\S+)/;
-        configSections.forEach((configSection) => {
-            const match = configSection.match(keyValueRegex);
-            if (match && match.length >= 2) {
-                this.updateConfig(match[1], match[2]);
+        let result = BoardConfigResult.Success;
+        for (const section of configSections) {
+            const match = section.match(keyValueRegex);
+            if (!match) {
+                return BoardConfigResult.InvalidFormat;
             }
-        });
+            const r = this.updateConfig(match[1], match[2]);
+            switch (r) {
+                case BoardConfigResult.SuccessNoChange:
+                    result = r;
+                    break;
+                case BoardConfigResult.Success:
+                    break;
+                default:
+                    return r;
+            }
+        };
+        return result;
     }
 
-    public updateConfig(configId: string, optionId: string): boolean {
+    /**
+     * For documentation see the documentation on IBoard.updateConfig().
+     */
+    public updateConfig(configId: string, optionId: string) {
         const targetConfig = this._configItems.find((config) => config.id === configId);
         if (!targetConfig) {
-            return false;
+            return BoardConfigResult.InvalidConfigID;
+        }
+        // Iterate through all options and ...
+        for (const o of targetConfig.options) {
+            // Make sure that we only set valid options, e.g. when loading
+            // from config files.
+            if (o.id === optionId) {
+                if (targetConfig.selectedOption !== optionId) {
+                    targetConfig.selectedOption = optionId;
+                    return BoardConfigResult.Success;
+                }
+                return BoardConfigResult.SuccessNoChange;
+            } else {
+                return BoardConfigResult.InvalidOptionID;
+            }
         }
-        if (targetConfig.selectedOption !== optionId) {
-            targetConfig.selectedOption = optionId;
-            const dc = DeviceContext.getInstance();
-            dc.configuration = this.customConfig;
-            return true;
+        return BoardConfigResult.InvalidOptionID;
+    }
+
+    public resetConfig() {
+        for (const c of this._configItems) {
+            c.selectedOption = c.options[0].id;
         }
-        return false;
     }
 
     public getPackageName() {
         return this.platform.packageName ? this.platform.packageName : this.platform.package.name;
     }
 }
+
+/**
+ * Test if two boards are of the same type, i.e. have the same key.
+ * @param {IBoard | undefined} a A board.
+ * @param {IBoard | undefined} b And another board.
+ * @returns {boolean} true if two boards are of the same type, else false.
+ */
+export function boardEqual(a: IBoard | undefined,
+                           b: IBoard | undefined) {
+    if (a && b) {
+        return a.key === b.key;
+    } else if (a || b) {
+        return false;
+    }
+    return true;
+}
diff --git a/src/arduino/boardManager.ts b/src/arduino/boardManager.ts
@@ -12,8 +12,8 @@ import { arduinoChannel } from "../common/outputChannel";
 import { DeviceContext } from "../deviceContext";
 import { ArduinoApp } from "./arduino";
 import { IArduinoSettings } from "./arduinoSettings";
-import { parseBoardDescriptor } from "./board";
-import { IBoard, IPackage, IPlatform } from "./package";
+import { boardEqual, parseBoardDescriptor } from "./board";
+import { BoardConfigResult, IBoard, IPackage, IPlatform } from "./package";
 import { VscodeSettings } from "./vscodeSettings";
 
 export class BoardManager {
@@ -67,18 +67,17 @@ export class BoardManager {
         // Load default platforms from arduino installation directory and user manually installed platforms.
         this.loadInstalledPlatforms();
 
-        // Load all supported boards type.
+        // Load all supported board types
         this.loadInstalledBoards();
-        this.updateStatusBar();
-        this._boardConfigStatusBar.show();
 
         const dc = DeviceContext.getInstance();
-        dc.onChangeBoard(() => {
-            this.updateStatusBar();
-        });
-        dc.onChangeConfiguration(() => {
-            this.updateStatusBar();
-        });
+        dc.onChangeBoard(() => this.onDeviceContextBoardChange());
+        dc.onChangeConfiguration(() => this.onDeviceContextConfigurationChange());
+
+        // load initial board from DeviceContext by emulating
+        // a board change event.
+        this.onDeviceContextBoardChange();
+        this.updateStatusBar(true);
     }
 
     public async changeBoardType() {
@@ -122,14 +121,24 @@ export class BoardManager {
 
     public doChangeBoardType(targetBoard: IBoard) {
         const dc = DeviceContext.getInstance();
+
+        if (dc.board === targetBoard.key) {
+            return;
+        }
+
+        // Resetting the board first that we don't overwrite the configuration
+        // of the previous board.
+        this._currentBoard = null;
+        // This will cause a configuration changed event which will have no
+        // effect because no current board is set.
+        dc.configuration = targetBoard.customConfig;
+        // This will generate a device context board event which will set the
+        // correct board and configuration. We know that it will trigger - we
+        // made sure above that the boards actually differ
         dc.board = targetBoard.key;
-        this._currentBoard = targetBoard;
-        dc.configuration = this._currentBoard.customConfig;
-        this._boardConfigStatusBar.text = targetBoard.name;
+
         // IS-REMOVE: to be removed completely when IntelliSense implementation is merged
         // this._arduinoApp.addLibPath(null);
-
-        this._onBoardTypeChanged.fire();
     }
 
     public get packages(): IPackage[] {
@@ -251,26 +260,90 @@ export class BoardManager {
         }
     }
 
-    public updateStatusBar(show: boolean = true): void {
+    private updateStatusBar(show: boolean = true): void {
         if (show) {
             this._boardConfigStatusBar.show();
-            const dc = DeviceContext.getInstance();
-            const selectedBoard = this._boards.get(dc.board);
-            if (selectedBoard) {
-                this._currentBoard = selectedBoard;
-                this._boardConfigStatusBar.text = selectedBoard.name;
-                if (dc.configuration) {
-                    this._currentBoard.loadConfig(dc.configuration);
-                }
+            if (this._currentBoard) {
+                this._boardConfigStatusBar.text = this._currentBoard.name;
             } else {
-                this._currentBoard = null;
                 this._boardConfigStatusBar.text = "<Select Board Type>";
             }
         } else {
             this._boardConfigStatusBar.hide();
         }
     }
 
+    /**
+     * Event callback if DeviceContext detected a new board - either when
+     * loaded from configuration file or when set by the doChangeBoardType
+     * member.
+     */
+    private onDeviceContextBoardChange() {
+        const dc = DeviceContext.getInstance();
+        const newBoard = this._boards.get(dc.board);
+        if (boardEqual(newBoard, this._currentBoard)) {
+            return;
+        }
+        if (newBoard) {
+            this._currentBoard = newBoard;
+            if (dc.configuration) {
+                // In case the configuration is incompatible, we reset it as
+                // setting partially valid configurations can lead to nasty
+                // surprises. When setting a new board this is acceptable
+                const r = this._currentBoard.loadConfig(dc.configuration);
+                if (r !== BoardConfigResult.Success && r !== BoardConfigResult.SuccessNoChange) {
+                    this._currentBoard.resetConfig();
+                    // we don't reset dc.configuration to give the user a
+                    // chance to fix her/his configuration
+                    this.invalidConfigWarning(r);
+                }
+            } else {
+                this._currentBoard.resetConfig();
+                dc.configuration = undefined;
+            }
+        } else {
+            this._currentBoard = null;
+        }
+        this._onBoardTypeChanged.fire();
+        this.updateStatusBar();
+    }
+
+    /**
+     * Event callback if DeviceContext detected a configuration change
+     * - either when loaded from configuration file or when set by the
+     * doChangeBoardType member.
+     */
+    private onDeviceContextConfigurationChange() {
+        const dc = DeviceContext.getInstance();
+        if (this._currentBoard) {
+            const r = this._currentBoard.loadConfig(dc.configuration);
+            if (r !== BoardConfigResult.Success && r !== BoardConfigResult.SuccessNoChange) {
+                this._currentBoard.resetConfig();
+                // We reset the configuration here but do not write it back
+                // to the configuration file - this can be annoying when
+                // someone tries to set a special configuration and doesn't
+                // get it right the first time.
+                this.invalidConfigWarning(r);
+            }
+        }
+    }
+
+    private invalidConfigWarning(result: BoardConfigResult) {
+        let what = "";
+        switch (result) {
+            case BoardConfigResult.InvalidFormat:
+                what = ": Invalid format must be of the form \"key1=value2,key1=value2,...\"";
+                break;
+            case BoardConfigResult.InvalidConfigID:
+                what = ": Invalid configuration key";
+                break;
+            case BoardConfigResult.InvalidOptionID:
+                what = ": Invalid configuration value";
+                break;
+        }
+        vscode.window.showWarningMessage(`Invalid board configuration detected in configuration file${what}. Falling back to defaults.`);
+    }
+
     private loadInstalledPlatforms() {
         const installed = this.getInstalledPlatforms();
         installed.forEach((platform) => {
diff --git a/src/arduino/package.ts b/src/arduino/package.ts
@@ -152,6 +152,35 @@ export interface IBoardConfigItem {
     options: IBoardConfigOption[];
 }
 
+/**
+ * Return values of calls to IBoard.loadConfig() and IBoard.updateConfig().
+ */
+export enum BoardConfigResult {
+    /**
+     * Setting configuration value(s) was successful
+     */
+    Success,
+    /**
+     * Setting configuration value(s) was successful. All or some items
+     * were already set to the requested values.
+     */
+    SuccessNoChange,
+    /**
+     * One or more configuration keys were invalid.
+     */
+    InvalidConfigID,
+    /**
+     * One or more options were invalid.
+     */
+    InvalidOptionID,
+    /**
+     * Can only happen when calling IBoard.loadConfig() and when
+     * the raw configuration string did contain invalid/unparsable
+     * elements.
+     */
+    InvalidFormat,
+}
+
 /**
  * Interface for classes that represent an Arduino supported board.
  *
@@ -206,14 +235,32 @@ export interface IBoard {
     getBuildConfig(): string;
 
     /**
-     * Load default configuration from saved context.
+     * Load configuration from saved context.
+     * Parses the configuration string and tries to set the individual
+     * configuration values. It will bail out on any error.
+     * @param {string} configString The configuration string from the
+     * configuration file.
+     * @returns {BoardConfigResult} Result of the operation - for more
+     * information see documentation of BoardConfigResult.
+     */
+    loadConfig(configString: string): BoardConfigResult;
+
+    /**
+     * Set configuration value.
+     * This function makes sure, that the configuration ID and the option ID
+     * are actually valid. It will bail out on any error
+     * @param {string} configId The ID of the configuration value
+     * @param {string} optionId The ID to which the option of the configuration
+     * value should be set to.
+     * @returns {BoardConfigResult} Result of the operation - for more
+     * information see documentation of BoardConfigResult.
      */
-    loadConfig(configString: string): void;
+    updateConfig(configId: string, optionId: string): BoardConfigResult;
 
     /**
-     * Update the configuration
+     * Reset configuration to defaults and update configuration file.
      */
-    updateConfig(configId: string, optionId: string): boolean;
+    resetConfig();
 
     /**
      * Get the board package name
diff --git a/src/extension.ts b/src/extension.ts
