From 55a8c5bf90d5f4520ed8dc55df64af044bd4e2d0 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 6 Feb 2024 12:50:00 -0300 Subject: [PATCH 1/4] Add tests for dtoverlay management in config.txt --- test/integration/config/config-txt.spec.ts | 137 ++++++++++++++++++++- test/integration/device-config.spec.ts | 12 +- 2 files changed, 142 insertions(+), 7 deletions(-) diff --git a/test/integration/config/config-txt.spec.ts b/test/integration/config/config-txt.spec.ts index b982f3da..ca1a5acb 100644 --- a/test/integration/config/config-txt.spec.ts +++ b/test/integration/config/config-txt.spec.ts @@ -4,6 +4,7 @@ import { stripIndent } from 'common-tags'; import { expect } from 'chai'; import * as hostUtils from '~/lib/host-utils'; +import { promises as fs } from 'fs'; import { ConfigTxt } from '~/src/config/backends/config-txt'; describe('config/config-txt', () => { @@ -18,7 +19,8 @@ describe('config/config-txt', () => { avoid_warnings=1 gpu_mem=16 hdmi_force_hotplug:1=1 - dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13`, + dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13 + dtparam=gpio_out_pin=17`, }).enable(); const configTxt = new ConfigTxt(); @@ -26,18 +28,147 @@ describe('config/config-txt', () => { // Will try to parse /test/data/mnt/boot/config.txt await expect(configTxt.getBootConfig()).to.eventually.deep.equal({ dtparam: ['i2c=on', 'audio=on'], - dtoverlay: ['ads7846', 'lirc-rpi,gpio_out_pin=17,gpio_in_pin=13'], + dtoverlay: [ + 'ads7846', + 'lirc-rpi,gpio_out_pin=17,gpio_in_pin=13,gpio_out_pin=17', + ], enable_uart: '1', avoid_warnings: '1', gpu_mem: '16', initramfs: 'initramf.gz 0x00800000', - // This syntax is supported by the backend but not the cloud side 'hdmi_force_hotplug:1': '1', }); await tfs.restore(); }); + it('correctly parses a config.txt file with an empty overlay', async () => { + const tfs = await testfs({ + [hostUtils.pathOnBoot('config.txt')]: stripIndent` + initramfs initramf.gz 0x00800000 + dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13 + dtparam=gpio_out_pin=17 + enable_uart=1 + avoid_warnings=1 + dtoverlay= + dtparam=i2c=on + dtparam=audio=on + dtoverlay=ads7846 + gpu_mem=16 + hdmi_force_hotplug:1=1 + `, + }).enable(); + + const configTxt = new ConfigTxt(); + + // Will try to parse /test/data/mnt/boot/config.txt + await expect(configTxt.getBootConfig()).to.eventually.deep.equal({ + dtparam: ['i2c=on', 'audio=on'], + dtoverlay: [ + 'lirc-rpi,gpio_out_pin=17,gpio_in_pin=13,gpio_out_pin=17', + 'ads7846', + ], + enable_uart: '1', + avoid_warnings: '1', + gpu_mem: '16', + initramfs: 'initramf.gz 0x00800000', + 'hdmi_force_hotplug:1': '1', + }); + + await tfs.restore(); + }); + + it('maintains ordering of dtoverlays and dtparams', async () => { + const tfs = await testfs({ + [hostUtils.pathOnBoot('config.txt')]: stripIndent` + initramfs initramf.gz 0x00800000 + dtparam=i2c=on + dtparam=audio=on + dtoverlay=ads7846 + enable_uart=1 + avoid_warnings=1 + gpu_mem=16 + hdmi_force_hotplug:1=1 + dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13 + dtoverlay=ads1015,addr=0x48,cha_enable=true + dtparam=chb_enable=true + dtparam=chc_enable=true + `, + }).enable(); + + const configTxt = new ConfigTxt(); + + // Will try to parse /test/data/mnt/boot/config.txt + await expect(configTxt.getBootConfig()).to.eventually.deep.equal({ + dtparam: ['i2c=on', 'audio=on'], + dtoverlay: [ + 'ads7846', + 'lirc-rpi,gpio_out_pin=17,gpio_in_pin=13', + 'ads1015,addr=0x48,cha_enable=true,chb_enable=true,chc_enable=true', + ], + enable_uart: '1', + avoid_warnings: '1', + gpu_mem: '16', + initramfs: 'initramf.gz 0x00800000', + 'hdmi_force_hotplug:1': '1', + }); + + await tfs.restore(); + }); + + it('splits dtoverlays into params to stay under the 80 char limit', async () => { + const tfs = await testfs({ + [hostUtils.pathOnBoot('config.txt')]: stripIndent` + enable_uart=1 + dtparam=i2c_arm=on + dtparam=spi=on + disable_splash=1 + dtparam=audio=on + gpu_mem=16 + `, + }).enable(); + + const configTxt = new ConfigTxt(); + + await configTxt.setBootConfig({ + dtparam: ['i2c=on', 'audio=on'], + dtoverlay: [ + 'ads7846', + 'lirc-rpi,gpio_out_pin=17,gpio_in_pin=13', + 'ads1015,addr=0x48,cha_enable=true,chb_enable=true', + ], + enable_uart: '1', + avoid_warnings: '1', + gpu_mem: '256', + initramfs: 'initramf.gz 0x00800000', + 'hdmi_force_hotplug:1': '1', + }); + + // Confirm that the file was written correctly + await expect( + fs.readFile(hostUtils.pathOnBoot('config.txt'), 'utf8'), + ).to.eventually.equal( + stripIndent` + dtparam=i2c=on + dtparam=audio=on + enable_uart=1 + avoid_warnings=1 + gpu_mem=256 + initramfs initramf.gz 0x00800000 + hdmi_force_hotplug:1=1 + dtoverlay=ads7846 + dtoverlay=lirc-rpi + dtparam=gpio_out_pin=17 + dtparam=gpio_in_pin=13 + dtoverlay=ads1015 + dtparam=addr=0x48 + dtparam=cha_enable=true + dtparam=chb_enable=true + ` + '\n', + ); + await tfs.restore(); + }); + it('ensures required fields are written to config.txt', async () => { const tfs = await testfs({ [hostUtils.pathOnBoot('config.txt')]: stripIndent` diff --git a/test/integration/device-config.spec.ts b/test/integration/device-config.spec.ts index 2997c126..1b7ac1f9 100644 --- a/test/integration/device-config.spec.ts +++ b/test/integration/device-config.spec.ts @@ -260,10 +260,12 @@ describe('device-config', () => { initramfs initramf.gz 0x00800000 dtparam=i2c=on dtparam=audio=off - dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13 - dtoverlay=balena-fin foobar=bat foobaz=bar + dtoverlay=lirc-rpi + dtparam=gpio_out_pin=17 + dtparam=gpio_in_pin=13 + dtoverlay=balena-fin ` + '\n', // add newline because stripIndent trims last newline ); }); @@ -301,10 +303,12 @@ describe('device-config', () => { initramfs initramf.gz 0x00800000 dtparam=i2c=on dtparam=audio=off - dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13 - dtoverlay=balena-fin foobar=bat foobaz=bar + dtoverlay=lirc-rpi + dtparam=gpio_out_pin=17 + dtparam=gpio_in_pin=13 + dtoverlay=balena-fin ` + '\n', // add newline because stripIndent trims last newline ); }); From a8e371f0c9bff61dbc761c342b51a539416d7686 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Tue, 6 Feb 2024 19:20:12 -0300 Subject: [PATCH 2/4] Refactor config-txt backend Cleans up code and adds better type detection --- src/config/backends/config-txt.ts | 62 ++++++++++++++++++------------- 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/src/config/backends/config-txt.ts b/src/config/backends/config-txt.ts index 87942764..9c77fdab 100644 --- a/src/config/backends/config-txt.ts +++ b/src/config/backends/config-txt.ts @@ -6,6 +6,26 @@ import log from '../../lib/supervisor-console'; import { exists } from '../../lib/fs-utils'; import * as hostUtils from '../../lib/host-utils'; +const ARRAY_CONFIGS = [ + 'dtparam', + 'dtoverlay', + 'device_tree_param', + 'device_tree_overlay', + 'gpio', +] as const; + +type ArrayConfig = typeof ARRAY_CONFIGS[number]; + +// Refinement on the ConfigOptions type +// to indicate what properties are arrays +type ConfigTxtOptions = ConfigOptions & { + [key in ArrayConfig]?: string[]; +}; + +function isArrayConfig(x: string): x is ArrayConfig { + return x != null && ARRAY_CONFIGS.includes(x as any); +} + /** * A backend to handle Raspberry Pi host configuration * @@ -16,7 +36,6 @@ import * as hostUtils from '../../lib/host-utils'; * - {BALENA|RESIN}_HOST_CONFIG_device_tree_overlay = value | "value" | "value1","value2" * - {BALENA|RESIN}_HOST_CONFIG_gpio = value | "value" | "value1","value2" */ - export class ConfigTxt extends ConfigBackend { private static bootConfigVarPrefix = `${constants.hostConfigVarPrefix}CONFIG_`; private static bootConfigPath = hostUtils.pathOnBoot('config.txt'); @@ -25,13 +44,6 @@ export class ConfigTxt extends ConfigBackend { '(?:' + _.escapeRegExp(ConfigTxt.bootConfigVarPrefix) + ')(.+)', ); - private static arrayConfigKeys = [ - 'dtparam', - 'dtoverlay', - 'device_tree_param', - 'device_tree_overlay', - 'gpio', - ]; private static forbiddenConfigKeys = [ 'disable_commandline_tags', 'cmdline', @@ -69,39 +81,38 @@ export class ConfigTxt extends ConfigBackend { 'utf-8', ); } else { - await hostUtils.writeToBoot(ConfigTxt.bootConfigPath, ''); + return {}; } - const conf: ConfigOptions = {}; + const conf: ConfigTxtOptions = {}; const configStatements = configContents.split(/\r?\n/); for (const configStr of configStatements) { // Don't show warnings for comments and empty lines - const trimmed = _.trimStart(configStr); + const trimmed = configStr.trimStart(); if (trimmed.startsWith('#') || trimmed === '') { continue; } + + // Try to split the line into key+value let keyValue = /^([^=]+)=(.*)$/.exec(configStr); if (keyValue != null) { const [, key, value] = keyValue; - if (!ConfigTxt.arrayConfigKeys.includes(key)) { + if (!isArrayConfig(key)) { + // If key is not one of the array configs, just add it to the + // configuration conf[key] = value; } else { - if (conf[key] == null) { - conf[key] = []; - } - const confArr = conf[key]; - if (!Array.isArray(confArr)) { - throw new Error( - `Expected '${key}' to have a config array but got ${typeof confArr}`, - ); - } - confArr.push(value); + // TODO: dtparams and dtoverlays need to be treated as a special case + // Otherwise push the new value to the array + const arrayConf = conf[key] == null ? [] : conf[key]!; + arrayConf.push(value); } continue; } - // Try the next regex instead + // If the line does not match a key-value pair, we check + // if it is initramfs, otherwise ignore it keyValue = /^(initramfs) (.+)/.exec(configStr); if (keyValue != null) { const [, key, value] = keyValue; @@ -115,7 +126,7 @@ export class ConfigTxt extends ConfigBackend { } public async setBootConfig(opts: ConfigOptions): Promise { - const confStatements = _.flatMap(opts, (value, key) => { + const confStatements = Object.entries(opts).flatMap(([key, value]) => { if (key === 'initramfs') { return `${key} ${value}`; } else if (Array.isArray(value)) { @@ -124,6 +135,7 @@ export class ConfigTxt extends ConfigBackend { return `${key}=${value}`; } }); + // TODO: split dtoverlay into dtparams const confStr = `${confStatements.join('\n')}\n`; await hostUtils.writeToBoot(ConfigTxt.bootConfigPath, confStr); } @@ -141,7 +153,7 @@ export class ConfigTxt extends ConfigBackend { } public processConfigVarValue(key: string, value: string): string | string[] { - if (ConfigTxt.arrayConfigKeys.includes(key)) { + if (isArrayConfig(key)) { if (!value.startsWith('"')) { return [value]; } else { From 9546a1a3b1b919649dc401b63f0ff0dedad918b0 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Wed, 7 Feb 2024 17:12:52 -0300 Subject: [PATCH 3/4] Fix processing of dtoverlay/dtparams on config.txt DT overlays and DT params need to be consumed in the order that they appear on the file. DT params apply to the last dtoverlay defined on the file, or to the base overlay. This commit updates config.txt parsing to consider this ordering, and it also ensures global dtparams are written first so they cannot be overriden by later overlays. Because of the more strict parsing method, it is possible that existing HOST_CONFIG vars do not match the interpretation of the parser. If that's the case, the supervisor will re-apply the target state which will cause the device to reboot. Change-type: major --- src/config/backends/config-txt.ts | 81 +++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 14 deletions(-) diff --git a/src/config/backends/config-txt.ts b/src/config/backends/config-txt.ts index 9c77fdab..3a856c25 100644 --- a/src/config/backends/config-txt.ts +++ b/src/config/backends/config-txt.ts @@ -26,6 +26,11 @@ function isArrayConfig(x: string): x is ArrayConfig { return x != null && ARRAY_CONFIGS.includes(x as any); } +// The DTOverlays type is a collection of DtParams +type DTParam = string; +type DTOverlays = { [name: string]: DTParam[] }; +const BASE_OVERLAY = ''; + /** * A backend to handle Raspberry Pi host configuration * @@ -87,6 +92,9 @@ export class ConfigTxt extends ConfigBackend { const conf: ConfigTxtOptions = {}; const configStatements = configContents.split(/\r?\n/); + let currOverlay = BASE_OVERLAY; + const dtOverlays: DTOverlays = { [BASE_OVERLAY]: [] }; + for (const configStr of configStatements) { // Don't show warnings for comments and empty lines const trimmed = configStr.trimStart(); @@ -103,10 +111,27 @@ export class ConfigTxt extends ConfigBackend { // configuration conf[key] = value; } else { - // TODO: dtparams and dtoverlays need to be treated as a special case - // Otherwise push the new value to the array - const arrayConf = conf[key] == null ? [] : conf[key]!; - arrayConf.push(value); + // dtparams and dtoverlays need to be treated as a special case + if (key === 'dtparam') { + dtOverlays[currOverlay].push(value); + } else if (key === 'dtoverlay') { + // Assume that the first element is the overlay name + // we don't validate that the value is well formed + const [overlay, ...params] = value.split(','); + + // Update the DTO for next dtparam + currOverlay = overlay; + if (dtOverlays[overlay] == null) { + dtOverlays[overlay] = []; + } + + // Add params to the list + dtOverlays[overlay].push(...params); + } else { + // Otherwise push the new value to the array + const arrayConf = conf[key] == null ? [] : conf[key]!; + arrayConf.push(value); + } } continue; } @@ -122,20 +147,48 @@ export class ConfigTxt extends ConfigBackend { } } + // Convert the base overlay to global dtparams + const baseOverlay = dtOverlays[BASE_OVERLAY]; + delete dtOverlays[BASE_OVERLAY]; + if (baseOverlay.length > 0) { + conf.dtparam = baseOverlay; + } + + // Convert dtoverlays to array format + const overlayEntries = Object.entries(dtOverlays); + if (overlayEntries.length > 0) { + conf.dtoverlay = overlayEntries.map(([overlay, params]) => + [overlay, ...params].join(','), + ); + } + return conf; } - public async setBootConfig(opts: ConfigOptions): Promise { - const confStatements = Object.entries(opts).flatMap(([key, value]) => { - if (key === 'initramfs') { - return `${key} ${value}`; - } else if (Array.isArray(value)) { - return value.map((entry) => `${key}=${entry}`); - } else { - return `${key}=${value}`; + public async setBootConfig(opts: ConfigTxtOptions): Promise { + const confStatements = Object.entries(opts) + // Treat dtoverlays separately + .filter(([key]) => key !== 'dtoverlay') + .flatMap(([key, value]) => { + if (key === 'initramfs') { + return `${key} ${value}`; + } else if (Array.isArray(value)) { + return value.map((entry) => `${key}=${entry}`); + } else { + return `${key}=${value}`; + } + }); + + // Split dtoverlays from their params to avoid running into char limits + // and write at the end to prevent overriding the base overlay + if (opts.dtoverlay != null) { + for (const entry of opts.dtoverlay) { + const [overlay, ...params] = entry.split(','); + confStatements.push(`dtoverlay=${overlay}`); + confStatements.push(...params.map((p) => `dtparam=${p}`)); } - }); - // TODO: split dtoverlay into dtparams + } + const confStr = `${confStatements.join('\n')}\n`; await hostUtils.writeToBoot(ConfigTxt.bootConfigPath, confStr); } From 6e6a796da5ecc846248eae4c8495bc626964c038 Mon Sep 17 00:00:00 2001 From: Felipe Lalanne <1822826+pipex@users.noreply.github.com> Date: Thu, 8 Feb 2024 12:05:12 -0300 Subject: [PATCH 4/4] Add special case for base DTO params on RPI config While ordering is important in the RPI firmware configuration file (config.txt), some dt params are by default considered part of the base dt overlay if they are not used by other overlays. Unfortunately the [list of dtparams](https://github.com/raspberrypi/firmware/blob/master/boot/overlays/README#L133) is too long to add all of them as exceptions, but we can add the params used in the default config.txt provided in OS images, to avoid reboots when updating to this new supervisor and correctly parsing the provisioning config.txt as variables. While this addition handles most common scenarios, there is still a chance a user may have use other base overlay dt params in the initial config, in which case those will be interpreted according to the relative ordering Change-type: patch --- src/config/backends/config-txt.ts | 28 ++++++++++++++++- test/integration/config/config-txt.spec.ts | 36 ++++++++++++++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/src/config/backends/config-txt.ts b/src/config/backends/config-txt.ts index 3a856c25..09539f91 100644 --- a/src/config/backends/config-txt.ts +++ b/src/config/backends/config-txt.ts @@ -31,6 +31,21 @@ type DTParam = string; type DTOverlays = { [name: string]: DTParam[] }; const BASE_OVERLAY = ''; +function isBaseParam(dtparam: string): boolean { + const match = /^([^=]+)=(.*)$/.exec(dtparam); + let key = dtparam; + if (match != null) { + key = match[1]; + } + + // These hardcoded params correspond to the params set + // in the default config.txt provided by balena for pi devices + if (['audio', 'i2c_arm', 'spi'].includes(key)) { + return true; + } + return false; +} + /** * A backend to handle Raspberry Pi host configuration * @@ -113,7 +128,18 @@ export class ConfigTxt extends ConfigBackend { } else { // dtparams and dtoverlays need to be treated as a special case if (key === 'dtparam') { - dtOverlays[currOverlay].push(value); + // The specification allows multiple params in a line + const params = value.split(','); + params.forEach((param) => { + if (isBaseParam(param)) { + // We make sure to put the base param in the right overlays + // since RPI doesn't seem to be too strict about the ordering + // when it comes to these base params + dtOverlays[BASE_OVERLAY].push(value); + } else { + dtOverlays[currOverlay].push(value); + } + }); } else if (key === 'dtoverlay') { // Assume that the first element is the overlay name // we don't validate that the value is well formed diff --git a/test/integration/config/config-txt.spec.ts b/test/integration/config/config-txt.spec.ts index ca1a5acb..5640f6a5 100644 --- a/test/integration/config/config-txt.spec.ts +++ b/test/integration/config/config-txt.spec.ts @@ -78,6 +78,42 @@ describe('config/config-txt', () => { await tfs.restore(); }); + it('correctly parses default params on config.txt', async () => { + const tfs = await testfs({ + [hostUtils.pathOnBoot('config.txt')]: stripIndent` + initramfs initramf.gz 0x00800000 + dtoverlay=lirc-rpi,gpio_out_pin=17,gpio_in_pin=13 + dtparam=gpio_out_pin=17 + enable_uart=1 + avoid_warnings=1 + dtparam=i2c_arm=on + dtparam=spi=on + dtparam=audio=on + dtoverlay=ads7846 + gpu_mem=16 + hdmi_force_hotplug:1=1 + `, + }).enable(); + + const configTxt = new ConfigTxt(); + + // Will try to parse /test/data/mnt/boot/config.txt + await expect(configTxt.getBootConfig()).to.eventually.deep.equal({ + dtparam: ['i2c_arm=on', 'spi=on', 'audio=on'], + dtoverlay: [ + 'lirc-rpi,gpio_out_pin=17,gpio_in_pin=13,gpio_out_pin=17', + 'ads7846', + ], + enable_uart: '1', + avoid_warnings: '1', + gpu_mem: '16', + initramfs: 'initramf.gz 0x00800000', + 'hdmi_force_hotplug:1': '1', + }); + + await tfs.restore(); + }); + it('maintains ordering of dtoverlays and dtparams', async () => { const tfs = await testfs({ [hostUtils.pathOnBoot('config.txt')]: stripIndent`