diff --git a/arduino-ide-extension/src/browser/contributions/ino-language.ts b/arduino-ide-extension/src/browser/contributions/ino-language.ts index 2577d5a73..810e60fca 100644 --- a/arduino-ide-extension/src/browser/contributions/ino-language.ts +++ b/arduino-ide-extension/src/browser/contributions/ino-language.ts @@ -6,7 +6,6 @@ import { inject, injectable } from '@theia/core/shared/inversify'; import { Mutex } from 'async-mutex'; import { ArduinoDaemon, - assertSanitizedFqbn, BoardsService, ExecutableService, sanitizeFqbn, @@ -93,8 +92,10 @@ export class InoLanguage extends SketchContribution { `Failed to sanitize the FQBN of the running language server. FQBN with the board settings was: ${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 matchingFqbn = dataChangePerFqbn.find( - (fqbn) => sanitizedFqbn === fqbn + (fqbn) => sanitizedFqbn === sanitizeFqbn(fqbn) ); const { boardsConfig } = this.boardsServiceProvider; if ( @@ -151,7 +152,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/common/protocol/boards-service.ts b/arduino-ide-extension/src/common/protocol/boards-service.ts index c955b9462..3f2001247 100644 --- a/arduino-ide-extension/src/common/protocol/boards-service.ts +++ b/arduino-ide-extension/src/common/protocol/boards-service.ts @@ -180,7 +180,7 @@ export namespace BoardSearch { 'Partner', 'Arduino@Heart', ] as const; - export type Type = typeof TypeLiterals[number]; + export type Type = (typeof TypeLiterals)[number]; export namespace Type { export function is(arg: unknown): arg is Type { return typeof arg === 'string' && TypeLiterals.includes(arg as Type); @@ -347,7 +347,7 @@ export namespace Port { export namespace Protocols { export const KnownProtocolLiterals = ['serial', 'network'] as const; - export type KnownProtocol = typeof KnownProtocolLiterals[number]; + export type KnownProtocol = (typeof KnownProtocolLiterals)[number]; export namespace KnownProtocol { export function is(protocol: unknown): protocol is KnownProtocol { return ( @@ -476,29 +476,106 @@ export namespace ConfigOption { fqbn: string, configOptions: ConfigOption[] ): string { - if (!configOptions.length) { - return fqbn; + const failInvalidFQBN = (): never => { + throw new Error(`Invalid FQBN: ${fqbn}`); + }; + + const [vendor, arch, id, rest] = fqbn.split(':'); + if (!vendor || !arch || !id) { + return failInvalidFQBN(); } - const toValue = (values: ConfigValue[]) => { + const existingOptions: Record = {}; + if (rest) { + // If rest exists, it must have the key=value(,key=value)* format. Otherwise, fail. + const tuples = rest.split(','); + for (const tuple of tuples) { + const segments = tuple.split('='); + if (segments.length !== 2) { + failInvalidFQBN(); + } + const [option, value] = segments; + if (!option || !value) { + failInvalidFQBN(); + } + if (existingOptions[option]) { + console.warn( + `Config value already exists for '${option}' on FQBN. Skipping it. Existing value: ${existingOptions[option]}, new value: ${value}, FQBN: ${fqbn}` + ); + } else { + existingOptions[option] = value; + } + } + } + + const newOptions: Record = {}; + for (const configOption of configOptions) { + const { option, values } = configOption; + if (!option) { + console.warn( + `Detected empty option on config options. Skipping it. ${JSON.stringify( + configOption + )}` + ); + continue; + } const selectedValue = values.find(({ selected }) => selected); - if (!selectedValue) { + if (selectedValue) { + const { value } = selectedValue; + if (!value) { + console.warn( + `Detected empty selected value on config options. Skipping it. ${JSON.stringify( + configOption + )}` + ); + continue; + } + if (newOptions[option]) { + console.warn( + `Config value already exists for '${option}' in config options. Skipping it. Existing value: ${ + newOptions[option] + }, new value: ${value}, config option: ${JSON.stringify( + configOption + )}` + ); + } else { + newOptions[option] = value; + } + } else { console.warn( - `None of the config values was selected. Values were: ${JSON.stringify( - values + `None of the config values was selected. Config options was: ${JSON.stringify( + configOption )}` ); - return undefined; } - return selectedValue.value; - }; - const options = configOptions - .map(({ option, values }) => [option, toValue(values)]) - .filter(([, value]) => !!value) + } + + // Collect all options from the FQBN. Call them existing. + // Collect all incoming options to decorate the FQBN with. Call them new. + // To keep the order, iterate through the existing ones and append to FQBN. + // If a new(er) value exists for the same option, use the new value. + // If there is a new value, "mark" it as visited by deleting it from new. Otherwise, use the existing value. + // Append all new ones to the FQBN. + const mergedOptions: Record = {}; + for (const existing of Object.entries(existingOptions)) { + const [option, value] = existing; + const newValue = newOptions[option]; + if (newValue) { + mergedOptions[option] = newValue; + delete newOptions[option]; + } else { + mergedOptions[option] = value; + } + } + Array.from(Object.entries(newOptions)).forEach( + ([option, value]) => (mergedOptions[option] = value) + ); + + const configSuffix = Object.entries(mergedOptions) .map(([option, value]) => `${option}=${value}`) .join(','); - return `${fqbn}:${options}`; + return `${vendor}:${arch}:${id}${configSuffix ? `:${configSuffix}` : ''}`; } export class ConfigOptionError extends Error { diff --git a/arduino-ide-extension/src/test/common/boards-service.test.ts b/arduino-ide-extension/src/test/common/boards-service.test.ts index d2cae5a53..01c811b79 100644 --- a/arduino-ide-extension/src/test/common/boards-service.test.ts +++ b/arduino-ide-extension/src/test/common/boards-service.test.ts @@ -4,6 +4,7 @@ import { expect } from 'chai'; import { AttachedBoardsChangeEvent, BoardInfo, + ConfigOption, getBoardInfo, noNativeSerialPort, nonSerialPort, @@ -93,6 +94,120 @@ describe('boards-service', () => { }); }); + describe('ConfigOption#decorate', () => { + ['invalid', 'a:b:c:foo=', 'a:b:c:foo=bar=baz', 'a:b:c:foo+bar'].map( + (fqbn) => + it(`should error when the FQBN is invalid (FQBN: ${fqbn})`, () => { + expect(() => ConfigOption.decorate(fqbn, [])).to.throw( + /^Invalid FQBN:*/ + ); + }) + ); + + it('should not append the trailing boards config part to FQBN if configs is empty', () => { + const fqbn = 'a:b:c'; + const actual = ConfigOption.decorate(fqbn, []); + expect(actual).to.be.equal(fqbn); + }); + + it('should be noop when config options is empty', () => { + const fqbn = 'a:b:c:menu1=value1'; + const actual = ConfigOption.decorate(fqbn, []); + expect(actual).to.be.equal(fqbn); + }); + + it('should not duplicate config options', () => { + const fqbn = 'a:b:c:menu1=value1'; + const actual = ConfigOption.decorate(fqbn, [ + { + label: 'Menu 1', + option: 'menu1', + values: [ + { label: 'Value 1', value: 'value1', selected: true }, + { label: 'Another Value', value: 'another1', selected: false }, + ], + }, + ]); + expect(actual).to.be.equal(fqbn); + }); + + it('should update config options', () => { + const fqbn = 'a:b:c:menu1=value1,menu2=value2'; + const actual = ConfigOption.decorate(fqbn, [ + { + label: 'Menu 1', + option: 'menu1', + values: [ + { label: 'Value 1', value: 'value1', selected: false }, + { label: 'Another Value', value: 'another1', selected: true }, + ], + }, + ]); + expect(actual).to.be.equal('a:b:c:menu1=another1,menu2=value2'); + }); + + it('should favor first over rest when there are duplicate options (duplicate on FQBN)', () => { + const fqbn = 'a:b:c:menu1=value1,menu1=value2'; + const actual = ConfigOption.decorate(fqbn, [ + { + label: 'Menu 1', + option: 'menu1', + values: [ + { label: 'Value 1', value: 'value1', selected: false }, + { label: 'Another Value', value: 'another1', selected: true }, + ], + }, + ]); + expect(actual).to.be.equal('a:b:c:menu1=another1'); + }); + + it('should favor first over rest when there are duplicate options (duplicate in config)', () => { + const fqbn = 'a:b:c'; + const actual = ConfigOption.decorate(fqbn, [ + { + label: 'Menu 1', + option: 'menu1', + values: [ + { label: 'Value 1', value: 'value1', selected: false }, + { label: 'Another Value', value: 'another1', selected: true }, + ], + }, + { + label: 'Menu 1', + option: 'menu1', + values: [ + { label: 'Value 1', value: 'value1', selected: true }, + { label: 'Another Value', value: 'another1', selected: false }, + ], + }, + ]); + expect(actual).to.be.equal('a:b:c:menu1=another1'); + }); + + it('should not change the existing config order', () => { + const fqbn = 'a:b:c:menu1=value1'; + const actual = ConfigOption.decorate(fqbn, [ + { + label: 'Menu 0', + option: 'menu0', + values: [ + { label: 'Value 0', value: 'value0', selected: true }, + { label: 'Another Value', value: 'another0', selected: false }, + ], + }, + { + label: 'Menu 1', + option: 'menu1', + values: [ + { label: 'Value 1', value: 'value1', selected: false }, + { label: 'Another Value', value: 'another1', selected: true }, + ], + }, + ]); + expect(actual).to.be.equal('a:b:c:menu1=another1,menu0=value0'); + }); + }); + describe('getBoardInfo', () => { const vid = '0x0'; const pid = '0x1';