Conversation
There was a problem hiding this comment.
Pull request overview
Adds a built-in SVD-based Peripheral Viewer feature to the SiFli VS Code extension, integrating with the existing sifli-probe-rs debug workflow and extension configuration/localization setup.
Changes:
- Introduces a new
src/peripheral-viewer/module (SVD resolution/parsing, memory read/write helpers, tree view + commands, analyzer registry). - Hooks probe-rs debug adapter tracking to forward DAP “event” messages to the Peripheral Viewer (for stopped/continued refresh behavior).
- Updates extension activation/deactivation, package contributions (view/commands/config), localization bundles, and third-party license notices.
Reviewed changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Adds dependencies for xml2js and its typings (plus transitive type deps). |
| third-party-licenses/peripheral-viewer/NOTICE.txt | Adds upstream attribution + modification notes for extracted peripheral viewer code. |
| third-party-licenses/peripheral-viewer/LICENSE.txt | Adds MIT license text for upstream peripheral viewer sources. |
| src/probe-rs/extension.ts | Adds a listener API + tracker plumbing to observe outgoing DAP messages per debug session. |
| src/peripheral-viewer/views/peripheral-tree-provider.ts | Implements the TreeDataProvider managing session-backed peripheral trees, persistence, and refresh triggers. |
| src/peripheral-viewer/views/nodes/peripheralregisternode.ts | Adds register node implementation (formatting, tooltip, write-back via DAP writeMemory). |
| src/peripheral-viewer/views/nodes/peripheralnode.ts | Adds peripheral node implementation with address range chunking + DAP readMemory integration. |
| src/peripheral-viewer/views/nodes/peripheralfieldnode.ts | Adds field node implementation, including enum quick-picks and bitfield writes. |
| src/peripheral-viewer/views/nodes/peripheralclusternode.ts | Adds cluster node implementation for hierarchical register groupings. |
| src/peripheral-viewer/views/nodes/messagenode.ts | Adds a simple message node for empty/loading/error states. |
| src/peripheral-viewer/views/nodes/basenode.ts | Defines the node base classes and session propagation. |
| src/peripheral-viewer/utils.ts | Adds number formatting/bit utilities and a URL fetch helper. |
| src/peripheral-viewer/types.ts | Re-exports key types for external use. |
| src/peripheral-viewer/svd-resolver.ts | Resolves SVD path from launch overrides, workspace settings, or SDK-derived defaults. |
| src/peripheral-viewer/svd-parser.ts | Parses SVD XML (via xml2js) into node structures + derived/enumeration handling. |
| src/peripheral-viewer/peripherals-provider.ts | Loads/parses SVD with caching and returns per-session peripheral sets. |
| src/peripheral-viewer/memreadutils.ts | Implements DAP readMemory chunking and writeMemory helpers. |
| src/peripheral-viewer/manifest.ts | Centralizes IDs/keys/constants for the peripheral viewer feature. |
| src/peripheral-viewer/index.ts | Wires up activation, conflict detection, command registration, and analyzer registry exports. |
| src/peripheral-viewer/common.ts | Adds shared enums/types (formats, persisted node settings). |
| src/peripheral-viewer/commands.ts | Registers VS Code commands for update/copy/format/refresh/pin actions. |
| src/peripheral-viewer/analysis/types.ts | Defines analysis result types/severity for SVD analyzers. |
| src/peripheral-viewer/analysis/analyzer.ts | Adds analyzer interface + registry execution wrapper. |
| src/peripheral-viewer/addrranges.ts | Adds address-range chunking utility used for memory read planning. |
| src/extension.ts | Initializes/disposes the peripheral viewer as part of extension lifecycle. |
| package.nls.zh-cn.json | Adds localized titles/descriptions for peripheral viewer commands/view/config. |
| package.nls.json | Adds localized titles/descriptions for peripheral viewer commands/view/config. |
| package.json | Contributes the debug view, commands, context menus, and settings; adds xml2js deps and launch schema property. |
| l10n/bundle.l10n.zh-cn.json | Adds UI string localizations used by the peripheral viewer feature. |
| l10n/bundle.l10n.json | Adds UI string localizations used by the peripheral viewer feature. |
| .gitignore | Ignores designs/ folder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const child of children) { | ||
| child.setSession(session); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export abstract class ClusterOrRegisterBaseNode extends PeripheralBaseNode { |
There was a problem hiding this comment.
setSession() starts async propagation to children but does not await the recursive child.setSession(session) calls. This can leave deeper nodes without the correct session when subsequent operations run (and can cause cross-session writes when multiple debug sessions exist). Consider awaiting each child call (or await Promise.all(...)) so session assignment completes deterministically.
| for (const child of children) { | |
| child.setSession(session); | |
| } | |
| } | |
| } | |
| export abstract class ClusterOrRegisterBaseNode extends PeripheralBaseNode { | |
| await Promise.all(children.map(child => child.setSession(session))); | |
| } | |
| } | |
| export abstract class ClusterOrRegisterBaseNode extends PeripheralBaseNode { | |
| export abstract class ClusterOrRegisterBaseNode extends PeripheralBaseNode { |
| const register = new PeripheralRegisterNode(parent, { | ||
| ...baseOptions, | ||
| name: r.name[0], | ||
| description: description, | ||
| addressOffset: parseInteger(r.addressOffset[0]), |
There was a problem hiding this comment.
addressOffset is derived via parseInteger(...) which can return undefined, but the register node assumes a numeric offset (used in sorting/address math/formatting). Please validate the parse result and throw a clear error (or default) when addressOffset is missing/invalid, similar to the dim branch’s offsetbase validation.
| const register = new PeripheralRegisterNode(parent, { | |
| ...baseOptions, | |
| name: r.name[0], | |
| description: description, | |
| addressOffset: parseInteger(r.addressOffset[0]), | |
| const offset = parseInteger(r.addressOffset[0]); | |
| if (offset === undefined || offset < 0) { | |
| throw new Error( | |
| `Unable to parse SVD file: register ${r.name[0]} has invalid addressOffset ${r.addressOffset[0] ?? 'undefined'}` | |
| ); | |
| } | |
| const register = new PeripheralRegisterNode(parent, { | |
| ...baseOptions, | |
| name: r.name[0], | |
| description: description, | |
| addressOffset: offset, |
| const options: any = { | ||
| name: p.name[0], | ||
| baseAddress: parseInteger(p.baseAddress ? p.baseAddress[0] : '0'), |
There was a problem hiding this comment.
baseAddress is assigned from parseInteger(...) without a fallback/validation, so an invalid or missing <baseAddress> can propagate undefined into PeripheralNode and later crash formatting/address arithmetic. Consider ?? 0 with a validation error (preferred) when parsing fails so the user gets a meaningful SVD parse error instead of a runtime exception.
| const options: any = { | |
| name: p.name[0], | |
| baseAddress: parseInteger(p.baseAddress ? p.baseAddress[0] : '0'), | |
| let baseAddress = 0; | |
| if (p.baseAddress && p.baseAddress[0] !== undefined) { | |
| const parsedBaseAddress = parseInteger(p.baseAddress[0]); | |
| if (parsedBaseAddress === undefined) { | |
| const peripheralName = p.name && p.name[0] ? p.name[0] : '<unknown>'; | |
| throw new Error(`Invalid baseAddress '${p.baseAddress[0]}' for peripheral '${peripheralName}' in SVD file.`); | |
| } | |
| baseAddress = parsedBaseAddress; | |
| } | |
| const options: any = { | |
| name: p.name[0], | |
| baseAddress: baseAddress, |
| const peripherials = []; | ||
| for (const key in peripheralMap) { | ||
| peripherials.push(this.parsePeripheral(peripheralMap[key], defaultOptions)); | ||
| } | ||
|
|
||
| peripherials.sort(PeripheralNode.compare); | ||
|
|
||
| for (const p of peripherials) { | ||
| p.resolveDeferedEnums(this.enumTypeValuesMap); // This can throw an exception | ||
| p.collectRanges(); | ||
| } | ||
|
|
||
| return peripherials; |
There was a problem hiding this comment.
The local variable peripherials appears to be a misspelling of "peripherals". Renaming it will improve readability and avoid propagating the typo into future refactors.
| const peripherials = []; | |
| for (const key in peripheralMap) { | |
| peripherials.push(this.parsePeripheral(peripheralMap[key], defaultOptions)); | |
| } | |
| peripherials.sort(PeripheralNode.compare); | |
| for (const p of peripherials) { | |
| p.resolveDeferedEnums(this.enumTypeValuesMap); // This can throw an exception | |
| p.collectRanges(); | |
| } | |
| return peripherials; | |
| const peripherals = []; | |
| for (const key in peripheralMap) { | |
| peripherals.push(this.parsePeripheral(peripheralMap[key], defaultOptions)); | |
| } | |
| peripherals.sort(PeripheralNode.compare); | |
| for (const p of peripherals) { | |
| p.resolveDeferedEnums(this.enumTypeValuesMap); // This can throw an exception | |
| p.collectRanges(); | |
| } | |
| return peripherals; |
| this.currentValue = buffer.readUInt32LE(0); | ||
| break; | ||
| default: | ||
| vscode.window.showErrorMessage(`Register ${this.name} has invalid size: ${this.size}. Should be 8, 16 or 32.`); |
There was a problem hiding this comment.
This error message is shown directly to users but is not localized. Please wrap it with vscode.l10n.t(...) (and add the corresponding bundle entry) to match the rest of the extension’s localization approach.
| vscode.window.showErrorMessage(`Register ${this.name} has invalid size: ${this.size}. Should be 8, 16 or 32.`); | |
| vscode.window.showErrorMessage(vscode.l10n.t( | |
| 'cortex-debug.peripheral.registerInvalidSize', | |
| 'Register {0} has invalid size: {1}. Should be 8, 16 or 32.', | |
| this.name, | |
| this.size | |
| )); |
| } catch (error) { | ||
| logToConsole( | ||
| `${ConsoleLogSources.error}: Failed to notify probe-rs debug listeners: ${JSON.stringify(error)}`, | ||
| true | ||
| ); |
There was a problem hiding this comment.
JSON.stringify(error) typically serializes to {} for Error objects (and can throw on circular structures), so this log line may hide the actual failure reason. Consider logging error instanceof Error ? (error.stack ?? error.message) : String(error) (or similar) to preserve useful diagnostics.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 48 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): AnalysisBucketPresentation[] { | ||
| const buckets: AnalysisBucketPresentation[] = []; | ||
| const definitions: Array<{ id: AnalysisBucketId; label: string; enabled: boolean }> = [ | ||
| { | ||
| id: 'error', | ||
| label: 'Errors', | ||
| enabled: | ||
| filters.status !== 'clean' && (filters.severity === 'all' || filters.severity === AnalysisSeverity.Error), | ||
| }, | ||
| { | ||
| id: 'warning', | ||
| label: 'Warnings', | ||
| enabled: | ||
| filters.status !== 'clean' && (filters.severity === 'all' || filters.severity === AnalysisSeverity.Warning), | ||
| }, | ||
| { | ||
| id: 'clean', | ||
| label: 'Clean', | ||
| enabled: filters.severity === 'all' && filters.status !== 'issues', | ||
| }, | ||
| { | ||
| id: 'not-analyzed', | ||
| label: 'Not Analyzed', | ||
| enabled: filters.severity === 'all' && filters.status === 'all', | ||
| }, |
There was a problem hiding this comment.
These bucket labels are hard-coded English strings ("Errors", "Warnings", "Clean", "Not Analyzed") while other user-facing strings in this feature use vscode.l10n.t. To keep the view localizable, wrap these labels with vscode.l10n.t (and add the corresponding entries to l10n bundles).
| if (!sessionState) { | ||
| return { | ||
| hasActiveSession: false, | ||
| viewMode, | ||
| filters, | ||
| availableGroups, | ||
| summary: createSummary(undefined, []), | ||
| groups: [], | ||
| buckets: [], | ||
| message: 'Peripheral analysis is available when a sifli-probe-rs debug session is active.', | ||
| }; |
There was a problem hiding this comment.
buildAnalysisPresentation returns a user-facing fallback message as a plain string. Since this message can surface in the UI, it should be localized via vscode.l10n.t (and backed by l10n bundle entries), consistent with the rest of the extension’s user messages.
| if (element.kind === 'instance') { | ||
| if (element.instance.findings.length === 0) { | ||
| if (element.instance.status === 'ok') { | ||
| return [new AnalysisMessageNode(vscode.l10n.t('No issues found.'))]; | ||
| } | ||
| if (element.instance.status === 'not-analyzed') { | ||
| return [new AnalysisMessageNode(vscode.l10n.t('Run analysis to inspect this peripheral.'))]; | ||
| } | ||
| } | ||
|
|
||
| return element.instance.findings.map(finding => new AnalysisFindingNode(finding)); | ||
| } | ||
|
|
||
| return []; | ||
| } | ||
|
|
||
| private getSnapshot(): AnalysisPresentationSnapshot { | ||
| const sessionState = this.runtime.getActiveSessionState(); | ||
| const filters = this.uiState.getFilters(sessionState); | ||
| return buildAnalysisPresentation(sessionState, this.uiState.getViewMode(), filters); | ||
| } | ||
|
|
||
| private updateViewMetadata(): void { | ||
| if (!this.view) { | ||
| return; | ||
| } | ||
|
|
||
| const snapshot = this.getSnapshot(); | ||
| this.view.description = this.describeView(snapshot); | ||
| this.view.badge = | ||
| snapshot.summary.issueCount > 0 | ||
| ? { | ||
| value: snapshot.summary.issueCount, | ||
| tooltip: vscode.l10n.t( | ||
| 'Visible findings: {0} (errors: {1}, warnings: {2})', | ||
| String(snapshot.summary.issueCount), | ||
| String(snapshot.summary.errorCount), | ||
| String(snapshot.summary.warningCount) | ||
| ), | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
New vscode.l10n.t strings introduced here (e.g., "Run analysis to inspect this peripheral.", "Visible findings: {0} (errors: {1}, warnings: {2})", "Errors", "Warnings", "All", "Issues", "Clean", "{0} groups", "{0}/{1} groups") are not present in l10n/bundle.l10n.json or l10n/bundle.l10n.zh-cn.json, so they won’t be translated. Please add these keys to both bundles to keep localization complete.
| private pickFallbackSessionId(): string | undefined { | ||
| return this.sessionTrees.keys().next().value; | ||
| } |
There was a problem hiding this comment.
pickFallbackSessionId() returns this.sessionTrees.keys().next().value, which is typed as a string by the iterator even when the map is empty (at runtime it becomes undefined). With strict TS settings this can hide an undefined case and cause type unsoundness. Prefer checking the iterator result’s done flag and returning undefined explicitly when there are no sessions.
| let numval: number; | ||
| if (val.match(this.hexRegex)) { | ||
| numval = parseInt(val.substr(2), 16); | ||
| } else if (val.match(this.binaryRegex)) { | ||
| numval = parseInt(val.substr(2), 2); | ||
| } else if (val.match(/^[0-9]+/)) { | ||
| numval = parseInt(val, 10); | ||
| if (numval >= this.maxValue) { | ||
| throw new Error(`Value entered (${numval}) is greater than the maximum value of ${this.maxValue}`); | ||
| } |
There was a problem hiding this comment.
Decimal parsing uses /^[0-9]+/ which matches prefixes, so inputs like "123abc" will be accepted and parsed as 123. Also val.substr(...) is deprecated. Consider tightening the decimal regex to /^
?\d+$/ (or equivalent) and using slice(2) for hex/binary parsing to avoid accepting malformed input and to remove deprecated APIs.
| for (const listener of probeRsDidSendMessageListeners) { | ||
| try { | ||
| listener(this.session, message); | ||
| } catch (error) { | ||
| logToConsole( | ||
| `${ConsoleLogSources.error}: Failed to notify probe-rs debug listeners: ${JSON.stringify(error)}`, | ||
| true | ||
| ); | ||
| } |
There was a problem hiding this comment.
In the listener error path, JSON.stringify(error) usually renders "{}" for Error objects (losing message/stack), making debugging harder. Prefer logging error instanceof Error ? error.stack ?? error.message : String(error), or include both message and stack explicitly.
| function createBuckets( | ||
| groups: AnalysisGroupPresentation[], | ||
| filters: AnalysisFilterState | ||
| ): AnalysisBucketPresentation[] { | ||
| const buckets: AnalysisBucketPresentation[] = []; | ||
| const definitions: Array<{ id: AnalysisBucketId; label: string; enabled: boolean }> = [ | ||
| { | ||
| id: 'error', | ||
| label: 'Errors', | ||
| enabled: | ||
| filters.status !== 'clean' && (filters.severity === 'all' || filters.severity === AnalysisSeverity.Error), | ||
| }, | ||
| { | ||
| id: 'warning', | ||
| label: 'Warnings', | ||
| enabled: | ||
| filters.status !== 'clean' && (filters.severity === 'all' || filters.severity === AnalysisSeverity.Warning), | ||
| }, | ||
| { | ||
| id: 'clean', | ||
| label: 'Clean', | ||
| enabled: filters.severity === 'all' && filters.status !== 'issues', | ||
| }, | ||
| { | ||
| id: 'not-analyzed', | ||
| label: 'Not Analyzed', | ||
| enabled: filters.severity === 'all' && filters.status === 'all', | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
AnalysisSeverity includes "info", and issueCount counts all findings, but the severity-bucket view only buckets error/warning/clean/not-analyzed. If any analyzer returns info findings, they won’t appear in the "severity" view (and counts/labels like describeCounts can incorrectly fall back to "OK"). Either add an "info" bucket (and include info in count/labels/tooltips) or restrict findings to warning/error so the presentation logic stays consistent.
| private generateTooltipMarkdown(): vscode.MarkdownString | null { | ||
| const mds = new vscode.MarkdownString('', true); | ||
| mds.isTrusted = true; | ||
|
|
||
| const address = `${hexFormat(this.getAddress())}`; | ||
|
|
||
| const formattedValue = this.getFormattedValue(this.getFormat()); | ||
|
|
||
| const roLabel = this.accessType === AccessType.ReadOnly ? '(Read Only)' : ' '; | ||
|
|
||
| mds.appendMarkdown(`| ${this.name}@${address} | ${roLabel} | *${formattedValue}* |\n`); | ||
| mds.appendMarkdown('|:---|:---:|---:|\n\n'); | ||
|
|
||
| if (this.accessType !== AccessType.WriteOnly) { | ||
| mds.appendMarkdown(`**Reset Value:** ${this.getFormattedResetValue(this.getFormat())}\n`); | ||
| } | ||
|
|
||
| mds.appendMarkdown('\n____\n\n'); | ||
| if (this.description) { | ||
| mds.appendMarkdown(this.description); | ||
| } |
There was a problem hiding this comment.
This tooltip uses mds.isTrusted = true and then appends this.description sourced from the SVD file. A malicious/compromised SVD could embed command links (command: URIs) or other trusted markdown behaviors, leading to command execution when clicked. Consider leaving tooltips untrusted (default), or only trusting content you fully control and sanitizing/escaping SVD-provided descriptions.
| private generateTooltipMarkdown(isReserved: boolean): vscode.MarkdownString | null { | ||
| const mds = new vscode.MarkdownString('', true); | ||
| mds.isTrusted = true; | ||
|
|
||
| const address = `${hexFormat(this.parent.getAddress())}${this.getFormattedRange()}`; | ||
|
|
||
| if (isReserved) { | ||
| mds.appendMarkdown(`| ${this.name}@${address} | | *Reserved* |\n`); | ||
| mds.appendMarkdown('|:---|:---:|---:|'); | ||
| return mds; | ||
| } | ||
|
|
||
| const formattedValue = this.getFormattedValue(this.getFormat(), true); | ||
|
|
||
| const roLabel = this.accessType === AccessType.ReadOnly ? '(Read Only)' : ' '; | ||
|
|
||
| mds.appendMarkdown(`| ${this.name}@${address} | ${roLabel} | *${formattedValue}* |\n`); | ||
| mds.appendMarkdown('|:---|:---:|---:|\n\n'); | ||
|
|
||
| if (this.accessType !== AccessType.WriteOnly) { | ||
| mds.appendMarkdown(`**Reset Value:** ${this.formatValue(this.getResetValue(), this.getFormat())}\n`); | ||
| } | ||
|
|
||
| mds.appendMarkdown('\n____\n\n'); | ||
| mds.appendMarkdown(this.description); | ||
|
|
||
| mds.appendMarkdown('\n_____\n\n'); |
There was a problem hiding this comment.
This tooltip sets mds.isTrusted = true and includes field/enum descriptions originating from the SVD file. Because SVD is an external input, this can allow command links (command: URIs) or other trusted markdown actions in the tooltip. Please avoid trusting SVD-provided markdown (keep tooltips untrusted, or sanitize/escape descriptions before appending).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 50 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const channelWriteEmitter = new vscode.EventEmitter<string>(); | ||
| let entry: ProbeRsTerminalEntry; | ||
| const channelPty: vscode.Pseudoterminal = { | ||
| onDidWrite: channelWriteEmitter.event, | ||
| open: () => { | ||
| this.notifyRttWindowState(entry, true, 'opened'); | ||
| }, | ||
| close: () => { | ||
| this.notifyRttWindowState(entry, false, 'closed'); | ||
| }, | ||
| handleInput: this.isUartConsoleChannel(channelNumber, channelName) | ||
| ? data => { | ||
| this.handleTerminalInput(entry, data); | ||
| } | ||
| : undefined, | ||
| }; |
There was a problem hiding this comment.
channelPty.open/close/handleInput capture entry before it is assigned (it’s declared let entry and only set after createTerminal). If VS Code invokes open() immediately on terminal creation, this will throw at runtime. Consider constructing the entry object first (or passing the needed primitives into notifyRttWindowState/handleTerminalInput), then creating the terminal and assigning it onto the entry, so callbacks never observe an uninitialized reference.
| for (const listener of probeRsDidSendMessageListeners) { | ||
| try { | ||
| listener(this.session, message); | ||
| } catch (error) { | ||
| logToConsole( | ||
| `${ConsoleLogSources.error}: Failed to notify probe-rs debug listeners: ${JSON.stringify(error)}`, | ||
| true | ||
| ); | ||
| } |
There was a problem hiding this comment.
JSON.stringify(error) often produces {} for Error instances (because message/stack are non-enumerable), which makes these logs much less useful when a listener throws. Consider logging String(error) or error instanceof Error ? error.stack ?? error.message : String(error) instead.
No description provided.