OpenOCD: Add new GDB commands for FreeRTOS and ROM ELF#417
Closed
Jason2866 wants to merge 11 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds runtime ROM-ELF symbol loading into the ESP32 debug flow: platform probes GDB/OpenOCD capabilities, reads ROM metadata from Changes
Sequence Diagram(s)sequenceDiagram
participant User as Developer/IDE
participant Platform as Espressif32Platform
participant ROM as misc/roms.json
participant OpenOCD as OpenOCD
participant Device as ESP32
participant GDB as GDB
User->>Platform: start debug session / request init_cmds
Platform->>ROM: read ROM entries for MCU family
ROM-->>Platform: return ROM metadata
Platform->>OpenOCD: supply generated init_cmds (reset, probe, conditional ROM checks, GDB commands)
OpenOCD->>Device: probe/read build-date address
Device-->>OpenOCD: return build-date string
OpenOCD->>OpenOCD: evaluate nested conditions, select ROM revision
OpenOCD->>GDB: instruct to load matching ROM-ELF symbols / enable FreeRTOS extension
GDB-->>User: symbols loaded, debug session ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. |
![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
🧹 Nitpick comments (1)
platform.py (1)
926-927: Add explicit encoding for consistency.Other file operations in this file specify
encoding='utf-8'(e.g., lines 262, 481). Adding it here ensures consistent behavior across different system locales.♻️ Suggested fix
- with open(roms_json) as f: + with open(roms_json, encoding="utf-8") as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform.py` around lines 926 - 927, The open call that reads roms_json uses the system default encoding; update the file-reading line that currently does "with open(roms_json) as f:" to explicitly pass encoding='utf-8' so it matches other file operations in this module (references: variable roms_json and the json.load call that assigns roms). This ensures consistent UTF-8 handling across the file.
🤖 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 921-923: The code constructs roms_json incorrectly as
Path(self.get_dir()) / "misc" / "roms.json" which misses the svd directory;
update the path construction to include "svd" (i.e. Path(self.get_dir()) /
"misc" / "svd" / "roms.json") where roms_json is created so roms_json.is_file()
checks the correct file, and optionally add a debug/log message near this check
in the same method to surface a missing roms.json instead of silently returning
an empty list; references: roms_json variable and get_dir() call.
---
Nitpick comments:
In `@platform.py`:
- Around line 926-927: The open call that reads roms_json uses the system
default encoding; update the file-reading line that currently does "with
open(roms_json) as f:" to explicitly pass encoding='utf-8' so it matches other
file operations in this module (references: variable roms_json and the json.load
call that assigns roms). This ensures consistent UTF-8 handling across the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d85d616-62de-4f16-81e7-3803aa7adbe6
📒 Files selected for processing (2)
misc/svd/roms.jsonplatform.py
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Specify UTF-8 encoding when opening roms_json file.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 972-975: The generated GDB command for ROM symbols builds rom_file
and uses add-symbol-file with rom_dir but emits an unquoted absolute path;
update the construction so the add-symbol-file argument is wrapped in double
quotes (e.g., build the string using rom_dir + rom_file but surround it with
"..." ) so GDB treats spaces correctly—apply this change where rom_file is used
with cls._rom_date_condition and the add-symbol-file invocation to mirror the
quoting used elsewhere in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 192d09aa-6e7f-4196-b024-95ceef883b7a
📒 Files selected for processing (2)
misc/roms.jsonplatform.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 `@misc/roms.json`:
- Around line 1-80: Missing ROM metadata for esp32c61 causes
_get_rom_elf_gdb_cmds() to return empty and disables ROM symbol loading; add an
"esp32c61" entry to misc/roms.json with the same shape as other entries (objects
containing rev, build_date_str_addr, and build_date_str). Use the existing
esp32c6/esp32c5 entries as templates (e.g., include a numeric "rev", a hex
"build_date_str_addr", and the readable "build_date_str") so
_get_rom_elf_gdb_cmds() recognizes esp32c61 and loads its ROM symbols during
debug sessions.
In `@platform.py`:
- Around line 871-872: The FreeRTOS GDB Python block is unconditionally appended
via _get_freertos_gdb_cmds() and will break on GDB builds without Python
support; modify the initialization so you first probe the selected GDB for
Python support (run the GDB binary with --batch-silent --ex "python import os"
and check exit status/output) and only extend init_cmds with the Python block
when that probe succeeds. Implement the probe either inside
_get_freertos_gdb_cmds() (returning an empty list when Python is unavailable) or
guard the call site before calling _get_freertos_gdb_cmds(), keeping the
existing _get_rom_elf_gdb_cmds(mcu) behavior unchanged. Ensure you reference the
same GDB executable used for the session and log or handle the probe failure
gracefully so non-Python GDBs continue using init_cmds plus
_get_rom_elf_gdb_cmds(mcu).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1aaf755a-73ca-4641-befa-ed04da9d1f3e
📒 Files selected for processing (2)
misc/roms.jsonplatform.py
Added a check to see if the GDB binary supports Python before extending FreeRTOS GDB commands.
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
platform.py (1)
862-873: Hoist the GDB probe and ROM-command build out of the per-link loop.These values are MCU-level invariants, but the current loop recomputes them for every debug tool entry. That means repeated GDB startups plus repeated
roms.jsonparsing while building one board manifest.♻️ Proposed refactor
- for link in upload_protocols: + has_gdb_python = self._gdb_has_python(mcu) + rom_elf_cmds = self._get_rom_elf_gdb_cmds(mcu) + + for link in upload_protocols: if link in non_debug_protocols or link in debug["tools"]: continue @@ - if self._gdb_has_python(mcu): + if has_gdb_python: init_cmds.extend(self._get_freertos_gdb_cmds()) - init_cmds.extend(self._get_rom_elf_gdb_cmds(mcu)) + init_cmds.extend(rom_elf_cmds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform.py` around lines 862 - 873, The init_cmds construction (calls to _gdb_has_python(mcu), _get_freertos_gdb_cmds(), and _get_rom_elf_gdb_cmds(mcu)) should be hoisted out of the per-link/debug-tool loop and computed once per MCU; compute init_cmds once using the MCU object (mcu) before iterating over link entries and then reuse that list for each debug tool to avoid repeated GDB startups and repeated roms.json parsing. Ensure you still call _gdb_has_python(mcu) to decide adding _get_freertos_gdb_cmds(), build init_cmds once, and extend or copy it into per-link data instead of recomputing inside the loop.
🤖 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 899-921: The _gdb_has_python function fails on Windows because it
only checks for a gdb filename without the .exe extension; update the
file-existence check to try both the plain gdb path and the .exe variant when
IS_WINDOWS is true (use the existing IS_WINDOWS import), e.g. build a list of
candidate paths for gdb (Path(pkg_dir)/"bin"/f"{toolchain_arch}-gdb" and, if
IS_WINDOWS, the same with ".exe"), pick the first candidate that exists and use
that path for the subprocess.run call and subsequent checks, keeping the rest of
the logic unchanged (including returning result.returncode == 0 and handling
exceptions in the same way).
---
Nitpick comments:
In `@platform.py`:
- Around line 862-873: The init_cmds construction (calls to
_gdb_has_python(mcu), _get_freertos_gdb_cmds(), and _get_rom_elf_gdb_cmds(mcu))
should be hoisted out of the per-link/debug-tool loop and computed once per MCU;
compute init_cmds once using the MCU object (mcu) before iterating over link
entries and then reuse that list for each debug tool to avoid repeated GDB
startups and repeated roms.json parsing. Ensure you still call
_gdb_has_python(mcu) to decide adding _get_freertos_gdb_cmds(), build init_cmds
once, and extend or copy it into per-link data instead of recomputing inside the
loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85fc6f55-bea4-47e8-986d-9e32161dcc10
📒 Files selected for processing (2)
misc/roms.jsonplatform.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 855-876: The code is probing for GDB/ROM-ELF support during board
metadata construction in _add_dynamic_options() using _gdb_has_python(mcu) and
_get_rom_elf_gdb_cmds(mcu), which runs before configure_default_packages()
installs those packages; move this logic into a post-install path (e.g.
configure_debug_session()) so the probes are executed after package
installation: remove the has_gdb_python/rom_elf_cmds/init_cmds construction from
_add_dynamic_options(), and instead recompute has_gdb_python via
_gdb_has_python(mcu), rom_elf_cmds via _get_rom_elf_gdb_cmds(mcu), and extend
init_cmds (including _get_freertos_gdb_cmds()) inside configure_debug_session()
where you already have access to openocd_interface (from
_get_openocd_interface(link, board)) and server_args (from
_get_debug_server_args(openocd_interface, debug)), ensuring thread-awareness and
ROM symbols are only added after packages are installed.
- Around line 970-980: The hook name is wrong: GDB will call
hookpost-extended-remote for the command "target extended-remote" used
elsewhere, but the code defines "hookpost-remote" so the ROM ELF loading never
runs; update the cmds list so the hook definition matches the actual command
(change the hook name used in the "define ..." entry), i.e., ensure the define
line uses hookpost-extended-remote (and keep the subsequent calls to
self._build_rom_elf_conditions(entries, mcu, rom_elfs_path, depth=1) unchanged)
so the post-hook executes when target extended-remote runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92ff71fb-6270-4f13-8e89-40d08e411675
📒 Files selected for processing (2)
misc/roms.jsonplatform.py
+ Wrong GDB hook name
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Note Docstrings generation - SUCCESS |
coderabbitai Bot
added a commit
that referenced
this pull request
Mar 7, 2026
Docstrings generation was requested by @Jason2866. * #417 (comment) The following files were modified: * `platform.py`
Jason2866
added a commit
that referenced
this pull request
Mar 7, 2026
* 📝 Add docstrings to `debug_gdb_rom` Docstrings generation was requested by @Jason2866. * #417 (comment) The following files were modified: * `platform.py` * Reformat docstrings for better readability --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Jason2866 <24528715+Jason2866@users.noreply.github.com>
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.
Checklist:
Summary by CodeRabbit
New Features
Chores