refactor OpenOCD Call CMD#416
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRecomputes OpenOCD executable/args for ESP32 upload, tightens uploader quoting, expands debug-tool detection and svd_path auto-fill, reads application_offset from build metadata with fallback, and adds/updates debug.openocd_board entries across many ESP32 board manifests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.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 the current code and only fix it if needed.
Inline comments:
In `@builder/main.py`:
- Around line 1863-1874: The code builds openocd_executable from openocd_pkg_dir
which becomes "" when platform.get_package_dir("tool-openocd-esp32") is missing,
producing a relative "bin/openocd" and preventing PATH lookup; change the logic
around openocd_pkg_dir/openocd_executable so that if openocd_pkg_dir is falsy
(after _to_unix_slashes/get_package_dir) you set openocd_executable to the
literal "openocd" to allow PATH resolution, otherwise set it to
str(Path(openocd_pkg_dir) / "bin" / "openocd"); keep the UPLOADER/UPLOADERFLAGS
env.Replace usage but reference the updated openocd_executable and ensure any
$PACKAGE_DIR replacement in openocd_args still works when openocd_pkg_dir is
empty.
In `@platform.py`:
- Around line 835-839: The current code writes svd_path into an initially empty
debug dict (debug["svd_path"] = ...), which makes debug truthy and causes the
method to advertise debug capabilities and later produce invalid fallback args
in _get_debug_server_args(); change the assignment to only set svd_path when
debug already contains other data (e.g., if debug and not debug.get("svd_path"):
...), or otherwise check for meaningful debug keys (like
openocd_board/openocd_target) before mutating debug in the method that calls
get_dir()/svd lookup so empty debug blocks remain falsey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff45581a-1e47-487d-814e-795b8cea0bdc
📒 Files selected for processing (2)
builder/main.pyplatform.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform.py (1)
929-939:⚠️ Potential issue | 🟡 MinorAvoid a misleading fallback warning for custom load flows.
Lines 935-939 warn on any early return, but one of those conditions is
debug_config.load_cmds != ["load"]. Boards likearduino_nano_esp32intentionally overrideload_cmds, so this now logs “flash_images metadata missing or incomplete” even when the metadata is fine. Split the custom-load case from the missing/incomplete-metadata cases and only warn for the latter.💡 Suggested adjustment
- ignore_conds = [ - debug_config.load_cmds != ["load"], - not flash_images, - not all([Path(item["path"]).is_file() for item in flash_images]), - ] - - if any(ignore_conds): - logger.warning( - "Falling back to default GDB load; " - "flash_images metadata missing or incomplete." - ) - return + if debug_config.load_cmds != ["load"]: + return + + if not flash_images or not all( + Path(item["path"]).is_file() for item in flash_images + ): + logger.warning( + "Falling back to default GDB load; " + "flash_images metadata missing or incomplete." + ) + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform.py` around lines 929 - 939, The warning is emitted whenever any condition in ignore_conds is true, so boards that intentionally set a custom load flow (debug_config.load_cmds != ["load"]) trigger a misleading "flash_images metadata missing" warning; change the logic to treat the custom-load case separately: first check if debug_config.load_cmds != ["load"] and if so skip the warning/early-return for custom flows, otherwise evaluate the metadata-related conditions (not flash_images or not all(Path(item["path"]).is_file() for item in flash_images)) and only call logger.warning when one of those metadata conditions is true. Keep references to ignore_conds, debug_config.load_cmds, flash_images and the existing logger.warning call so behavior and messages remain consistent.
♻️ Duplicate comments (1)
builder/main.py (1)
1863-1873:⚠️ Potential issue | 🟠 MajorPATH fallback still leaves OpenOCD with a broken script search path.
Lines 1866-1868 still replace
$PACKAGE_DIReven whenplatform.get_package_dir("tool-openocd-esp32")is missing. That turns"$PACKAGE_DIR/share/openocd/scripts"into/share/openocd/scripts, so the new"openocd"fallback at Lines 1870-1873 still cannot resolve the interface/board cfgs. Only substitute$PACKAGE_DIRwhen the package dir exists; otherwise drop that-s .../share/openocd/scriptspair and let the PATH-provided OpenOCD use its own default scripts directory.🛠️ Suggested fix
- openocd_args = [ - f.replace("$PACKAGE_DIR", openocd_pkg_dir) - for f in openocd_args - ] - if openocd_pkg_dir: - openocd_executable = str(Path(openocd_pkg_dir) / "bin" / "openocd") - else: - openocd_executable = "openocd" + if openocd_pkg_dir: + openocd_args = [ + f.replace("$PACKAGE_DIR", openocd_pkg_dir) + for f in openocd_args + ] + openocd_executable = str(Path(openocd_pkg_dir) / "bin" / "openocd") + else: + filtered_args = [] + skip_next = False + for index, arg in enumerate(openocd_args): + if skip_next: + skip_next = False + continue + if ( + arg == "-s" + and index + 1 < len(openocd_args) + and "$PACKAGE_DIR" in openocd_args[index + 1] + ): + skip_next = True + continue + filtered_args.append(arg) + openocd_args = filtered_args + openocd_executable = "openocd"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` around lines 1863 - 1873, The current logic always replaces "$PACKAGE_DIR" in openocd_args even when platform.get_package_dir("tool-openocd-esp32") returns empty, producing paths like "/share/openocd/scripts" that break the PATH fallback; change the handling around openocd_pkg_dir/openocd_args so that you only perform the f.replace("$PACKAGE_DIR", openocd_pkg_dir) when openocd_pkg_dir is truthy, and when it is falsy filter out any argument entries that reference "$PACKAGE_DIR" (or the "-s" + "$PACKAGE_DIR/.../scripts" pair) so the fallback openocd_executable = "openocd" can use its own default script search paths; update code locations referencing openocd_pkg_dir, openocd_args and openocd_executable accordingly.
🤖 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 `@platform.py`:
- Around line 929-939: The warning is emitted whenever any condition in
ignore_conds is true, so boards that intentionally set a custom load flow
(debug_config.load_cmds != ["load"]) trigger a misleading "flash_images metadata
missing" warning; change the logic to treat the custom-load case separately:
first check if debug_config.load_cmds != ["load"] and if so skip the
warning/early-return for custom flows, otherwise evaluate the metadata-related
conditions (not flash_images or not all(Path(item["path"]).is_file() for item in
flash_images)) and only call logger.warning when one of those metadata
conditions is true. Keep references to ignore_conds, debug_config.load_cmds,
flash_images and the existing logger.warning call so behavior and messages
remain consistent.
---
Duplicate comments:
In `@builder/main.py`:
- Around line 1863-1873: The current logic always replaces "$PACKAGE_DIR" in
openocd_args even when platform.get_package_dir("tool-openocd-esp32") returns
empty, producing paths like "/share/openocd/scripts" that break the PATH
fallback; change the handling around openocd_pkg_dir/openocd_args so that you
only perform the f.replace("$PACKAGE_DIR", openocd_pkg_dir) when openocd_pkg_dir
is truthy, and when it is falsy filter out any argument entries that reference
"$PACKAGE_DIR" (or the "-s" + "$PACKAGE_DIR/.../scripts" pair) so the fallback
openocd_executable = "openocd" can use its own default script search paths;
update code locations referencing openocd_pkg_dir, openocd_args and
openocd_executable accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 884daafa-b356-4f67-9f7d-10dd230045e8
📒 Files selected for processing (2)
builder/main.pyplatform.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@boards/m5stack-fire.json`:
- Around line 25-27: The debug.openocd_board value is wrong for this
PSRAM-equipped board: because the file contains BOARD_HAS_PSRAM and PSRAM cache
fix flags, update the "openocd_board" entry (the debug.openocd_board key in the
m5stack-fire.json diff) from "esp-wroom-32.cfg" to "esp32-wrover.cfg" to match
other PSRAM boards (e.g., m5stack_paper and m5stack-core2).
In `@builder/main.py`:
- Around line 1873-1886: The loop that filters openocd_args is currently
discarding every "-s <path>" pair; change it so only "-s" entries whose path
contains "$PACKAGE_DIR" (e.g. the generated
"$PACKAGE_DIR/share/openocd/scripts") are removed while preserving other "-s
<path>" pairs; specifically update the logic around the variables openocd_args,
filtered and skip_next to check the next token when encountering "-s" and only
set skip_next/continue when that next token contains "$PACKAGE_DIR" (otherwise
append both "-s" and its path to filtered), then assign openocd_args = filtered
and leave openocd_executable = "openocd" as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d32e48af-6908-4800-97fa-fd82c0bce47f
📒 Files selected for processing (58)
boards/bpi-bit.jsonboards/cnrs_aw2eth.jsonboards/denky32.jsonboards/denky_d4.jsonboards/esp32-pico-devkitm-2.jsonboards/esp32-poe-iso.jsonboards/esp32-poe.jsonboards/esp32-pro.jsonboards/esp320.jsonboards/espea32.jsonboards/etboard.jsonboards/healthypi4.jsonboards/heltec_wifi_kit_32.jsonboards/heltec_wifi_kit_32_v2.jsonboards/heltec_wireless_stick_lite.jsonboards/imbrios-logsens-v1p1.jsonboards/inex_openkb.jsonboards/intorobot.jsonboards/kits-edu.jsonboards/labplus_mpython.jsonboards/lilygo-t-display.jsonboards/m5stack-atom.jsonboards/m5stack-core-esp32-16M.jsonboards/m5stack-core-esp32.jsonboards/m5stack-core2.jsonboards/m5stack-coreink.jsonboards/m5stack-fire.jsonboards/m5stack-grey.jsonboards/m5stack-station.jsonboards/m5stack-timer-cam.jsonboards/m5stack_paper.jsonboards/m5stamp-pico.jsonboards/m5stick-c.jsonboards/magicbit.jsonboards/mgbot-iotik32a.jsonboards/mgbot-iotik32b.jsonboards/microduino-core-esp32.jsonboards/nano32.jsonboards/nina_w10.jsonboards/nscreen-32.jsonboards/odroid_esp32.jsonboards/onehorse32dev.jsonboards/oroca_edubot.jsonboards/piranha_esp32.jsonboards/pycom_gpy.jsonboards/qchip.jsonboards/quantum.jsonboards/roboheart_hercules.jsonboards/s_odi_ultra.jsonboards/sparkfun_esp32micromod.jsonboards/tinypico.jsonboards/ttgo-t-watch.jsonboards/ttgo-t7-v13-mini32.jsonboards/turta_iot_node.jsonboards/widora-air.jsonboards/wifiduino32.jsonbuilder/main.pyplatform.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform.py`:
- Around line 626-627: The current logic calls an OpenOCD installer
unconditionally when _needs_debug_tools() is true; update it so
tool-openocd-esp32 is installed only when the configured debug tool actually
requires OpenOCD. Concretely, check variables.get("debug_tool") and/or
variables.get("upload_protocol") for values that map to OpenOCD (e.g., "openocd"
or whatever upload_protocol you treat as OpenOCD) before invoking the
installation of tool-openocd-esp32, while leaving _needs_debug_tools() as the
broader gate for any debug-tool-related setup.
- Around line 929-930: The guard currently compares debug_config.load_cmds to
the exact list ["load"] which fails when load_cmds is a string (e.g., "preload")
or other shapes; update the check in platform.py to normalize load_cmds (e.g.,
coerce to a list or treat string as single-item list) or test the semantic value
(e.g., check if "load" is in load_cmds when treated as an iterable) before
returning so the flash-image loading code runs when the effective load command
is "load" regardless of whether debug_config.load_cmds is a string or a list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4cc72320-ee8e-42f8-9113-0214bb183df6
📒 Files selected for processing (65)
boards/adafruit_feather_esp32_v2.jsonboards/adafruit_itsybitsy_esp32.jsonboards/adafruit_qtpy_esp32.jsonboards/bpi-bit.jsonboards/cnrs_aw2eth.jsonboards/denky32.jsonboards/denky_d4.jsonboards/esp32-pico-devkitm-2.jsonboards/esp32-poe-iso.jsonboards/esp32-poe.jsonboards/esp32-pro.jsonboards/esp320.jsonboards/esp32cam.jsonboards/espea32.jsonboards/etboard.jsonboards/healthypi4.jsonboards/heltec_wifi_kit_32.jsonboards/heltec_wifi_kit_32_v2.jsonboards/heltec_wireless_stick_lite.jsonboards/imbrios-logsens-v1p1.jsonboards/inex_openkb.jsonboards/intorobot.jsonboards/ioxesp32ps.jsonboards/kb32-ft.jsonboards/kits-edu.jsonboards/labplus_mpython.jsonboards/lilygo-t-display.jsonboards/m5stack-atom.jsonboards/m5stack-core-esp32-16M.jsonboards/m5stack-core-esp32.jsonboards/m5stack-core2.jsonboards/m5stack-coreink.jsonboards/m5stack-fire.jsonboards/m5stack-grey.jsonboards/m5stack-station.jsonboards/m5stack-timer-cam.jsonboards/m5stack_paper.jsonboards/m5stamp-pico.jsonboards/m5stick-c.jsonboards/magicbit.jsonboards/mgbot-iotik32a.jsonboards/mgbot-iotik32b.jsonboards/microduino-core-esp32.jsonboards/nano32.jsonboards/nina_w10.jsonboards/nscreen-32.jsonboards/odroid_esp32.jsonboards/onehorse32dev.jsonboards/oroca_edubot.jsonboards/piranha_esp32.jsonboards/pycom_gpy.jsonboards/qchip.jsonboards/quantum.jsonboards/roboheart_hercules.jsonboards/s_odi_ultra.jsonboards/sparkfun_esp32micromod.jsonboards/tinypico.jsonboards/ttgo-t-beam.jsonboards/ttgo-t-watch.jsonboards/ttgo-t7-v13-mini32.jsonboards/turta_iot_node.jsonboards/widora-air.jsonboards/wifiduino32.jsonbuilder/main.pyplatform.py
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:
Checklist:
Summary by CodeRabbit
Bug Fixes
Improvements
New Features