Extract Pname from debug config name to select correct SVD file (#458)

* extract pname and validate against processor list in cbuild-run
* extended utils.ts tests, fixed regexp
* renamed simple to multi-core.cbuild-run.yml and cleaned up/extended extended tests for multi-core case
* tests for SVD file selection based on pname

---------

Signed-off-by: Jens Reinecke <[email protected]>

Author: jreineckearm

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Change Log
 
+## Unreleased
+
+- Fixes [#439](https://github.com/Open-CMSIS-Pack/vscode-cmsis-debugger/issues/439): Peripheral Inspector receives wrong SVD file path for secondary core of a multi-core connection.
+
 ## 1.0.0
 
 - Removes `Preview` status from extension.

src/cbuild-run/__snapshots__/cbuild-run-reader.test.ts.snap

@@ -1,4 +1,4 @@
-// Jest Snapshot v1, https://goo.gl/fbAQLP
+// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing
 
 exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`] = `
 {
@@ -67,8 +67,16 @@ exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`]
   },
   "debugger": {
     "clock": 10000000,
-    "gdbserver": "My Server Options
-",
+    "gdbserver": [
+      {
+        "pname": "Core0",
+        "port": 3336,
+      },
+      {
+        "pname": "Core1",
+        "port": 3339,
+      },
+    ],
     "name": "<default>",
     "port": "swd",
     "start-pname": "Core0",
@@ -81,18 +89,19 @@ exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`]
     {
       "file": "out/MyApp.bin",
       "info": "generate by MyApp",
+      "load": "image",
       "type": "bin",
     },
     {
       "file": "out/out/MyApp.axf",
       "info": "generate by MyApp",
+      "load": "symbols",
       "type": "elf",
     },
   ],
   "programming": [
     {
       "algorithm": "\${CMSIS_PACK_ROOT}/MyVendor/MyDevice/1.0.0/Flash/algorithms/MyAlgorithm_Core0.FLM",
-      "default": true,
       "pname": "Core0",
       "ram-size": 131072,
       "ram-start": 536870912,
@@ -101,7 +110,6 @@ exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`]
     },
     {
       "algorithm": "\${CMSIS_PACK_ROOT}/MyVendor/MyDevice/1.0.0/Flash/algorithms/MyAlgorithm_Extern.FLM",
-      "default": true,
       "pname": "Core0",
       "ram-size": 131072,
       "ram-start": 536870912,
@@ -116,12 +124,16 @@ exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`]
       "pname": "Core0",
       "type": "svd",
     },
+    {
+      "file": "\${CMSIS_PACK_ROOT}/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core1.svd",
+      "pname": "Core1",
+      "type": "svd",
+    },
   ],
   "system-resources": {
     "memory": [
       {
         "access": "rx",
-        "default": true,
         "from-pack": "MyVendor::[email protected]",
         "name": "Flash",
         "pname": "Core0",
@@ -130,7 +142,6 @@ exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`]
       },
       {
         "access": "rwx",
-        "default": true,
         "from-pack": "MyVendor::[email protected]",
         "name": "SRAM0",
         "pname": "Core0",
@@ -139,7 +150,6 @@ exports[`CbuildRunReader Parser successfully parses a *.cbuild-run.yml file 1`]
       },
       {
         "access": "rwx",
-        "default": true,
         "from-pack": "MyVendor::[email protected]",
         "name": "SRAM1",
         "pname": "Core0",

src/cbuild-run/cbuild-run-reader.test.ts

@@ -16,7 +16,7 @@
 
 import { CbuildRunReader } from './cbuild-run-reader';
 
-const TEST_CBUILD_RUN_FILE = 'test-data/simple.cbuild-run.yml'; // Relative to repo root
+const TEST_CBUILD_RUN_FILE = 'test-data/multi-core.cbuild-run.yml'; // Relative to repo root
 const TEST_FILE_PATH = 'test-data/fileReaderTest.txt'; // Relative to repo root
 
 describe('CbuildRunReader', () => {
@@ -53,11 +53,37 @@ describe('CbuildRunReader', () => {
             cbuildRunReader = new CbuildRunReader();
         });
 
-        it('returns SVD file path', async () => {
-            const expectedSvdFilePaths = ['/my/pack/root/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core0.svd'];
+        it.each([
+            {
+                info: 'no pname',
+                pname: undefined,
+                expectedSvdPaths: [
+                    '/my/pack/root/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core0.svd',
+                    '/my/pack/root/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core1.svd'
+                ]
+            },
+            {
+                info: 'Core0',
+                pname: 'Core0',
+                expectedSvdPaths: [
+                    '/my/pack/root/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core0.svd',
+                ]
+            },
+            {
+                info: 'Core1',
+                pname: 'Core1',
+                expectedSvdPaths: [
+                    '/my/pack/root/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core1.svd'
+                ]
+            },
+        ])('returns SVD file path ($info)', async ({ pname, expectedSvdPaths }) => {
             await cbuildRunReader.parse(TEST_CBUILD_RUN_FILE);
-            const svdFilePaths = cbuildRunReader.getSvdFilePaths('/my/pack/root');
-            expect(svdFilePaths).toEqual(expectedSvdFilePaths);
+            const svdFilePaths = cbuildRunReader.getSvdFilePaths('/my/pack/root', pname);
+            expect(svdFilePaths.length).toEqual(svdFilePaths.length);
+            for (let i = 0; i < svdFilePaths.length; i++) {
+                // eslint-disable-next-line security/detect-object-injection
+                expect(expectedSvdPaths[i]).toEqual(svdFilePaths[i]);
+            }
         });
 
         it('returns empty SVD file path list if nothing is parsed', () => {

src/cbuild-run/cbuild-run-reader.ts

@@ -43,7 +43,7 @@ export class CbuildRunReader {
         }
     }
 
-    public getSvdFilePaths(cmsisPackRoot?: string): string[] {
+    public getSvdFilePaths(cmsisPackRoot?: string, pname?: string): string[] {
         if (!this.cbuildRun) {
             return [];
         }
@@ -56,10 +56,23 @@ export class CbuildRunReader {
         // Replace potential ${CMSIS_PACK_ROOT} placeholder
         const effectiveCmsisPackRoot = cmsisPackRoot ?? getCmsisPackRootPath();
         // Map to copies, leave originals untouched
-        const svdFilePaths = svdFileDescriptors.map(descriptor => `${effectiveCmsisPackRoot
+        const filteredSvdDescriptors = pname ? svdFileDescriptors.filter(descriptor => descriptor.pname === pname): svdFileDescriptors;
+        const svdFilePaths = filteredSvdDescriptors.map(descriptor => `${effectiveCmsisPackRoot
             ? descriptor.file.replaceAll(CMSIS_PACK_ROOT_ENVVAR, effectiveCmsisPackRoot)
             : descriptor.file}`);
         return svdFilePaths;
     }
 
+    public getPnames(): string[] {
+        if (!this.cbuildRun) {
+            return [];
+        }
+        const processors = this.cbuildRun['debug-topology']?.processors;
+        const pnameProcessors = processors?.filter(p => p.pname);
+        if (!pnameProcessors?.length) {
+            return [];
+        }
+        return pnameProcessors.map(p => p.pname!);
+    }
+
 }

src/debug-configuration/subproviders/base-configuration-provider.ts

@@ -18,6 +18,7 @@ import * as vscode from 'vscode';
 import { ExtendedGDBTargetConfiguration, GDBTargetConfiguration } from '../gdbtarget-configuration';
 import { CbuildRunReader } from '../../cbuild-run';
 import { logger } from '../../logger';
+import { extractPname } from '../../utils';
 
 export abstract class BaseConfigurationProvider implements vscode.DebugConfigurationProvider {
     protected _cbuildRunReader?: CbuildRunReader;
@@ -51,18 +52,38 @@ export abstract class BaseConfigurationProvider implements vscode.DebugConfigura
         return !this.parameterExists(paramName, params) && (!commandName || await this.commandExists(commandName));
     }
 
-    protected resolveSvdFile(debugConfiguration: GDBTargetConfiguration) {
+    protected resolveSvdFile(debugConfiguration: GDBTargetConfiguration, pname?: string) {
         const extDebugConfig = debugConfiguration as ExtendedGDBTargetConfiguration;
         const cbuildRunFilePath = debugConfiguration.cmsis?.cbuildRunFile;
         // 'definitionPath' is current default name for SVD file settings in Eclipse CDT Cloud Peripheral Inspector.
         if (extDebugConfig.definitionPath !== undefined || !cbuildRunFilePath?.length) {
             return;
         }
-        const svdFilePaths = this.cbuildRunReader.getSvdFilePaths(debugConfiguration?.target?.environment?.CMSIS_PACK_ROOT);
-        // Needs update when we better support multiple `debugger:` YAML nodes
+        const svdFilePaths = this.cbuildRunReader.getSvdFilePaths(debugConfiguration?.target?.environment?.CMSIS_PACK_ROOT, pname);
+        if (!svdFilePaths.length) {
+            // No SVD file found for config
+            return;
+        }
+        // Only one SVD file per pname should be left, log a warning if more
+        if (svdFilePaths.length > 1) {
+            let message = 'Found more than one SVD file';
+            if (pname) {
+                message += ` for Pname '${pname}'`;
+            }
+            message += ', using first';
+            logger.warn(message);
+        }
         extDebugConfig.definitionPath = svdFilePaths[0];
     }
 
+    protected extractPnameFromDebugConfig(debugConfiguration: GDBTargetConfiguration): string | undefined {
+        const pnames = this.cbuildRunReader.getPnames();
+        if (!pnames.length) {
+            return undefined;
+        }
+        return extractPname(debugConfiguration.name, pnames);
+    }
+
     protected abstract resolveServerParameters(debugConfiguration: GDBTargetConfiguration): Promise<GDBTargetConfiguration>;
 
     public async resolveDebugConfigurationWithSubstitutedVariables(
@@ -71,7 +92,7 @@ export abstract class BaseConfigurationProvider implements vscode.DebugConfigura
         _token?: vscode.CancellationToken
     ): Promise<vscode.DebugConfiguration | null | undefined> {
         await this.parseCbuildRunFile(debugConfiguration);
-        this.resolveSvdFile(debugConfiguration);
+        this.resolveSvdFile(debugConfiguration, this.extractPnameFromDebugConfig(debugConfiguration));
         return this.resolveServerParameters(debugConfiguration);
     }
 

src/debug-configuration/subproviders/generic-configuration-provider.test.ts

@@ -15,8 +15,11 @@
  */
 
 import { gdbTargetConfiguration, targetConfigurationFactory } from '../debug-configuration.factory';
+import { ExtendedGDBTargetConfiguration } from '../gdbtarget-configuration';
 import { GenericConfigurationProvider } from './generic-configuration-provider';
 
+const TEST_CBUILD_RUN_FILE = 'test-data/multi-core.cbuild-run.yml'; // Relative to repo root
+
 describe('GenericConfigurationProvider', () => {
 
     describe('resolveDebugConfigurationWithSubstitutedVariables', () => {
@@ -29,6 +32,25 @@ describe('GenericConfigurationProvider', () => {
             const debugConfig = await configProvider.resolveDebugConfigurationWithSubstitutedVariables(undefined, config, undefined);
             expect(debugConfig).toBeDefined();
         });
+
+        it.each([
+            { info: 'no pname', pname: undefined, expectedSvdPath: '/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core0.svd' },
+            { info: 'Core0', pname: 'Core0', expectedSvdPath: '/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core0.svd' },
+            { info: 'Core1', pname: 'Core1', expectedSvdPath: '/MyVendor/MyDevice/1.0.0/Debug/SVD/MyDevice_Core1.svd' },
+        ])('parses a cbuild-run file and returns pname and svd file paths ($info)', async ({ pname, expectedSvdPath }) => {
+            const configProvider = new GenericConfigurationProvider();
+            const config = gdbTargetConfiguration({
+                name: `${pname} probe@gdbserver (launch)`,
+                target: targetConfigurationFactory(),
+                cmsis: {
+                    cbuildRunFile: TEST_CBUILD_RUN_FILE
+                }
+            });
+            const debugConfig = await configProvider.resolveDebugConfigurationWithSubstitutedVariables(undefined, config, undefined);
+            const gdbTargetConfig = debugConfig as ExtendedGDBTargetConfiguration;
+            expect(gdbTargetConfig.definitionPath?.endsWith(expectedSvdPath)).toBeTruthy();
+        });
+
     });
 
 });

src/utils.test.ts

@@ -17,7 +17,7 @@
 jest.mock('path');
 import * as os from 'os';
 import * as path from 'path';
-import { getCmsisPackRootPath, isWindows } from './utils';
+import { extractPname, getCmsisPackRootPath, isWindows } from './utils';
 
 const CMSIS_PACK_ROOT_DEFAULT = 'mock/path';
 describe('getCmsisPackRoot', () => {
@@ -47,3 +47,46 @@ describe('getCmsisPackRoot', () => {
         process.env = originalProcessEnv;
     });
 });
+
+describe('extractPname', () => {
+
+    afterEach(() => {
+        jest.clearAllMocks();
+    });
+
+    it('extracts pname if first part of string but with now pname list', () => {
+        const result = extractPname('dev-ice_name01 probe@gdbserver');
+        expect(result).toEqual('dev-ice_name01');
+    });
+
+    it('extracts pname if first part of string and in pname list', () => {
+        const result = extractPname('dev-ice_name01 probe@gdbserver', ['dev2', 'dev-ice_name01']);
+        expect(result).toEqual('dev-ice_name01');
+    });
+
+    it('fails to extract if pname not first part of string but in pname list', () => {
+        const result = extractPname('prefix dev-ice_name01 probe@gdbserver', ['dev2', 'dev-ice_name01']);
+        expect(result).toBeUndefined();
+    });
+
+    it('fails to extract if pname first part of string but not in pname list', () => {
+        const result = extractPname('dev-ice_name01 probe@gdbserver', ['dev2', 'dev-ice_name03']);
+        expect(result).toBeUndefined();
+    });
+
+    it('fails to extract if first part contains char invalid in pname', () => {
+        const result = extractPname('dev-ice_*name01 probe@gdbserver', ['dev2', 'dev-ice_*name01']);
+        expect(result).toBeUndefined();
+    });
+
+    it('fails to extract if first part contains char invalid in pname and in pname list', () => {
+        const result = extractPname('dev-ice_*name01 probe@gdbserver', ['dev2', 'dev-ice_*name01']);
+        expect(result).toBeUndefined();
+    });
+
+    it('fails to extract if first part contains char invalid in pname and no pname list', () => {
+        const result = extractPname('dev-ice_*name01 probe@gdbserver');
+        expect(result).toBeUndefined();
+    });
+
+});

src/utils.ts

@@ -32,3 +32,20 @@ export const getCmsisPackRootPath = (): string => {
 
     return cmsisPackRootDefault;
 };
+
+export const extractPname = (configString: string, pnames?: string[]): string | undefined => {
+    const trimmedString = configString.trim();
+    // Config names in debugger templates are pretty free-form. Hence, can't do a lot
+    // of format validation without reading debugger templates. Only check if name
+    // begins with valid pname string, and if string is part of processor list.
+    const pnameRegexp = /^[-_A-Za-z0-9]+\s+.+$/;
+    if (!pnameRegexp.test(trimmedString)) {
+        // Not the right format, Pname is 'RestrictedString' in PDSC format.
+        return undefined;
+    }
+    const pname = trimmedString.slice(0, trimmedString.indexOf(' '));
+    if (!pnames || pnames.includes(pname)) {
+        return pname;
+    }
+    return undefined;
+};