Conversation
📝 WalkthroughWalkthroughAdds “no board selected” validation to the OpenOCD debug and serial JTAG flash launch paths. Board configuration resolution is centralized in ChangesBoard-not-selected guard across debug and flash launch paths
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LaunchConfigurationDelegate
participant JtagVariableResolver
participant BoardNotSelectedStatusHandler
participant ILaunchTargetUIManager
User->>LaunchConfigurationDelegate: start debug launch
LaunchConfigurationDelegate->>JtagVariableResolver: isBoardConfigResolvable()
JtagVariableResolver-->>LaunchConfigurationDelegate: board configuration unavailable
LaunchConfigurationDelegate->>BoardNotSelectedStatusHandler: handleStatus(6001)
BoardNotSelectedStatusHandler->>User: show board-not-selected dialog
User-->>BoardNotSelectedStatusHandler: confirm
BoardNotSelectedStatusHandler->>ILaunchTargetUIManager: editLaunchTarget(active target)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Collaborator
Author
|
After further thought, I went with failing early via a pop-up instead. It avoids waiting for and parsing every OpenOCD message, and it's more accurate: invalid command name "program_esp_bins" can stem from a corrupted configuration file too, not just an unselected board. 
Also added the same check for jtag flashing |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java`:
- Around line 618-631: Extract the board-selection validation currently embedded
in checkBinaryDetails() into a reusable check, then invoke it whenever the
server is started, before branching on attach versus non-attach launches. Ensure
attach launches with local OpenOCD enabled also call isBoardConfigured(config)
and report the existing BOARD_NOT_SELECTED_STATUS_CODE status before starting
the server, while preserving the current behavior for other launch paths.
- Around line 652-657: Normalize path separators in the resolved manual
board-configuration options before checking for the “board/” marker. Apply this
change in LaunchConfigurationDelegate around Configuration.resolveAll and in
SerialFlashLaunchConfigDelegate around the resolved value, preserving the
existing detection behavior while accepting Windows backslash paths.
🪄 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 Plus
Run ID: 9d6ed08a-81db-4edc-983e-4f0849f42d14
📒 Files selected for processing (9)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/JtagVariableResolver.javabundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xmlbundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/BoardNotSelectedStatusHandler.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/Messages.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/messages.propertiesbundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/Messages.javabundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.javabundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/messages.properties
Comment on lines
+618
to
+631
| // Abort early with a clear, actionable message when no board is selected for the active target. Otherwise | ||
| // OpenOCD would start without a board configuration and fail later with the cryptic | ||
| // "invalid command name \"program_esp_bins\"" error. | ||
| if (!isBoardConfigured(config)) | ||
| { | ||
| IStatus status = new Status(IStatus.OK, Activator.PLUGIN_ID, BOARD_NOT_SELECTED_STATUS_CODE, | ||
| "No board is selected for the debug target.", null); //$NON-NLS-1$ | ||
| IStatusHandler handler = DebugPlugin.getDefault().getStatusHandler(status); | ||
| if (handler != null) | ||
| { | ||
| handler.handleStatus(status, null); | ||
| } | ||
| throw new DebugException(Status.OK_STATUS); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate board selection for attach launches too.
checkBinaryDetails() is called only under if (!attach) (Lines 339-340). An attach launch with “Start OpenOCD locally” enabled therefore bypasses this guard and can still start OpenOCD without a board configuration. Extract this check and run it whenever the server is started, before the attach branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java`
around lines 618 - 631, Extract the board-selection validation currently
embedded in checkBinaryDetails() into a reusable check, then invoke it whenever
the server is started, before branching on attach versus non-attach launches.
Ensure attach launches with local OpenOCD enabled also call
isBoardConfigured(config) and report the existing BOARD_NOT_SELECTED_STATUS_CODE
status before starting the server, while preserving the current behavior for
other launch paths.
Comment on lines
+652
to
+657
| // Fall back to inspecting the resolved Config options for a manually configured board file. | ||
| try | ||
| { | ||
| String resolvedOptions = Configuration.resolveAll(Configuration.getGdbServerOtherConfig(config), config); | ||
| return resolvedOptions != null && resolvedOptions.contains("board/"); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept Windows paths in manual board-config detection.
Both fallbacks require the literal board/. A valid manually configured Windows path such as ...\board\esp32.cfg is treated as unconfigured and blocked. Normalize separators before matching.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java#L652-L657: normalizeresolvedOptionsbefore checking forboard/.bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java#L297-L305: normalizeresolvedbefore checking forboard/.
Proposed fix
- return resolvedOptions != null && resolvedOptions.contains("board/");
+ return resolvedOptions != null
+ && resolvedOptions.replace('\\', '/').contains("board/");- return resolved != null && resolved.contains("board/");
+ return resolved != null && resolved.replace('\\', '/').contains("board/");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fall back to inspecting the resolved Config options for a manually configured board file. | |
| try | |
| { | |
| String resolvedOptions = Configuration.resolveAll(Configuration.getGdbServerOtherConfig(config), config); | |
| return resolvedOptions != null && resolvedOptions.contains("board/"); //$NON-NLS-1$ | |
| } | |
| // Fall back to inspecting the resolved Config options for a manually configured board file. | |
| try | |
| { | |
| String resolvedOptions = Configuration.resolveAll(Configuration.getGdbServerOtherConfig(config), config); | |
| return resolvedOptions != null | |
| && resolvedOptions.replace('\\', '/').contains("board/"); //$NON-NLS-1$ | |
| } |
📍 Affects 2 files
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java#L652-L657(this comment)bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/internal/SerialFlashLaunchConfigDelegate.java#L297-L305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java`
around lines 652 - 657, Normalize path separators in the resolved manual
board-configuration options before checking for the “board/” marker. Apply this
change in LaunchConfigurationDelegate around Configuration.resolveAll and in
SerialFlashLaunchConfigDelegate around the resolved value, preserving the
existing detection behavior while accepting Windows backslash paths.
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
1 participant
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
Replacedinvalid command name "program_esp_bins"with userfriendly clickable message that allows edit the target and select the board:Note: After further thought, I went with failing early via a pop-up instead. It avoids waiting for and parsing every OpenOCD message, and it's more accurate: invalid command name "program_esp_bins" can stem from a corrupted configuration file too, not just an unselected board.
Also added the same check for jtag flashing
Fixes # (IEP-1772)
Type of change
Please delete options that are not relevant.
How has this been tested?
Connect the esp chip, select a port, but do not select a board and start debug session
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Summary by CodeRabbit