Conversation
📝 WalkthroughWalkthroughDetects OpenOCD version at runtime and selects modern ( Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant OpenOCD as OpenOCD Exec
participant Logger as Activator.log
Config->>OpenOCD: execute "openocd --version"
OpenOCD-->>Config: stdout / stderr or error
alt parsed major/minor >= threshold
Config->>Config: choose "gdb port / telnet port / tcl port" formats
else parse/process failed or older version
Config->>Config: choose "gdb_port / telnet_port / tcl_port" formats
Config->>Logger: log warning (CoreException / Status)
end
Config->>Config: build GDB server command array using chosen formats
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java (1)
100-127:⚠️ Potential issue | 🟠 MajorUse a locale-stable formatter for OpenOCD port command strings.
String.format(...)uses the default JVM formatting locale, so%dcan emit localized digits (e.g., Arabic ٠١٢٣٤٥٦٧٨٩) in some user locales. OpenOCD command parsing fails with non-ASCII digits. AddLocale.ROOTas the first parameter to ensure ASCII output.Fix locations
Lines 113, 120, 127: All three
String.format()calls formatting port numbers.Proposed fix
import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ - lst.add(String.format(fmtGdbPort, port)); + lst.add(String.format(Locale.ROOT, fmtGdbPort, port)); @@ - lst.add(String.format(fmtTelnetPort, port)); + lst.add(String.format(Locale.ROOT, fmtTelnetPort, port)); @@ - lst.add(String.format(fmtTclPort, port)); + lst.add(String.format(Locale.ROOT, fmtTclPort, port));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java` around lines 100 - 127, Replace the locale-sensitive String.format(...) calls used to build OpenOCD port command strings so they always produce ASCII digits: update the three calls that use String.format(fmtGdbPort, port), String.format(fmtTelnetPort, port), and String.format(fmtTclPort, port) to call String.format(Locale.ROOT, fmtGdbPort, port) / String.format(Locale.ROOT, fmtTelnetPort, port) / String.format(Locale.ROOT, fmtTclPort, port) respectively (the code around useModernPortSyntax(...), fmtGdbPort/fmtTelnetPort/fmtTclPort, and the PortChecker.getAvailablePort(...) calls in Configuration.java).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java`:
- Around line 100-127: Replace the locale-sensitive String.format(...) calls
used to build OpenOCD port command strings so they always produce ASCII digits:
update the three calls that use String.format(fmtGdbPort, port),
String.format(fmtTelnetPort, port), and String.format(fmtTclPort, port) to call
String.format(Locale.ROOT, fmtGdbPort, port) / String.format(Locale.ROOT,
fmtTelnetPort, port) / String.format(Locale.ROOT, fmtTclPort, port) respectively
(the code around useModernPortSyntax(...), fmtGdbPort/fmtTelnetPort/fmtTclPort,
and the PortChecker.getAvailablePort(...) calls in Configuration.java).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: faf0f63a-9ff7-4f58-be42-68cf93b035df
📒 Files selected for processing (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java
There was a problem hiding this comment.
@sigmaaa hi !
Tested under:
OS: Windows 11 / Mac
IDE uses correct commands with OpenOCD higher versions = 12 (gdb port) ✔️
IDE uses correct commands with OpenOCD lower versions < 12 (gdb_port) ✔️
the only thing I think we should improve is the fallback behavior.
IDE does fallback if find external OpenOCD been used, so if the OpenOCD version is 12.0 but it is located under C:/myCooLOpenOCD/openocd.exe then IDE considers this version as <12 . I think we should think about it, since this might cause issues in future, once OpenOCD team stops supporting old command syntax.
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
LGTM. Worth addressing the @AndriiFilippov point. I usually download github OpenOCD from here https://github.com/espressif/openocd-esp32/releases and verify for any fixes which are not yet available as part of esp-idf release, in that case, pattern doesn't match and fallback to old style which is incorrect I think.
|
Hi @kolipakakondal, @AndriiFilippov Thank you for the review. Changed the approach in the last commit, PTAL |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java (2)
105-108: Consider caching the version-detection result per executable path.
useModernPortSyntax(executable)spawns the OpenOCD process and parses--versionoutput every timegetGdbServerCommandLineArrayis invoked. The answer is deterministic for a given executable path and can be memoized in aConcurrentHashMap<String, Boolean>to avoid repeated subprocess startup latency on the debug-launch path.♻️ Suggested cache
+ private static final java.util.concurrent.ConcurrentMap<String, Boolean> MODERN_SYNTAX_CACHE = + new java.util.concurrent.ConcurrentHashMap<>(); + private static boolean useModernPortSyntax(String executablePath) { if (executablePath == null || executablePath.isEmpty()) { return false; } + Boolean cached = MODERN_SYNTAX_CACHE.get(executablePath); + if (cached != null) + { + return cached; + } + boolean result = detectModernPortSyntax(executablePath); + MODERN_SYNTAX_CACHE.put(executablePath, result); + return result; + }(Then move the existing body into
detectModernPortSyntax.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java` around lines 105 - 108, The call to useModernPortSyntax(executable) in getGdbServerCommandLineArray repeatedly spawns OpenOCD to parse --version; cache the result per executable path to avoid repeated subprocess costs by introducing a ConcurrentHashMap<String, Boolean> (e.g., modernSyntaxCache) keyed by the executable path, refactor the version-checking logic into a detectModernPortSyntax method that performs the process spawn and parsing, and change useModernPortSyntax to consult/compute the cache (computeIfAbsent using detectModernPortSyntax) so subsequent calls for the same executable return the memoized Boolean.
380-388: UseActivator.log(IStatus)directly instead of wrappingStatusin a syntheticCoreException.The
Activatorclass exposeslog(IStatus)as a direct overload. Wrapping aStatusinCoreExceptionbefore passing it tolog(Throwable)is unnecessary and obscures intent—the severity level (WARNING) is lost in favor of the ERROR status used bylog(Throwable). Pass the Status directly:Activator.log(new Status(IStatus.WARNING, Activator.PLUGIN_ID, "...", e))for clearer logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java` around lines 380 - 388, Replace the current calls that wrap a Status in a CoreException before logging (e.g. Activator.log(new CoreException(new Status(IStatus.WARNING, Activator.PLUGIN_ID, "...", e)))) with the direct Status overload: call Activator.log(new Status(IStatus.WARNING, Activator.PLUGIN_ID, "...", e)) in the same catch blocks (see the InterruptedException catch and the preceding catch that handles execution/parse fallback) so the original WARNING severity and exception are preserved in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java`:
- Around line 347-374: The version-probe can hang because reader.lines() may
block while the process never exits; modify the code around the Process creation
in Configuration.java so you wait for the process to exit with a timeout before
reading its output: call process.waitFor(<timeout>, TimeUnit.<UNIT>) (e.g. a few
seconds) and only call process.inputReader()/reader.lines() if waitFor returned
true (process terminated); if waitFor times out, destroy/destroyForcibly the
process and return false or an error path instead of attempting to read; keep
the existing cleanup in the finally block but ensure the pre-read wait prevents
a blocking read.
---
Nitpick comments:
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java`:
- Around line 105-108: The call to useModernPortSyntax(executable) in
getGdbServerCommandLineArray repeatedly spawns OpenOCD to parse --version; cache
the result per executable path to avoid repeated subprocess costs by introducing
a ConcurrentHashMap<String, Boolean> (e.g., modernSyntaxCache) keyed by the
executable path, refactor the version-checking logic into a
detectModernPortSyntax method that performs the process spawn and parsing, and
change useModernPortSyntax to consult/compute the cache (computeIfAbsent using
detectModernPortSyntax) so subsequent calls for the same executable return the
memoized Boolean.
- Around line 380-388: Replace the current calls that wrap a Status in a
CoreException before logging (e.g. Activator.log(new CoreException(new
Status(IStatus.WARNING, Activator.PLUGIN_ID, "...", e)))) with the direct Status
overload: call Activator.log(new Status(IStatus.WARNING, Activator.PLUGIN_ID,
"...", e)) in the same catch blocks (see the InterruptedException catch and the
preceding catch that handles execution/parse fallback) so the original WARNING
severity and exception are preserved in logs.
🪄 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: d022f7ba-3439-4657-8b93-8e0c467dd3f3
📒 Files selected for processing (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java (1)
105-109: Cache syntax detection per executable path to reduce launch overhead.
useModernPortSyntaxstarts a subprocess during command-line assembly. Caching the result avoids repeated probes and repeated timeout delays.♻️ Suggested refactor
+import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; @@ public class Configuration { + private static final Map<String, Boolean> MODERN_PORT_SYNTAX_CACHE = new ConcurrentHashMap<>(); @@ - boolean useModernSyntax = useModernPortSyntax(executable); + boolean useModernSyntax = MODERN_PORT_SYNTAX_CACHE.computeIfAbsent(executable, + Configuration::useModernPortSyntax);Also applies to: 339-389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java` around lines 105 - 109, The code calls useModernPortSyntax(executable) repeatedly while assembling commands; cache its Boolean result per executable path (e.g., in a private static ConcurrentHashMap<String,Boolean> or instance Map) and consult the cache before invoking useModernPortSyntax to avoid repeated subprocess probes; update Configuration where fmtGdbPort/fmtTelnetPort/fmtTclPort are chosen (and the similar code at the other location referenced) to first lookup the executable path in the cache, compute and store the Boolean on a miss, then use that cached value to pick the format strings; ensure the cache key is the executable absolute path and the cache is thread-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java`:
- Around line 105-109: The code calls useModernPortSyntax(executable) repeatedly
while assembling commands; cache its Boolean result per executable path (e.g.,
in a private static ConcurrentHashMap<String,Boolean> or instance Map) and
consult the cache before invoking useModernPortSyntax to avoid repeated
subprocess probes; update Configuration where
fmtGdbPort/fmtTelnetPort/fmtTclPort are chosen (and the similar code at the
other location referenced) to first lookup the executable path in the cache,
compute and store the Boolean on a miss, then use that cached value to pick the
format strings; ensure the cache key is the executable absolute path and the
cache is thread-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1dffab0-2da8-4ce3-8f4c-39de30ae0970
📒 Files selected for processing (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java
kolipakakondal
left a comment
kolipakakondal
left a comment
There was a problem hiding this comment.
Thanks for the changes. LGTM.
3 participants
Description
Parse the OpenOCD version from the executable path to dynamically determine whether to use the old (gdb_port) or new (gdb port) syntax.
Fixes # (IEP-1534)
Type of change
Please delete options that are not relevant.
How has this been tested?
Debug with different esp-idf versions and see if there are no "deprecated" message in logs
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Bug Fixes
Improvements