Skip to content

Commit 60b3120

Browse files
author
Akos Kitta
committed
fix: invalid custom board option handling in FQBN
Closes #1588 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
1 parent 0987ea8 commit 60b3120

File tree

10 files changed

+280
-130
lines changed

10 files changed

+280
-130
lines changed

‎arduino-ide-extension/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
"fast-json-stable-stringify": "^2.1.0",
7373
"fast-safe-stringify": "^2.1.1",
7474
"filename-reserved-regex": "^2.0.0",
75+
"fqbn": "^1.0.3",
7576
"glob": "^7.1.6",
7677
"google-protobuf": "^3.20.1",
7778
"hash.js": "^1.1.7",

‎arduino-ide-extension/src/browser/boards/boards-data-store.ts‎

Lines changed: 59 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { ILogger } from '@theia/core/lib/common/logger';
1212
import { deepClone, deepFreeze } from '@theia/core/lib/common/objects';
1313
import type { Mutable } from '@theia/core/lib/common/types';
1414
import { inject, injectable, named } from '@theia/core/shared/inversify';
15+
import { FQBN } from 'fqbn';
1516
import {
1617
BoardDetails,
1718
BoardsService,
@@ -29,6 +30,14 @@ import type {
2930
import { NotificationCenter } from '../notification-center';
3031
import { BoardsServiceProvider } from './boards-service-provider';
3132

33+
export interface SelectConfigOptionParams {
34+
readonly fqbn: string;
35+
readonly optionsToUpdate: readonly Readonly<{
36+
option: string;
37+
selectedValue: string;
38+
}>[];
39+
}
40+
3241
@injectable()
3342
export class BoardsDataStore
3443
implements
@@ -64,7 +73,12 @@ export class BoardsDataStore
6473
this.toDispose.pushAll([
6574
this.boardsServiceProvider.onBoardsConfigDidChange((event) => {
6675
if (isBoardIdentifierChangeEvent(event)) {
67-
this.updateSelectedBoardData(event.selectedBoard?.fqbn);
76+
this.updateSelectedBoardData(
77+
event.selectedBoard?.fqbn,
78+
// If the change event comes from toolbar and the FQBN contains custom board options, change the currently selected options
79+
// https://github.com/arduino/arduino-ide/issues/1588
80+
event.reason === 'toolbar'
81+
);
6882
}
6983
}),
7084
this.notificationCenter.onPlatformDidInstall(async ({ item }) => {
@@ -125,9 +139,22 @@ export class BoardsDataStore
125139
}
126140

127141
private async updateSelectedBoardData(
128-
fqbn: string | undefined
142+
fqbn: string | undefined,
143+
updateConfigOptions = false
129144
): Promise<void> {
130145
this._selectedBoardData = await this.getSelectedBoardData(fqbn);
146+
if (fqbn && updateConfigOptions) {
147+
const { options } = new FQBN(fqbn);
148+
if (options) {
149+
const optionsToUpdate = Object.entries(options).map(([key, value]) => ({
150+
option: key,
151+
selectedValue: value,
152+
}));
153+
const params = { fqbn, optionsToUpdate };
154+
await this.selectConfigOption(params);
155+
this._selectedBoardData = await this.getSelectedBoardData(fqbn); // reload the updated data
156+
}
157+
}
131158
}
132159

133160
onStop(): void {
@@ -168,7 +195,7 @@ export class BoardsDataStore
168195
return undefined;
169196
}
170197
const { configOptions } = await this.getData(fqbn);
171-
return ConfigOption.decorate(fqbn, configOptions);
198+
return new FQBN(fqbn).withConfigOptions(...configOptions).toString();
172199
}
173200

174201
async getData(fqbn: string | undefined): Promise<BoardsDataStore.Data> {
@@ -213,36 +240,41 @@ export class BoardsDataStore
213240
return true;
214241
}
215242

216-
async selectConfigOption({
217-
fqbn,
218-
option,
219-
selectedValue,
220-
}: {
221-
fqbn: string;
222-
option: string;
223-
selectedValue: string;
224-
}): Promise<boolean> {
225-
const data = deepClone(await this.getData(fqbn));
226-
const { configOptions } = data;
227-
const configOption = configOptions.find((c) => c.option === option);
228-
if (!configOption) {
243+
async selectConfigOption(params: SelectConfigOptionParams): Promise<boolean> {
244+
const { fqbn, optionsToUpdate } = params;
245+
if (!optionsToUpdate.length) {
229246
return false;
230247
}
231-
let updated = false;
232-
for (const value of configOption.values) {
233-
const mutable: Mutable<ConfigValue> = value;
234-
if (mutable.value === selectedValue) {
235-
mutable.selected = true;
236-
updated = true;
237-
} else {
238-
mutable.selected = false;
248+
249+
const mutableData = deepClone(await this.getData(fqbn));
250+
let didChange = false;
251+
252+
for (const { option, selectedValue } of optionsToUpdate) {
253+
const { configOptions } = mutableData;
254+
const configOption = configOptions.find((c) => c.option === option);
255+
if (configOption) {
256+
let updated = false;
257+
for (const value of configOption.values) {
258+
const mutable: Mutable<ConfigValue> = value;
259+
if (mutable.value === selectedValue) {
260+
mutable.selected = true;
261+
updated = true;
262+
} else {
263+
mutable.selected = false;
264+
}
265+
}
266+
if (updated) {
267+
didChange = true;
268+
}
239269
}
240270
}
241-
if (!updated) {
271+
272+
if (!didChange) {
242273
return false;
243274
}
244-
await this.setData({ fqbn, data });
245-
this.fireChanged({ fqbn, data });
275+
276+
await this.setData({ fqbn, data: mutableData });
277+
this.fireChanged({ fqbn, data: mutableData });
246278
return true;
247279
}
248280

‎arduino-ide-extension/src/browser/boards/boards-service-provider.ts‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,9 @@ export interface BoardListUIActions {
136136
}
137137
export type BoardListUI = BoardList & BoardListUIActions;
138138

139+
export type BoardsConfigChangeEventUI = BoardsConfigChangeEvent &
140+
Readonly<{ reason?: UpdateBoardsConfigReason }>;
141+
139142
@injectable()
140143
export class BoardListDumper implements Disposable {
141144
@inject(OutputChannelManager)
@@ -190,7 +193,7 @@ export class BoardsServiceProvider
190193
private _ready = new Deferred<void>();
191194

192195
private readonly boardsConfigDidChangeEmitter =
193-
new Emitter<BoardsConfigChangeEvent>();
196+
new Emitter<BoardsConfigChangeEventUI>();
194197
readonly onBoardsConfigDidChange = this.boardsConfigDidChangeEmitter.event;
195198

196199
private readonly boardListDidChangeEmitter = new Emitter<BoardListUI>();
@@ -384,6 +387,10 @@ export class BoardsServiceProvider
384387
this._boardsConfig.selectedPort = event.selectedPort;
385388
}
386389

390+
if (reason) {
391+
event = Object.assign(event, { reason });
392+
}
393+
387394
this.boardsConfigDidChangeEmitter.fire(event);
388395
this.refreshBoardList();
389396
this.saveState();

‎arduino-ide-extension/src/browser/contributions/boards-data-menu-updater.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,7 @@ export class BoardsDataMenuUpdater extends Contribution {
8787
execute: () =>
8888
this.boardsDataStore.selectConfigOption({
8989
fqbn,
90-
option,
91-
selectedValue: value.value,
90+
optionsToUpdate: [{ option, selectedValue: value.value }],
9291
}),
9392
isToggled: () => value.selected,
9493
};

‎arduino-ide-extension/src/browser/contributions/ino-language.ts‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,9 @@ import {
99
BoardIdentifier,
1010
BoardsService,
1111
ExecutableService,
12-
assertSanitizedFqbn,
1312
isBoardIdentifierChangeEvent,
14-
sanitizeFqbn,
1513
} from '../../common/protocol';
14+
import { FQBN } from 'fqbn';
1615
import {
1716
defaultAsyncWorkers,
1817
maxAsyncWorkers,
@@ -159,14 +158,11 @@ export class InoLanguage extends SketchContribution {
159158
this.notificationCenter.onDidReinitialize(() => forceRestart()),
160159
this.boardDataStore.onDidChange((event) => {
161160
if (this.languageServerFqbn) {
162-
const sanitizedFqbn = sanitizeFqbn(this.languageServerFqbn);
163-
if (!sanitizeFqbn) {
164-
throw new Error(
165-
`Failed to sanitize the FQBN of the running language server. FQBN with the board settings was: ${this.languageServerFqbn}`
166-
);
167-
}
168-
const matchingChange = event.changes.find(
169-
(change) => change.fqbn === sanitizedFqbn
161+
const sanitizedFqbn = new FQBN(this.languageServerFqbn).sanitize();
162+
// The incoming FQBNs might contain custom boards configs, sanitize them before the comparison.
163+
// https://github.com/arduino/arduino-ide/pull/2113#pullrequestreview-1499998328
164+
const matchingChange = event.changes.find((change) =>
165+
new FQBN(change.fqbn).sanitize().equals(sanitizedFqbn)
170166
);
171167
const { boardsConfig } = this.boardsServiceProvider;
172168
if (
@@ -228,7 +224,6 @@ export class InoLanguage extends SketchContribution {
228224
}
229225
return;
230226
}
231-
assertSanitizedFqbn(fqbn);
232227
const fqbnWithConfig = await this.boardDataStore.appendConfigToFqbn(fqbn);
233228
if (!fqbnWithConfig) {
234229
throw new Error(

‎arduino-ide-extension/src/browser/contributions/upload-sketch.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { Emitter } from '@theia/core/lib/common/event';
22
import { nls } from '@theia/core/lib/common/nls';
33
import { inject, injectable } from '@theia/core/shared/inversify';
4-
import { CoreService, sanitizeFqbn } from '../../common/protocol';
4+
import { FQBN } from 'fqbn';
5+
import { CoreService } from '../../common/protocol';
56
import { ArduinoMenus } from '../menu/arduino-menus';
67
import { CurrentSketch } from '../sketches-service-client-impl';
78
import { ArduinoToolbar } from '../toolbar/arduino-toolbar';
@@ -173,7 +174,11 @@ export class UploadSketch extends CoreServiceContribution {
173174
const [fqbn, { selectedProgrammer: programmer }, verify, verbose] =
174175
await Promise.all([
175176
verifyOptions.fqbn, // already decorated FQBN
176-
this.boardsDataStore.getData(sanitizeFqbn(verifyOptions.fqbn)),
177+
this.boardsDataStore.getData(
178+
verifyOptions.fqbn
179+
? new FQBN(verifyOptions.fqbn).toString(true)
180+
: undefined
181+
),
177182
this.preferences.get('arduino.upload.verify'),
178183
this.preferences.get('arduino.upload.verbose'),
179184
]);

‎arduino-ide-extension/src/common/protocol/boards-service.ts‎

Lines changed: 3 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { nls } from '@theia/core/lib/common/nls';
2+
import { FQBN } from 'fqbn';
23
import type { MaybePromise } from '@theia/core/lib/common/types';
34
import type URI from '@theia/core/lib/common/uri';
45
import {
@@ -367,40 +368,6 @@ export interface ConfigOption {
367368
readonly values: ConfigValue[];
368369
}
369370
export namespace ConfigOption {
370-
/**
371-
* Appends the configuration options to the `fqbn` argument.
372-
* Throws an error if the `fqbn` does not have the `segment(':'segment)*` format.
373-
* The provided output format is always segment(':'segment)*(':'option'='value(','option'='value)*)?
374-
*/
375-
export function decorate(
376-
fqbn: string,
377-
configOptions: ConfigOption[]
378-
): string {
379-
if (!configOptions.length) {
380-
return fqbn;
381-
}
382-
383-
const toValue = (values: ConfigValue[]) => {
384-
const selectedValue = values.find(({ selected }) => selected);
385-
if (!selectedValue) {
386-
console.warn(
387-
`None of the config values was selected. Values were: ${JSON.stringify(
388-
values
389-
)}`
390-
);
391-
return undefined;
392-
}
393-
return selectedValue.value;
394-
};
395-
const options = configOptions
396-
.map(({ option, values }) => [option, toValue(values)])
397-
.filter(([, value]) => !!value)
398-
.map(([option, value]) => `${option}=${value}`)
399-
.join(',');
400-
401-
return `${fqbn}:${options}`;
402-
}
403-
404371
export class ConfigOptionError extends Error {
405372
constructor(message: string) {
406373
super(message);
@@ -574,30 +541,6 @@ export namespace Board {
574541
}
575542
}
576543

577-
/**
578-
* Throws an error if the `fqbn` argument is not sanitized. A sanitized FQBN has the `VENDOR:ARCHITECTURE:BOARD_ID` construct.
579-
*/
580-
export function assertSanitizedFqbn(fqbn: string): void {
581-
if (fqbn.split(':').length !== 3) {
582-
throw new Error(
583-
`Expected a sanitized FQBN with three segments in the following format: 'VENDOR:ARCHITECTURE:BOARD_ID'. Got ${fqbn} instead.`
584-
);
585-
}
586-
}
587-
588-
/**
589-
* Converts the `VENDOR:ARCHITECTURE:BOARD_ID[:MENU_ID=OPTION_ID[,MENU2_ID=OPTION_ID ...]]` FQBN to
590-
* `VENDOR:ARCHITECTURE:BOARD_ID` format.
591-
* See the details of the `{build.fqbn}` entry in the [specs](https://arduino.github.io/arduino-cli/latest/platform-specification/#global-predefined-properties).
592-
*/
593-
export function sanitizeFqbn(fqbn: string | undefined): string | undefined {
594-
if (!fqbn) {
595-
return undefined;
596-
}
597-
const [vendor, arch, id] = fqbn.split(':');
598-
return `${vendor}:${arch}:${id}`;
599-
}
600-
601544
export type PlatformIdentifier = Readonly<{ vendorId: string; arch: string }>;
602545
export function createPlatformIdentifier(
603546
board: BoardWithPackage
@@ -752,8 +695,8 @@ export function boardIdentifierEquals(
752695
return false; // TODO: This a strict now. Maybe compare name in the future.
753696
}
754697
if (left.fqbn && right.fqbn) {
755-
const leftFqbn = options.looseFqbn ? sanitizeFqbn(left.fqbn) : left.fqbn;
756-
const rightFqbn = options.looseFqbn ? sanitizeFqbn(right.fqbn) : right.fqbn;
698+
const leftFqbn = new FQBN(left.fqbn).toString(options.looseFqbn);
699+
const rightFqbn = new FQBN(right.fqbn).toString(options.looseFqbn);
757700
return leftFqbn === rightFqbn;
758701
}
759702
// No more Genuino hack.

‎arduino-ide-extension/src/node/board-discovery.ts‎

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -267,24 +267,12 @@ export class BoardDiscovery
267267
const { port, boards } = detectedPort;
268268
const key = Port.keyOf(port);
269269
if (eventType === EventType.Add) {
270-
const alreadyDetectedPort = newState[key];
271-
if (alreadyDetectedPort) {
272-
console.warn(
273-
`Detected a new port that has been already discovered. The old value will be overridden. Old value: ${JSON.stringify(
274-
alreadyDetectedPort
275-
)}, new value: ${JSON.stringify(detectedPort)}`
276-
);
277-
}
270+
// Note that, the serial discovery might detect port details (such as addressLabel) in chunks.
271+
// For example, first, the Teensy 4.1 port is detected with the `[no_device] Triple Serial` address label,
272+
// Then, when more refinements are available, the same port is detected with `/dev/cu.usbmodem127902301 Triple Serial` address label.
273+
// In such cases, an `add` event is received from the CLI, and the the detected port is overridden in the state.
278274
newState[key] = { port, boards };
279275
} else if (eventType === EventType.Remove) {
280-
const alreadyDetectedPort = newState[key];
281-
if (!alreadyDetectedPort) {
282-
console.warn(
283-
`Detected a port removal but it has not been discovered. This is most likely a bug! Detected port was: ${JSON.stringify(
284-
detectedPort
285-
)}`
286-
);
287-
}
288276
delete newState[key];
289277
}
290278
}

0 commit comments

Comments
 (0)