Skip to content

Commit b3299ba

Browse files
author
Akos Kitta
committed
fix: flawed config handling in the FQBN
Also relaxed the LS assertions. As turned out, the CLI can detect FQBNs with already appended board config values. Closes #1588 Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
1 parent d79bc0d commit b3299ba

File tree

3 files changed

+203
-16
lines changed

3 files changed

+203
-16
lines changed

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import { inject, injectable } from '@theia/core/shared/inversify';
66
import { Mutex } from 'async-mutex';
77
import {
88
ArduinoDaemon,
9-
assertSanitizedFqbn,
109
BoardsService,
1110
ExecutableService,
1211
sanitizeFqbn,
@@ -151,7 +150,6 @@ export class InoLanguage extends SketchContribution {
151150
}
152151
return;
153152
}
154-
assertSanitizedFqbn(fqbn);
155153
const fqbnWithConfig = await this.boardDataStore.appendConfigToFqbn(fqbn);
156154
if (!fqbnWithConfig) {
157155
throw new Error(

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

Lines changed: 94 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ export namespace BoardSearch {
180180
'Partner',
181181
'Arduino@Heart',
182182
] as const;
183-
export type Type = typeof TypeLiterals[number];
183+
export type Type = (typeof TypeLiterals)[number];
184184
export namespace Type {
185185
export function is(arg: unknown): arg is Type {
186186
return typeof arg === 'string' && TypeLiterals.includes(arg as Type);
@@ -347,7 +347,7 @@ export namespace Port {
347347

348348
export namespace Protocols {
349349
export const KnownProtocolLiterals = ['serial', 'network'] as const;
350-
export type KnownProtocol = typeof KnownProtocolLiterals[number];
350+
export type KnownProtocol = (typeof KnownProtocolLiterals)[number];
351351
export namespace KnownProtocol {
352352
export function is(protocol: unknown): protocol is KnownProtocol {
353353
return (
@@ -476,29 +476,109 @@ export namespace ConfigOption {
476476
fqbn: string,
477477
configOptions: ConfigOption[]
478478
): string {
479+
const failInvalidFQBN = (): never => {
480+
throw new Error(`Invalid FQBN: ${fqbn}`);
481+
};
482+
483+
const [vendor, arch, id, rest] = fqbn.split(':');
484+
if (!vendor || !arch || !id) {
485+
return failInvalidFQBN();
486+
}
487+
479488
if (!configOptions.length) {
480489
return fqbn;
481490
}
482491

483-
const toValue = (values: ConfigValue[]) => {
492+
const existingOptions: Record<string, string> = {};
493+
if (rest) {
494+
// If rest exists, it must have the key=value(,key=value)* format. Otherwise, fail.
495+
const tuples = rest.split(',');
496+
for (const tuple of tuples) {
497+
const segments = tuple.split('=');
498+
if (segments.length !== 2) {
499+
failInvalidFQBN();
500+
}
501+
const [option, value] = segments;
502+
if (!option || !value) {
503+
failInvalidFQBN();
504+
}
505+
if (existingOptions[option]) {
506+
console.warn(
507+
`Config value already exists for '${option}' on FQBN. Skipping it. Existing value: ${existingOptions[option]}, new value: ${value}, FQBN: ${fqbn}`
508+
);
509+
} else {
510+
existingOptions[option] = value;
511+
}
512+
}
513+
}
514+
515+
const newOptions: Record<string, string> = {};
516+
for (const configOption of configOptions) {
517+
const { option, values } = configOption;
518+
if (!option) {
519+
console.warn(
520+
`Detected empty option on config options. Skipping it. ${JSON.stringify(
521+
configOption
522+
)}`
523+
);
524+
continue;
525+
}
484526
const selectedValue = values.find(({ selected }) => selected);
485-
if (!selectedValue) {
527+
if (selectedValue) {
528+
const { value } = selectedValue;
529+
if (!value) {
530+
console.warn(
531+
`Detected empty selected value on config options. Skipping it. ${JSON.stringify(
532+
configOption
533+
)}`
534+
);
535+
continue;
536+
}
537+
if (newOptions[option]) {
538+
console.warn(
539+
`Config value already exists for '${option}' in config options. Skipping it. Existing value: ${
540+
newOptions[option]
541+
}, new value: ${value}, config option: ${JSON.stringify(
542+
configOption
543+
)}`
544+
);
545+
} else {
546+
newOptions[option] = value;
547+
}
548+
} else {
486549
console.warn(
487-
`None of the config values was selected. Values were: ${JSON.stringify(
488-
values
550+
`None of the config values was selected. Config options was: ${JSON.stringify(
551+
configOption
489552
)}`
490553
);
491-
return undefined;
492554
}
493-
return selectedValue.value;
494-
};
495-
const options = configOptions
496-
.map(({ option, values }) => [option, toValue(values)])
497-
.filter(([, value]) => !!value)
555+
}
556+
557+
// Collect all options from the FQBN. Call them existing.
558+
// Collect all incoming options to decorate the FQBN with. Call them new.
559+
// To keep the order, iterate through the existing ones and append to FQBN.
560+
// If a new(er) value exists for the same option, use the new value.
561+
// If there is a new value, "mark" it as visited by deleting it from new. Otherwise, use the existing value.
562+
// Append all new ones to the FQBN.
563+
const mergedOptions: Record<string, string> = {};
564+
for (const existing of Object.entries(existingOptions)) {
565+
const [option, value] = existing;
566+
const newValue = newOptions[option];
567+
if (newValue) {
568+
mergedOptions[option] = newValue;
569+
delete newOptions[option];
570+
} else {
571+
mergedOptions[option] = value;
572+
}
573+
}
574+
Array.from(Object.entries(newOptions)).forEach(
575+
([option, value]) => (mergedOptions[option] = value)
576+
);
577+
578+
const configSuffix = Object.entries(mergedOptions)
498579
.map(([option, value]) => `${option}=${value}`)
499580
.join(',');
500-
501-
return `${fqbn}:${options}`;
581+
return `${vendor}:${arch}:${id}:${configSuffix}`;
502582
}
503583

504584
export class ConfigOptionError extends Error {

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

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { expect } from 'chai';
44
import {
55
AttachedBoardsChangeEvent,
66
BoardInfo,
7+
ConfigOption,
78
getBoardInfo,
89
noNativeSerialPort,
910
nonSerialPort,
@@ -93,6 +94,114 @@ describe('boards-service', () => {
9394
});
9495
});
9596

97+
describe('ConfigOption#decorate', () => {
98+
['invalid', 'a:b:c:foo=', 'a:b:c:foo=bar=baz', 'a:b:c:foo+bar'].map(
99+
(fqbn) =>
100+
it(`should error when the FQBN is invalid (FQBN: ${fqbn})`, () => {
101+
expect(() => ConfigOption.decorate('invalid', [])).to.throw(
102+
/^Invalid FQBN:*/
103+
);
104+
})
105+
);
106+
107+
it('should be noop when config options is empty', () => {
108+
const fqbn = 'a:b:c:menu1=value1';
109+
const actual = ConfigOption.decorate(fqbn, []);
110+
expect(actual).to.be.equal(fqbn);
111+
});
112+
113+
it('should not duplicate config options', () => {
114+
const fqbn = 'a:b:c:menu1=value1';
115+
const actual = ConfigOption.decorate(fqbn, [
116+
{
117+
label: 'Menu 1',
118+
option: 'menu1',
119+
values: [
120+
{ label: 'Value 1', value: 'value1', selected: true },
121+
{ label: 'Another Value', value: 'another1', selected: false },
122+
],
123+
},
124+
]);
125+
expect(actual).to.be.equal(fqbn);
126+
});
127+
128+
it('should update config options', () => {
129+
const fqbn = 'a:b:c:menu1=value1,menu2=value2';
130+
const actual = ConfigOption.decorate(fqbn, [
131+
{
132+
label: 'Menu 1',
133+
option: 'menu1',
134+
values: [
135+
{ label: 'Value 1', value: 'value1', selected: false },
136+
{ label: 'Another Value', value: 'another1', selected: true },
137+
],
138+
},
139+
]);
140+
expect(actual).to.be.equal('a:b:c:menu1=another1,menu2=value2');
141+
});
142+
143+
it('should favor first over rest when there are duplicate options (duplicate on FQBN)', () => {
144+
const fqbn = 'a:b:c:menu1=value1,menu1=value2';
145+
const actual = ConfigOption.decorate(fqbn, [
146+
{
147+
label: 'Menu 1',
148+
option: 'menu1',
149+
values: [
150+
{ label: 'Value 1', value: 'value1', selected: false },
151+
{ label: 'Another Value', value: 'another1', selected: true },
152+
],
153+
},
154+
]);
155+
expect(actual).to.be.equal('a:b:c:menu1=another1');
156+
});
157+
158+
it('should favor first over rest when there are duplicate options (duplicate in config)', () => {
159+
const fqbn = 'a:b:c';
160+
const actual = ConfigOption.decorate(fqbn, [
161+
{
162+
label: 'Menu 1',
163+
option: 'menu1',
164+
values: [
165+
{ label: 'Value 1', value: 'value1', selected: false },
166+
{ label: 'Another Value', value: 'another1', selected: true },
167+
],
168+
},
169+
{
170+
label: 'Menu 1',
171+
option: 'menu1',
172+
values: [
173+
{ label: 'Value 1', value: 'value1', selected: true },
174+
{ label: 'Another Value', value: 'another1', selected: false },
175+
],
176+
},
177+
]);
178+
expect(actual).to.be.equal('a:b:c:menu1=another1');
179+
});
180+
181+
it('should not change the existing config order', () => {
182+
const fqbn = 'a:b:c:menu1=value1';
183+
const actual = ConfigOption.decorate(fqbn, [
184+
{
185+
label: 'Menu 0',
186+
option: 'menu0',
187+
values: [
188+
{ label: 'Value 0', value: 'value0', selected: true },
189+
{ label: 'Another Value', value: 'another0', selected: false },
190+
],
191+
},
192+
{
193+
label: 'Menu 1',
194+
option: 'menu1',
195+
values: [
196+
{ label: 'Value 1', value: 'value1', selected: false },
197+
{ label: 'Another Value', value: 'another1', selected: true },
198+
],
199+
},
200+
]);
201+
expect(actual).to.be.equal('a:b:c:menu1=another1,menu0=value0');
202+
});
203+
});
204+
96205
describe('getBoardInfo', () => {
97206
const vid = '0x0';
98207
const pid = '0x1';

0 commit comments

Comments
 (0)