feat(esp_detect_config.py): integrate devkits detect script in the target wizard and use in OpenOCD #1226
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java (1)
125-162: UI freezing risk: long-running script executed on UI threadThe implementation still risks UI freezing despite using ProgressMonitorDialog. While it shows a modal dialog,
dialog.run()is synchronous and blocks the UI thread until the external Python process completes.This matches the previous review comment recommending moving to a background Job. Apply that approach instead:
String selectedTargetString = idfTargetCombo.getText(); -Shell shell = display.getActiveShell(); final List<String> boardDisplayNames = new ArrayList<>(); -try -{ - ProgressMonitorDialog dialog = new ProgressMonitorDialog(shell); - dialog.run(true, false, monitor -> { - monitor.beginTask("Finding the Connected Boards...", IProgressMonitor.UNKNOWN); //$NON-NLS-1$ - String json = IDFUtil.runEspDetectConfigScript(); - Logger.log("esp_detect_config.py JSON output: " + json); //$NON-NLS-1$ - if (json != null) - { - try - { - JSONObject root = (JSONObject) new JSONParser().parse(json); - JSONArray boards = (JSONArray) root.get("boards"); //$NON-NLS-1$ - boardDisplayNames.addAll(getBoardDisplayNamesForTarget(selectedTargetString, boards)); - } - catch (Exception ex) - { - Logger.log(ex); - } - } - monitor.done(); - }); -} -catch (Exception ex) -{ - Logger.log(ex); -} -// Update UI on SWT thread -display.asyncExec(() -> { - fBoardCombo.setItems(boardDisplayNames.toArray(new String[0])); - if (!boardDisplayNames.isEmpty()) - { - fBoardCombo.select(0); - } -}); +// Use background job to prevent UI freezing +Job.create("Detect DevKits", monitor -> { + monitor.beginTask("Finding the Connected Boards...", IProgressMonitor.UNKNOWN); //$NON-NLS-1$ + String json = IDFUtil.runEspDetectConfigScript(); + Logger.log("esp_detect_config.py JSON output: " + json); //$NON-NLS-1$ + if (json != null) + { + try + { + JSONObject root = (JSONObject) new JSONParser().parse(json); + JSONArray boards = (JSONArray) root.get("boards"); //$NON-NLS-1$ + boardDisplayNames.addAll(getBoardDisplayNamesForTarget(selectedTargetString, boards)); + } + catch (Exception ex) + { + Logger.log(ex); + } + } + monitor.done(); + // Update UI on SWT thread + display.asyncExec(() -> { + fBoardCombo.setItems(boardDisplayNames.toArray(new String[0])); + if (!boardDisplayNames.isEmpty()) + { + fBoardCombo.select(0); + } + }); + return Status.OK_STATUS; +}).schedule();
🧹 Nitpick comments (1)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java (1)
494-508: Extract JSON keys as constantsThe JSON keys like "target", "name", and "location" are used as string literals. For better maintainability, consider extracting them as constants.
+private static final String JSON_KEY_TARGET = "target"; //$NON-NLS-1$ +private static final String JSON_KEY_NAME = "name"; //$NON-NLS-1$ +private static final String JSON_KEY_LOCATION = "location"; //$NON-NLS-1$ +private static final String BOARD_DISPLAY_FORMAT = "%s [%s]"; //$NON-NLS-1$ private List<String> getBoardDisplayNamesForTarget(String selectedTarget, JSONArray boards) { List<String> boardDisplayNames = new ArrayList<>(); for (Object obj : boards) { JSONObject board = (JSONObject) obj; - if (selectedTarget.equals(board.get("target"))) //$NON-NLS-1$ + if (selectedTarget.equals(board.get(JSON_KEY_TARGET))) { - String name = (String) board.get("name"); //$NON-NLS-1$ - String location = (String) board.get("location"); //$NON-NLS-1$ - boardDisplayNames.add(String.format("%s [%s]", name, location)); //$NON-NLS-1$ + String name = (String) board.get(JSON_KEY_NAME); + String location = (String) board.get(JSON_KEY_LOCATION); + boardDisplayNames.add(String.format(BOARD_DISPLAY_FORMAT, name, location)); } } return boardDisplayNames; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (2)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java (2)
303-305: LGTM - Good UI improvement!Setting minimumWidth and proper grid data improves board combo box visibility, making it easier for users to see the full board name including USB location.
478-488: LGTM - Well-implemented extraction methodThe method correctly extracts the USB location from the board name format and handles edge cases appropriately.
WalkthroughThis update introduces dynamic detection of connected boards for serial flashing by running an external Python script and parsing its output. It adds new constants, environment variable handling, and utility methods to manage USB adapter locations. The launch configuration and UI are enhanced to support and propagate USB location information throughout the launch and board selection workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WizardPage
participant IDFUtil
participant PythonScript
participant UI
User->>WizardPage: Selects IDF target
WizardPage->>IDFUtil: runEspDetectConfigScript()
IDFUtil->>PythonScript: Executes esp_detect_config.py
PythonScript-->>IDFUtil: Returns JSON with board info
IDFUtil-->>WizardPage: JSON output
WizardPage->>UI: Populate board combo with detected boards
sequenceDiagram
participant User
participant Wizard
participant WizardPage
participant LaunchConfigProvider
User->>Wizard: Completes wizard
Wizard->>WizardPage: Get selected board USB location
Wizard->>LaunchConfigProvider: Set OPENOCD_USB_LOCATION in config
LaunchConfigProvider->>LaunchConfigProvider: Save updated config
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
889-897: Use path API consistently for Windows compatibility.Path construction using string concatenation with hard-coded slashes can break on Windows, especially with UNC paths. While some path construction uses
Paths.get(), file creation doesn't.For consistent path handling, consider using
Paths.get()for all path construction:- File scriptsDir = Paths.get(openocdRoot.getPath(), "share", "openocd", "scripts").toFile(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ - File toolsDir = Paths.get(openocdRoot.getPath(), "share", "openocd", "espressif", "tools").toFile(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ - String scriptPath = new File(toolsDir, "esp_detect_config.py").getAbsolutePath(); //$NON-NLS-1$ - String configPath = new File(scriptsDir, "esp-config.json").getAbsolutePath(); //$NON-NLS-1$ + File scriptsDir = Paths.get(openocdRoot.getPath(), "share", "openocd", "scripts").toFile(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ + File toolsDir = Paths.get(openocdRoot.getPath(), "share", "openocd", "espressif", "tools").toFile(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ + String scriptPath = Paths.get(toolsDir.getPath(), "esp_detect_config.py").toString(); //$NON-NLS-1$ + String configPath = Paths.get(scriptsDir.getPath(), "esp-config.json").toString(); //$NON-NLS-1$
888-933:⚠️ Potential issueFix typo in CLI flag and add validation for external resources.
- The CLI flag on line 918 is
--oocdbut it should be--openocd.- The code doesn't validate the existence of the script files before attempting to run them.
Apply this fix:
- if (StringUtil.isEmpty(openocdBinDir)) { - Logger.log("OpenOCD location could not be determined."); //$NON-NLS-1$ - return null; - } + if (StringUtil.isEmpty(openocdBinDir)) { + Logger.log("OpenOCD location could not be determined."); //$NON-NLS-1$ + return null; + } // Derive the scripts and tools directories from the bin directory File binDir = new File(openocdBinDir); File openocdRoot = binDir.getParentFile(); File scriptsDir = Paths.get(openocdRoot.getPath(), "share", "openocd", "scripts").toFile(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ File toolsDir = Paths.get(openocdRoot.getPath(), "share", "openocd", "espressif", "tools").toFile(); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ - String scriptPath = new File(toolsDir, "esp_detect_config.py").getAbsolutePath(); //$NON-NLS-1$ - String configPath = new File(scriptsDir, "esp-config.json").getAbsolutePath(); //$NON-NLS-1$ + File detectScript = new File(toolsDir, "esp_detect_config.py"); //$NON-NLS-1$ + if (!detectScript.exists()) { + Logger.log("esp_detect_config.py not found at " + detectScript); //$NON-NLS-1$ + return null; + } + String scriptPath = detectScript.getAbsolutePath(); + + File configFile = new File(scriptsDir, "esp-config.json"); //$NON-NLS-1$ + if (!configFile.exists()) { + Logger.log("esp-config.json not found at " + configFile); //$NON-NLS-1$ + return null; + } + String configPath = configFile.getAbsolutePath(); String openocdExecutable = Platform.getOS().equals(Platform.OS_WIN32) ? "openocd.exe" : "openocd"; //$NON-NLS-1$ //$NON-NLS-2$ File openocdBin = new File(openocdBinDir, openocdExecutable); if (!openocdBin.exists()) { Logger.log("OpenOCD binary not found at expected location."); //$NON-NLS-1$ return null; } String idfPythonEnvPath = IDFUtil.getIDFPythonEnvPath(); if (StringUtil.isEmpty(idfPythonEnvPath)) { Logger.log("IDF_PYTHON_ENV_PATH could not be found."); //$NON-NLS-1$ return null; } List<String> command = new ArrayList<>(); command.add(idfPythonEnvPath); command.add(scriptPath); command.add("--esp-config"); //$NON-NLS-1$ command.add(configPath); - command.add("--oocd");//$NON-NLS-1$ + command.add("--openocd");//$NON-NLS-1$ command.add(openocdBin.getAbsolutePath());bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java (1)
130-166: 👍 UI freeze issue resolvedSwitching from a direct call to
IDFUtil.runEspDetectConfigScript()on the SWT thread toProgressMonitorDialog.run(true, …)off-loads the long-running detection to a worker thread, keeping the UI responsive. Thanks for addressing the earlier feedback!
🧹 Nitpick comments (2)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbServerBackend.java (1)
214-226: Guard against empty USB-location values to avoid polluting the child env
OPENOCD_USB_LOCATION=(empty value) shadows any user/global setting and may confuse OpenOCD.
Only inject the variable when the launch attribute is both non-null and non-blank:- if (openocdLoc != null) { - envMap.put(IDFLaunchConstants.OPENOCD_USB_LOCATION, openocdLoc); - } + if (openocdLoc != null && !openocdLoc.isBlank()) { + envMap.put(IDFLaunchConstants.OPENOCD_USB_LOCATION, openocdLoc); + }Minor, but avoids edge-case surprises.
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java (1)
484-494: Prefer an empty string over null for USB location
getSelectedBoardUsbLocation()returnsnullwhen the combo text does not contain brackets.
Downstream callers have to add an extra null-check or risk an NPE. Returning an empty string communicates “no location selected” while avoiding null semantics.- return null; + return StringUtil.EMPTY;(Same package already depends on
StringUtil.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.core/src/com/espressif/idf/core/variable/JtagVariableResolver.java(1 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbServerBackend.java(2 hunks)bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java(2 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizard.java(1 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
IDFLaunchConstants(3-22)
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizard.java (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
IDFLaunchConstants(3-22)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
🔇 Additional comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFLaunchConstants.java (1)
21-21: New constant for OpenOCD USB adapter location.The new constant defines the key for the OpenOCD USB adapter location environment variable that will be propagated throughout the launch configuration workflow.
bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java (2)
41-47: Propagates USB location from launch target to configuration.This code properly handles setting the OPENOCD_USB_LOCATION attribute from the launch target to the launch configuration, ensuring it's available during the debugging session.
95-95: Code formatting improvement.Minor readability improvement changing a multi-line if statement to single-line.
bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizard.java (1)
69-78: Sets USB adapter location from detected board.This code correctly processes the USB location from the selected board, strips the "usb://" prefix if present, and sets it on the launch target working copy so it can be propagated to the OpenOCD debugging environment.
| int idx = board.lastIndexOf(" [usb://"); //$NON-NLS-1$ | ||
| String boardKey = (idx != -1) ? board.substring(0, idx) : board; | ||
| var boardConfigs = boardConfigMap.get(boardKey); | ||
| var result = new StringBuilder(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (1)
943-944:⚠️ Potential issueTypo in CLI flag parameter
The CLI flag is specified as "--oocd" when it should be "--openocd". This typo was also identified in a previous review.
- command.add("--oocd");//$NON-NLS-1$ + command.add("--openocd");//$NON-NLS-1$
🧹 Nitpick comments (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (4)
887-897: Inconsistent path construction in the new methodThe method uses a mix of
Paths.get().toFile()andFileconstructor approaches for path construction. While the use ofPaths.get()is good for OS compatibility, it should be used consistently throughout the method for better maintainability.- File scriptFile = new File(toolsDir, "esp_detect_config.py"); //$NON-NLS-1$ + File scriptFile = Paths.get(toolsDir.getPath(), "esp_detect_config.py").toFile(); //$NON-NLS-1$
919-922: Redundant existence check and inconsistent error reportingThe method calls
espDetectConfigScriptExists()but also reconstructs the path to create an error message. This is redundant and could lead to inconsistencies if the path construction logic changes in either method.- if (!espDetectConfigScriptExists()) { - Logger.log("esp_detect_config.py not found at expected location: " + new File(toolsDir, "esp_detect_config.py").getAbsolutePath()); //$NON-NLS-1$ //$NON-NLS-2$ + File scriptFile = Paths.get(toolsDir.getPath(), "esp_detect_config.py").toFile(); //$NON-NLS-1$ + if (!scriptFile.exists()) { + Logger.log("esp_detect_config.py not found at expected location: " + scriptFile.getAbsolutePath()); //$NON-NLS-1$ return null; } - String scriptPath = new File(toolsDir, "esp_detect_config.py").getAbsolutePath(); //$NON-NLS-1$ + String scriptPath = scriptFile.getAbsolutePath();
914-918: Missing validation for intermediate directoriesWhile the code checks if the config file exists, it doesn't validate if intermediate directories exist. If any of these directories are missing, the user would only see an error about the final file being missing, which might be confusing.
Consider adding validation for the existence of intermediate directories:
+ if (!scriptsDir.exists()) { + Logger.log("OpenOCD scripts directory not found at expected location: " + scriptsDir.getAbsolutePath()); //$NON-NLS-1$ + return null; + } + if (!toolsDir.exists()) { + Logger.log("OpenOCD Espressif tools directory not found at expected location: " + toolsDir.getAbsolutePath()); //$NON-NLS-1$ + return null; + } File configFile = new File(scriptsDir, "esp-config.json"); //$NON-NLS-1$
947-957: Missing validation of script output formatThe method doesn't validate if the output from the script is a valid JSON string before returning it. This could lead to parsing errors downstream if the script output is unexpected.
Consider adding basic validation of the output format:
try { IStatus status = new ProcessBuilderFactory().runInBackground(command, null, env); if (status == null) { Logger.log("esp_detect_config.py did not return a result."); //$NON-NLS-1$ return null; } - return status.getMessage(); + String output = status.getMessage(); + // Basic validation that output looks like JSON + if (output == null || !(output.trim().startsWith("{") && output.trim().endsWith("}"))) { + Logger.log("esp_detect_config.py did not return valid JSON: " + output); //$NON-NLS-1$ + return null; + } + return output; } catch (Exception e) { Logger.log(e); return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java(1 hunks)bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java(4 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizard.java(3 hunks)bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java
- bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizard.java
- bundles/com.espressif.idf.launch.serial.ui/src/com/espressif/idf/launch/serial/ui/internal/NewSerialFlashTargetWizardPage.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/IDFUtil.java (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/StringUtil.java (1)
StringUtil(11-25)bundles/com.espressif.idf.core/src/com/espressif/idf/core/logging/Logger.java (1)
Logger(21-137)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build_macos
- GitHub Check: build
|
@kolipakakondal hi ! Tested under: could you please share with me the esp_detect_config.py docs, for now it is not clear for me, how this command should work. for now: Speed Functionality
|
|
Hi @AndriiFilippov This MR could help you to understand on this Here are some issues I've reported on this |
Yes, the board selection is for debugging purposes only and will be used during debugging. Based on the board selected, we will set the environment variable for OpenOCD (OPENOCD_USB_ADAPTER_LOCATION=usb location). The serial port is used for flashing and monitoring.
I found this issue. during the development and reported it here and it was fixed with this MR.
Which is incorrect, again I have reported in this, please try with the MR build |
If it’s taking too long to find the boards, we’ll display the progress bar as shown in the PR description. Could you please check again? That said, I didn’t notice any speed issues with Alexey’s new MR — it was quick in my testing.
|
...launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java
Outdated
Show resolved
Hide resolved
If there truly are cases where opening the launch target editor takes too long, perhaps we should consider making this feature optional. One idea could be to add an "Auto-detect" button directly in the ESP target wizard near the board selection. This button could run the detection script and automatically select the appropriate board. |
Not the case anymore! With the latest PR from Alexey, I don’t see any lag, so we’re good. Even if it’s there, we’ve handled it gracefully with the progress bar. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbServerBackend.java (1)
208-241: Good implementation with minor style improvement needed.The method correctly implements environment variable injection for OpenOCD USB location. The logic properly handles null checks and environment variable parsing.
Minor style inconsistency on line 227 - the opening brace placement doesn't match the project's style:
- if (activeLaunchTarget != null) - { + if (activeLaunchTarget != null) {The environment variable parsing logic on line 221 correctly skips malformed entries by checking
idx > 0, which is appropriate for safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbServerBackend.java(2 hunks)bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- bundles/com.espressif.idf.launch.serial.core/src/com/espressif/idf/launch/serial/core/IDFCoreLaunchConfigProvider.java
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build_macos
🔇 Additional comments (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbServerBackend.java (1)
17-21: LGTM: Import additions support the new functionality.The imported classes are appropriately used in the new
launchGdbServerProcessmethod for file handling, environment variable management, and launch target integration.Also applies to: 25-25, 31-31, 33-34, 38-38
|
Thanks for the review @sigmaaa. I’ve addressed the PR comments. PTAL |
|
Hi @AndriiFilippov, Are there any other open issues with this? If not, can we consider it for the v3.5.0 release? The new changes from Alexey might be included in the upcoming releases of ESP-IDF and OpenOCD. However, what’s more important for us here is that, even if the script isn’t available, the workflow should continue to function without any issues. |
Description
Integrates the esp_detect_config.py script into the Target Wizard and IDE workflow. When launching OpenOCD from the IDE, the script automatically detects connected Espressif DevKits and sets the correct environment variable OPENOCD_USB_ADAPTER_LOCATION while starting the OpenOCD during the debugging.
There are few issues around the esp_detect_config.py script, reported here https://jira.espressif.com:8443/browse/OCD-1188
Fixes # (IEP-1423)
Type of change
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit