Add customOpenOCDPath setting for custom openOCD to use in extension.#1829
Merged
Conversation
Co-authored-by: Copilot <copilot@github.com>
📝 WalkthroughWalkthroughAdds a resource-scoped Changes
Sequence Diagram(s)sequenceDiagram
participant VSCode as "VS Code Extension"
participant Config as "Workspace Settings\n(idf.customOpenOCDPath)"
participant OpenOCDMgr as "OpenOCDManager"
participant FS as "Filesystem (exists & exec)"
participant PATH as "PATH resolver"
participant Consumer as "Caller (prepareEnv / devkits / hints)"
VSCode->>Config: read `idf.customOpenOCDPath`
Config-->>OpenOCDMgr: provide configured path (may be empty)
OpenOCDMgr->>FS: check exists & executable (trimmed path)
alt configured path valid
FS-->>OpenOCDMgr: exists & executable
OpenOCDMgr-->>Consumer: return configured absolute path
else configured path empty/invalid
FS-->>OpenOCDMgr: not valid
OpenOCDMgr->>PATH: search for `openocd` / `openocd-esp32`
PATH-->>OpenOCDMgr: resolved path or null
OpenOCDMgr-->>Consumer: return PATH-resolved path or null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the idf.customOpenOCDPath configuration setting, allowing users to specify a custom OpenOCD executable. The changes span documentation, localization files, and the core logic for resolving the OpenOCD path across multiple modules, including the troubleshooting report generator. Review feedback suggests improving the implementation in OpenOCDManager by using asynchronous file existence checks instead of synchronous ones to maintain consistency with the rest of the codebase. Additionally, a minor wording adjustment was recommended for the support report output to ensure consistent labeling of configuration access entries.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/espIdf/setTarget/DevkitsCommand.ts (1)
60-69:⚠️ Potential issue | 🟠 MajorReuse the checked helper-script lookup for custom installs.
getOpenOcdPath()now accepts arbitrary binaries, but this code still assumesshare/openocd/espressif/tools/esp_detect_config.pylives two directories above the binary. That only holds for the bundled layout, so a valid custom install can now fail later with a missing script. Please reusegetScriptPath()here or validatescriptPathbefore launching Python.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/setTarget/DevkitsCommand.ts` around lines 60 - 69, The code in DevkitsCommand.ts constructs scriptPath relative to the openOcd binary which only works for the bundled layout; instead use the existing helper getScriptPath(openOcdPath, "esp_detect_config.py") (or call getScriptPath() with the binary path) so custom installs are supported, or if you prefer keep your own resolution then validate that scriptPath exists before spawning Python and surface a clear error if missing; update references to scriptPath and the call site that launches esp_detect_config.py to use the validated path returned by getScriptPath().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs_espressif/en/debugproject.rst`:
- Around line 95-96: The heading "Using a Custom OpenOCD Path" has an underline
that is shorter than the title; update the underline so it matches the exact
length of the title (use the same character used for other headings, e.g., '-'
repeated to match the title length) so Sphinx stops warning and the docs build
succeeds.
In `@src/espIdf/openOcd/openOcdManager.ts`:
- Around line 53-62: The code currently returns customOpenOcdPath after
existsSync; update getOpenOcdPath so that after reading idf.customOpenOCDPath
and trimming, you verify the path is a file and executable before returning it:
use fs.statSync or fs.lstatSync (ensure .isFile()) and fs.accessSync(path,
fs.constants.X_OK) (or async equivalents) to confirm executability, and if
either check fails, fall through to the existing isBinInPath("openocd",
modifiedEnv, ["openocd-esp32"]) call; reference the customOpenOcdPath variable,
the idfConf.readParameter call that retrieves it, and the isBinInPath helper to
locate where to add these checks.
In `@src/support/configurationAccess.ts`:
- Around line 62-68: The check is using read permission (constants.R_OK) for an
executable path; change the permission flag passed to canAccessFile so it checks
execute permission instead (use constants.X_OK) when setting
reportedResult.configurationAccess.customOpenOcdPath based on
reportedResult.configurationSettings.customOpenOcdPath; update the call site in
configurationAccess.ts (the canAccessFile invocation) to pass the execute
constant and ensure the constants symbol X_OK is available.
In `@src/support/configurationSettings.ts`:
- Line 69: The config lookup uses the wrong casing for the key: change the
conf.get call that populates customOpenOcdPath (in configurationSettings.ts) to
use "idf.customOpenOCDPath" (capital D) so it matches the package.json
contribution id; update the property access in the object initializer that
currently reads customOpenOcdPath: conf.get("idf.customOpenOcdPath") to
conf.get("idf.customOpenOCDPath") so the setting is actually populated for
reports/diagnostics.
In `@src/support/writeReport.ts`:
- Around line 75-77: The report lines that reference
reportedResult.configurationSettings.customOpenOcdPath should be corrected to
use the proper setting name casing and distinct label text: replace occurrences
that print "idf.customOpenOcdPath" with "idf.customOpenOCDPath" and make the
access/secondary row use a clearer label (e.g., "Configured OpenOCD path
(idf.customOpenOCDPath)" for the settings line and a distinct label like
"OpenOCD executable path detected" for the access/usage line); update both
instances around reportedResult.configurationSettings.customOpenOcdPath
(including the similar block at lines ~98-100) so the label text and casing are
consistent and not duplicated.
---
Outside diff comments:
In `@src/espIdf/setTarget/DevkitsCommand.ts`:
- Around line 60-69: The code in DevkitsCommand.ts constructs scriptPath
relative to the openOcd binary which only works for the bundled layout; instead
use the existing helper getScriptPath(openOcdPath, "esp_detect_config.py") (or
call getScriptPath() with the binary path) so custom installs are supported, or
if you prefer keep your own resolution then validate that scriptPath exists
before spawning Python and surface a clear error if missing; update references
to scriptPath and the call site that launches esp_detect_config.py to use the
validated path returned by getScriptPath().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23c7bd7a-df0d-4006-a624-623928122636
📒 Files selected for processing (20)
docs_espressif/en/debugproject.rstdocs_espressif/en/settings.rstdocs_espressif/zh_CN/debugproject.rstdocs_espressif/zh_CN/settings.rstpackage.jsonpackage.nls.es.jsonpackage.nls.jsonpackage.nls.pt.jsonpackage.nls.ru.jsonpackage.nls.zh-cn.jsonsrc/common/prepareEnv.tssrc/espIdf/hints/utils.tssrc/espIdf/openOcd/openOcdManager.tssrc/espIdf/setTarget/DevkitsCommand.tssrc/extension.tssrc/support/configurationAccess.tssrc/support/configurationSettings.tssrc/support/initReportObj.tssrc/support/types.tssrc/support/writeReport.ts
|
Download the artifacts for this pull request: |
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/espIdf/setTarget/DevkitsCommand.ts (1)
136-156: PreferLogger.errorfor caught exceptions.
Logger.errorNotify(...)changes the catch-path behavior and diverges from the project guideline for caught errors. Keep the logging call onLogger.error(...)and leave notification handling separate if you still want the user-facing message.As per coding guidelines: Use
Logger.error(message, error, "scope identifier")for caught errors, with a consistent scope identifier as the third argument.♻️ Suggested adjustment
- Logger.errorNotify(msg, error as Error, "DevkitsCommand"); + Logger.error(msg, error as Error, "DevkitsCommand"); OutputChannel.appendLine(msg, "ESP Detect Config"); OutputChannel.show();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/setTarget/DevkitsCommand.ts` around lines 136 - 156, The catch handlers in DevkitsCommand are using Logger.errorNotify(...) for caught exceptions which breaks the project guideline; change those calls to Logger.error(msg, error as Error, "DevkitsCommand") in both the inner catch inside the async callback and the outer catch (including the opts?.silent branch where currently Logger.error(...) is used for silent true but Logger.errorNotify is used for non-silent), ensuring all caught-error logging uses Logger.error with the same scope identifier "DevkitsCommand" and keep notification behavior separate if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/espIdf/openOcd/openOcdManager.ts`:
- Around line 53-75: getOpenOcdPath currently uses lstatSync(...).isFile() which
rejects symlinks; replace that call with statSync(...).isFile() so the function
follows symlinks to real executables, keeping the existing
accessSync(customOpenOcdPath, constants.X_OK) check and try/catch behavior;
update the reference in getOpenOcdPath (replace lstatSync with statSync) and
ensure the rest of the logic (returning customOpenOcdPath or falling back to
isBinInPath) remains unchanged.
In `@src/espIdf/setTarget/DevkitsCommand.ts`:
- Around line 50-60: Remove the guard that throws when openOCDVersion is falsy
and instead call getScriptPath using the resolved OpenOCD binary location
regardless of whether parsing produced a version string; keep openOCDVersion
only for informational logging. Concretely, stop throwing in the openOCDVersion
check, call this.getScriptPath(...) even when openOCDVersion is empty (pass the
binary path the method expects), and only use openOCDVersion for
logs/diagnostics. Apply the same change to the second occurrence that mirrors
this logic (the block referenced at 162-172) so both code paths perform script
discovery based on the binary location and not on the parsed version value.
---
Nitpick comments:
In `@src/espIdf/setTarget/DevkitsCommand.ts`:
- Around line 136-156: The catch handlers in DevkitsCommand are using
Logger.errorNotify(...) for caught exceptions which breaks the project
guideline; change those calls to Logger.error(msg, error as Error,
"DevkitsCommand") in both the inner catch inside the async callback and the
outer catch (including the opts?.silent branch where currently Logger.error(...)
is used for silent true but Logger.errorNotify is used for non-silent), ensuring
all caught-error logging uses Logger.error with the same scope identifier
"DevkitsCommand" and keep notification behavior separate if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7bd02a5-8684-43a5-9c35-1ba7da06d30b
📒 Files selected for processing (5)
src/espIdf/openOcd/openOcdManager.tssrc/espIdf/setTarget/DevkitsCommand.tssrc/support/configurationAccess.tssrc/support/configurationSettings.tssrc/support/writeReport.ts
✅ Files skipped from review due to trivial changes (2)
- src/support/configurationSettings.ts
- src/support/writeReport.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/espIdf/setTarget/index.ts (1)
105-116: Avoid resolving OpenOCD twice on the target-selection path.
runDevkitsScript()now owns thegetOpenOcdPath()/getScriptPath()lookup, so this preflight check only adds anotheropenocd --versionprobe and can drift from the actual execution path if the config changes mid-flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/espIdf/setTarget/index.ts` around lines 105 - 116, Remove the redundant preflight OpenOCD probing: do not call OpenOCDManager.init()/version(), OpenOCDManager.getOpenOcdPath(...) or devkitsCmd.getScriptPath(...) before running the script because runDevkitsScript() now performs those lookups itself; instead, simply create the DevkitsCommand (new DevkitsCommand(workspaceFolder.uri)), optionally configure env via configureEnvVariables(...) if needed, then call devkitsCmd.runDevkitsScript(...) directly and use its result. Update any code that relied on the earlier openOCD variables (openOCDVersion, openOcdPath, scriptPath) to use the output/status returned by runDevkitsScript() or let runDevkitsScript handle reporting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/espIdf/setTarget/index.ts`:
- Around line 105-116: Remove the redundant preflight OpenOCD probing: do not
call OpenOCDManager.init()/version(), OpenOCDManager.getOpenOcdPath(...) or
devkitsCmd.getScriptPath(...) before running the script because
runDevkitsScript() now performs those lookups itself; instead, simply create the
DevkitsCommand (new DevkitsCommand(workspaceFolder.uri)), optionally configure
env via configureEnvVariables(...) if needed, then call
devkitsCmd.runDevkitsScript(...) directly and use its result. Update any code
that relied on the earlier openOCD variables (openOCDVersion, openOcdPath,
scriptPath) to use the output/status returned by runDevkitsScript() or let
runDevkitsScript handle reporting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d17cf060-9d17-40e4-81bf-8147b72290c3
📒 Files selected for processing (3)
src/espIdf/openOcd/openOcdManager.tssrc/espIdf/setTarget/DevkitsCommand.tssrc/espIdf/setTarget/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/espIdf/openOcd/openOcdManager.ts
Collaborator
|
@brianignacio5 hi ! Tested under: Custom OpenOCD path works ✅ LGTM 👍 |
AndriiFilippov
approved these changes
May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds support for specifying a custom OpenOCD executable path in the ESP-IDF VS Code extension. Users can now configure the absolute path to their preferred OpenOCD binary via the new
idf.customOpenOCDPathsetting. If this setting is unset or invalid, the extension will fall back to searching for OpenOCD in the systemPATH. The change is thoroughly documented and integrated into the extension's configuration, environment preparation, and reporting features.The most important changes include:
Feature: Custom OpenOCD Path Support
idf.customOpenOCDPathconfiguration option topackage.json, with localization support in all relevantpackage.nls.*.jsonfiles. This option allows users to specify an absolute path to a custom OpenOCD executable. [1] [2] [3] [4] [5] [6]OpenOCDManager.getOpenOcdPath()to use the custom path if set and valid, otherwise fallback to searching inPATH. Updated all usages of OpenOCD path resolution to use this method. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]Documentation Updates
debugproject.rst), and updated the settings reference in both languages. [1] [2] [3] [4]Reporting and Diagnostics
customOpenOcdPathsetting, its access status, and its value in generated reports. [1] [2] [3] [4] [5] [6] [7] [8]These changes improve flexibility for users who need to use a custom or patched version of OpenOCD, and ensure the new option is well-documented and visible in diagnostics.
Fixes #1799
Type of change
Steps to test this pull request
idf.customOpenOcdPathto OpenOCD from a different location from the current ESP-IDF setup.ESP-IDF: OpenOCD Managercommand to start OpenOCD or from the status bar[OpenOCD]button.idf.customOpenOcdPathand theESP-IDF: Doctor Commandshows the custom OpenOCD path and if the file is accessible.The OpenOCD used is from the
idf.customOpenOcdPathotherwise it will use OpenOCD from ESP-IDF setup.The OpenOCD used is from the
idf.customOpenOcdPathotherwise it will use OpenOCD from ESP-IDF setup.How has this been tested?
Manual testing by following steps above.
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation
Localization
Diagnostics