Skip to content

feature: CLI offline use exception decoder#420

Merged
Jason2866 merged 11 commits into
developfrom
cli_decoder
Mar 8, 2026
Merged

feature: CLI offline use exception decoder#420
Jason2866 merged 11 commits into
developfrom
cli_decoder

Conversation

@Jason2866
Copy link
Copy Markdown

@Jason2866 Jason2866 commented Mar 8, 2026

Checklist:

  • The pull request is done against the latest develop branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR, more changes are allowed when changing boards.json
  • I accept the CLA

Summary by CodeRabbit

  • New Features

    • Standalone offline CLI for decoding ESP32 crash logs with incremental processing and optional file or stdout output
    • Automatic architecture-aware toolchain discovery for accurate decoding across supported ESP chips
    • Improved multi-chip detection and more robust stack unwinding across RISC-V and Xtensa targets
  • Documentation

    • New comprehensive standalone guide: installation, usage examples, workflow, troubleshooting, advanced options, and license info

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c729f5e1-9b8b-4cf9-ac78-0aacb862df0e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a standalone CLI and offline mode to the ESP32 Exception Decoder: auto-detects ELF architecture (RISC‑V or Xtensa), discovers addr2line/gdb toolchains (PATH or PlatformIO packages or ~/.platformio/packages), provides minimal PlatformIO shims, and decodes crash logs to stdout or file. Documentation file added.

Changes

Cohort / File(s) Summary
Documentation
monitor/README_STANDALONE.md
New standalone decoder documentation covering offline usage, architecture and toolchain auto-detection, installation, examples, workflow, troubleshooting, advanced features, supported chips, and license.
Standalone CLI & Decoder Logic
monitor/filter_exception_decoder.py
Adds standalone CLI entry and offline decoding workflow; introduces _find_toolchain_in_path() method on Esp32ExceptionDecoder, module-level _find_toolchain_binaries() and _run_standalone_decoder() functions; guards PlatformIO imports and provides minimal shims; enhances chip detection (considers board_mcu); auto-detects architecture to resolve addr2line/gdb (RISC‑V vs Xtensa); incremental crash-log processing and output-to-file/stdout handling.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant CLI as CLI Entry
    participant ELF as ELF Inspector
    participant Finder as Toolchain Finder
    participant Decoder as Exception Decoder
    participant Output as Output Writer

    User->>CLI: run with --elf and --log (opt --out)
    CLI->>ELF: inspect ELF to detect architecture
    ELF-->>CLI: return architecture (RISC‑V / Xtensa)
    CLI->>Finder: locate addr2line / gdb (PATH or ~/.platformio/packages)
    Finder-->>CLI: return toolchain binary paths
    CLI->>Decoder: init with ELF + toolchain paths
    Decoder->>Decoder: parse crash log and resolve addresses
    Decoder-->>Output: decoded stack traces / symbols
    Output->>User: write to stdout or output file
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I found the ELF beneath a log so gray,

I sniffed the arch and chased each stray address,
Addr2line and GDB lit up the way,
I stitched the stacks and sorted every mess,
🥕✨ A hopping rabbit with a debugging dress.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feature: CLI offline use exception decoder' clearly summarizes the main change: adding standalone/offline CLI functionality to the exception decoder for use without PlatformIO dependencies.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cli_decoder

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 1482-1484: The code incorrectly uses rx() to pass the entire crash
log as one chunk; switch to incremental decoding: open the file and read it in
reasonably sized chunks or line-by-line (ensuring chunks < _RX_BUF_MAX), call
the decoder's incremental feed method for each chunk (instead of rx()), and
after the file is fully read explicitly flush/close EOF state so final lines and
RISC-V backtraces trigger (_feed_riscv_line() must see EOF to call
_invoke_gdb_backtrace()); apply the same change to the other occurrence around
the 1561-1564 region. Ensure you reference and preserve use of _RX_BUF_MAX,
rx(), _feed_riscv_line(), and _invoke_gdb_backtrace() while replacing the
single-shot rx() usage with incremental feed + explicit EOF flush.
- Around line 1519-1523: Module-level imports for platformio are executed
regardless of _RSP_SERVER_MODE, causing standalone CLI runs to fail; update the
code to make platformio imports lazy or guarded by the same CLI check used to
set _RSP_SERVER_MODE. Specifically, remove or defer the platformio.* imports
that occur at module load and instead import them inside the monitor-only
entrypoints (e.g., within _run_standalone_decoder(), the monitor/server
initialization code, or the existing mock fallback block around
_RSP_SERVER_MODE) or wrap them in try/except ImportError with a clear fallback.
Also apply the same lazy/guarded import approach to the later monitor-only
section around the code referenced at lines ~1575–1630 so platformio-dependent
symbols are only imported when actually running in monitor/RSP server mode.

In `@monitor/README_STANDALONE.md`:
- Around line 70-79: Add a language/info string to the fenced code block
containing the sample output (the triple-backtick block that starts with "Core 
0 register dump:") so markdownlint MD040 is satisfied; update the opening fence
from ``` to ```text (or another appropriate info string) and leave the closing
fence as ``` unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2b362744-0050-4d3a-93c1-b7627b9f8989

📥 Commits

Reviewing files that changed from the base of the PR and between 187e0bf and 6024ac5.

📒 Files selected for processing (2)
  • monitor/README_STANDALONE.md
  • monitor/filter_exception_decoder.py

Comment thread monitor/filter_exception_decoder.py Outdated
Comment thread monitor/filter_exception_decoder.py Outdated
Comment thread monitor/README_STANDALONE.md Outdated
@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
monitor/filter_exception_decoder.py (3)

1542-1549: _rom_matcher not initialized when pyelftools is available.

When HAS_PYELFTOOLS is True, _rom_matcher is never assigned. While this doesn't cause issues currently (since rom_elf_path is None and the code path accessing _rom_matcher is skipped), it leaves the attribute undefined. For defensive consistency with PlatformIO mode initialization:

Suggested fix
     if HAS_PYELFTOOLS:
         decoder._firmware_matcher = PcAddressMatcher(decoder.firmware_path)
         decoder._has_working_matcher = bool(decoder._firmware_matcher.intervals)
+        decoder._rom_matcher = None  # ROM ELF not used in standalone mode
     else:
         decoder._firmware_matcher = None
         decoder._rom_matcher = None
         decoder._has_working_matcher = False
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 1542 - 1549, The
_rom_matcher attribute is left undefined when HAS_PYELFTOOLS is True; set
decoder._rom_matcher in that branch as well (mirroring the PlatformIO-mode
initialization) so the attribute always exists. Specifically, when creating
decoder._firmware_matcher via PcAddressMatcher(decoder.firmware_path) also
initialize decoder._rom_matcher (using PcAddressMatcher(decoder.rom_elf_path) or
None if rom_elf_path is falsy) and keep decoder._has_working_matcher logic
intact; ensure _rom_matcher is defined in both the HAS_PYELFTOOLS True and False
branches.

1458-1467: Silent exception handling hides potential ELF read failures.

When the ELF file cannot be read or parsed, the exception is silently swallowed and the code defaults to Xtensa architecture. This could lead to confusing errors if the wrong toolchain is selected. Consider adding a warning message:

Suggested fix
     try:
         with open(elf_path, "rb") as f:
             # Read ELF header to detect architecture
             elf_header = f.read(20)
             if len(elf_header) >= 18:
                 e_machine = struct.unpack("<H", elf_header[18:20])[0]
                 # 0xF3 = EM_RISCV, 0x5E = EM_XTENSA
                 is_riscv = (e_machine == 0xF3)
-    except Exception:
-        pass
+    except Exception as e:
+        sys.stderr.write("Warning: Could not detect ELF architecture, defaulting to Xtensa: %s\n" % e)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 1458 - 1467, The try/except
around reading the ELF header (variables elf_path, elf_header, e_machine,
is_riscv) is swallowing all exceptions and silently falling back to Xtensa;
update the except block to log a warning including the exception details and the
affected elf_path (e.g., using the module logger or processLogger) and preserve
the fallback behavior only after logging; ensure the log message clarifies that
ELF detection failed and which exception was raised so the wrong toolchain
selection can be diagnosed.

578-603: System PATH search not implemented despite documentation claim.

The method only searches ~/.platformio/packages/toolchain-*/bin/, but the CLI help epilog (lines 1641-1642) claims it also searches "System PATH". Consider either:

  1. Adding PATH search as a fallback (recommended for ESP-IDF standalone users):
Suggested implementation
         if os.path.isdir(pio_packages):
             for pkg_dir in os.listdir(pio_packages):
                 if pkg_dir.startswith("toolchain-"):
                     bin_dir = os.path.join(pio_packages, pkg_dir, "bin")
                     if os.path.isdir(bin_dir):
                         for tool_name in tool_names:
                             candidate = os.path.join(bin_dir, tool_name)
                             if IS_WINDOWS:
                                 candidate += ".exe"
                             if os.path.isfile(candidate):
                                 return candidate
         
+        # Fallback: search system PATH
+        import shutil
+        for tool_name in tool_names:
+            found = shutil.which(tool_name)
+            if found:
+                return found
+        
         return None
  1. Or update the CLI epilog to remove the "System PATH" claim.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 578 - 603, The
_find_toolchain_in_path function currently only checks pio_packages and must
also search the system PATH as the CLI epilog claims; implement a PATH fallback
by iterating os.environ.get("PATH", "").split(os.pathsep), for each dir check
for each tool_name from the tool_names param, append ".exe" when IS_WINDOWS is
true, and return the first os.path.isfile(candidate) found (same return None
otherwise); ensure you reuse the existing IS_WINDOWS and tool_names symbols and
preserve the existing PlatformIO search logic, or if you prefer not to change
behavior, instead update the CLI epilog to remove the "System PATH" claim.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 1542-1549: The _rom_matcher attribute is left undefined when
HAS_PYELFTOOLS is True; set decoder._rom_matcher in that branch as well
(mirroring the PlatformIO-mode initialization) so the attribute always exists.
Specifically, when creating decoder._firmware_matcher via
PcAddressMatcher(decoder.firmware_path) also initialize decoder._rom_matcher
(using PcAddressMatcher(decoder.rom_elf_path) or None if rom_elf_path is falsy)
and keep decoder._has_working_matcher logic intact; ensure _rom_matcher is
defined in both the HAS_PYELFTOOLS True and False branches.
- Around line 1458-1467: The try/except around reading the ELF header (variables
elf_path, elf_header, e_machine, is_riscv) is swallowing all exceptions and
silently falling back to Xtensa; update the except block to log a warning
including the exception details and the affected elf_path (e.g., using the
module logger or processLogger) and preserve the fallback behavior only after
logging; ensure the log message clarifies that ELF detection failed and which
exception was raised so the wrong toolchain selection can be diagnosed.
- Around line 578-603: The _find_toolchain_in_path function currently only
checks pio_packages and must also search the system PATH as the CLI epilog
claims; implement a PATH fallback by iterating os.environ.get("PATH",
"").split(os.pathsep), for each dir check for each tool_name from the tool_names
param, append ".exe" when IS_WINDOWS is true, and return the first
os.path.isfile(candidate) found (same return None otherwise); ensure you reuse
the existing IS_WINDOWS and tool_names symbols and preserve the existing
PlatformIO search logic, or if you prefer not to change behavior, instead update
the CLI epilog to remove the "System PATH" claim.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6acfb000-0a53-46d7-b821-d970dcf253ed

📥 Commits

Reviewing files that changed from the base of the PR and between 187e0bf and 1155c87.

📒 Files selected for processing (2)
  • monitor/README_STANDALONE.md
  • monitor/filter_exception_decoder.py

@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
monitor/filter_exception_decoder.py (1)

1584-1605: ⚠️ Potential issue | 🟡 Minor

EOF flushing still bypasses normal decoding for the buffered tail.

If the crash log does not end with \n, this path only feeds the last line into _feed_riscv_line() and then appends it verbatim. A final PC:SP line, stack-memory line, or register dump never goes through _should_decode_line() / build_*_trace(), so the last frame can still stay undecoded.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 1584 - 1605, The EOF flush
currently bypasses normal decoding by directly feeding decoder.buffer into
_feed_riscv_line and appending final_line verbatim; instead, run the buffered
final_line through the standard decode path: call
decoder._should_decode_line(final_line) and invoke the same
decoding/trace-building functions you use for regular lines (e.g., the existing
build_*_trace or the per-line decode routine used elsewhere) so that RISC-V
PC/SP, stack-memory, and register dumps are handled; only append final_line to
output_buffer after it has been processed (and append any produced trace), and
keep the existing fallback _feed_riscv_line/_invoke_gdb_backtrace logic for
residual state (_is_riscv, _riscv_state, _riscv_regs, _riscv_stack_lines) as
needed.
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)

998-1005: Update the docstring to match the current address semantics.

build_backtrace() no longer decrements return addresses, but this docstring still documents the old -1 rule. That mismatch is risky here because it describes the exact behavior that was intentionally removed. Based on learnings, the is_return_addr / -1 decrement was intentionally removed because ESP-IDF already reports call-site-adjusted addresses and subtracting again breaks RISC-V compressed-instruction decoding.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 998 - 1005, The docstring
for build_backtrace is stale: remove the line stating that "subsequent addresses
are return addresses (decremented by 1)" and update it to reflect current
semantics where return addresses are not decremented because ESP-IDF already
provides call-site-adjusted addresses; mention that build_backtrace pre-fetches
addresses via addr2line and formats them into a numbered trace block, and remove
or replace any reference to is_return_addr / -1 adjustment so the docstring
matches the actual behavior of build_backtrace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 578-603: The _find_toolchain_in_path function only checks
~/.platformio/packages/...; update it to also honor PLATFORMIO_CORE_DIR if set
(check PLATFORMIO_CORE_DIR/packages/*/bin) and to search the system PATH for any
matching tool_names before returning None, appending ".exe" on IS_WINDOWS as
currently done; use shutil.which or iterate PATH entries to locate executables
so system addr2line/gdb are found as a fallback.
- Around line 43-48: Remove the filesystem existence check from the CLI
detection and make standalone detection depend on execution context: update
_CLI_MODE to drop os.path.exists(sys.argv[1]) (only use startswith("-") or
presence of "/" or "\\" or the --rsp-server check via _RSP_SERVER_MODE) and set
_STANDALONE_MODE to include __name__ == "__main__" (e.g., _STANDALONE_MODE =
(__name__ == "__main__") or _RSP_SERVER_MODE or _CLI_MODE) so platform imports
aren't attempted based on filesystem state; adjust references to _CLI_MODE and
_STANDALONE_MODE accordingly in filter_exception_decoder.py.

In `@monitor/README_STANDALONE.md`:
- Around line 21-25: The README's example suggests zero setup but doesn't state
prerequisites or the script path; update the intro and first code block in
monitor/README_STANDALONE.md to explicitly state that Python 3 and an
installed/available ESP toolchain are required, and show two example
invocations: one for running from the repo root (python3
monitor/filter_exception_decoder.py <elf_file> <crash_log>) and one for running
after changing into the monitor directory (cd monitor && python3
filter_exception_decoder.py <elf_file> <crash_log>), plus a note that the ESP
toolchain must be on PATH or otherwise configured before running the script.

---

Duplicate comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 1584-1605: The EOF flush currently bypasses normal decoding by
directly feeding decoder.buffer into _feed_riscv_line and appending final_line
verbatim; instead, run the buffered final_line through the standard decode path:
call decoder._should_decode_line(final_line) and invoke the same
decoding/trace-building functions you use for regular lines (e.g., the existing
build_*_trace or the per-line decode routine used elsewhere) so that RISC-V
PC/SP, stack-memory, and register dumps are handled; only append final_line to
output_buffer after it has been processed (and append any produced trace), and
keep the existing fallback _feed_riscv_line/_invoke_gdb_backtrace logic for
residual state (_is_riscv, _riscv_state, _riscv_regs, _riscv_stack_lines) as
needed.

---

Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 998-1005: The docstring for build_backtrace is stale: remove the
line stating that "subsequent addresses are return addresses (decremented by 1)"
and update it to reflect current semantics where return addresses are not
decremented because ESP-IDF already provides call-site-adjusted addresses;
mention that build_backtrace pre-fetches addresses via addr2line and formats
them into a numbered trace block, and remove or replace any reference to
is_return_addr / -1 adjustment so the docstring matches the actual behavior of
build_backtrace.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d31bbebb-b3c9-4d2a-9937-f22820a6b90f

📥 Commits

Reviewing files that changed from the base of the PR and between 187e0bf and 8453d13.

📒 Files selected for processing (2)
  • monitor/README_STANDALONE.md
  • monitor/filter_exception_decoder.py

Comment thread monitor/filter_exception_decoder.py Outdated
Comment thread monitor/filter_exception_decoder.py
Comment thread monitor/README_STANDALONE.md Outdated
Jason2866 and others added 2 commits March 8, 2026 16:18
@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
monitor/filter_exception_decoder.py (3)

1478-1481: Creating a decoder instance just for toolchain search is an odd pattern.

Instantiating Esp32ExceptionDecoder() solely to call _find_toolchain_in_path() works but couples these functions unnecessarily. Consider making _find_toolchain_in_path a module-level function or a @staticmethod.

📝 Suggested refactor
+def _find_toolchain_in_path_standalone(tool_names):
+    """Search for a toolchain binary in ~/.platformio/packages/toolchain-*/bin."""
+    home = os.path.expanduser("~")
+    pio_packages = os.path.join(home, ".platformio/packages")
+    
+    if os.path.isdir(pio_packages):
+        for pkg_dir in os.listdir(pio_packages):
+            if pkg_dir.startswith("toolchain-"):
+                bin_dir = os.path.join(pio_packages, pkg_dir, "bin")
+                if os.path.isdir(bin_dir):
+                    for tool_name in tool_names:
+                        candidate = os.path.join(bin_dir, tool_name)
+                        if IS_WINDOWS:
+                            candidate += ".exe"
+                        if os.path.isfile(candidate):
+                            return candidate
+    return None
+
 def _find_toolchain_binaries(elf_path):
     ...
-    decoder = Esp32ExceptionDecoder()
-    addr2line_path = decoder._find_toolchain_in_path(addr2line_names)
-    gdb_path = decoder._find_toolchain_in_path(gdb_names)
+    addr2line_path = _find_toolchain_in_path_standalone(addr2line_names)
+    gdb_path = _find_toolchain_in_path_standalone(gdb_names)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 1478 - 1481, The code
creates an Esp32ExceptionDecoder instance only to call _find_toolchain_in_path,
which couples toolchain lookup to the class; change _find_toolchain_in_path into
a module-level function or a `@staticmethod` on Esp32ExceptionDecoder, then update
callers (e.g., the code using decoder = Esp32ExceptionDecoder(); addr2line_path
= decoder._find_toolchain_in_path(addr2line_names); gdb_path =
decoder._find_toolchain_in_path(gdb_names)) to call the new function directly
(either _find_toolchain_in_path(addr2line_names) /
_find_toolchain_in_path(gdb_names) if module-level, or
Esp32ExceptionDecoder._find_toolchain_in_path(addr2line_names) /
Esp32ExceptionDecoder._find_toolchain_in_path(gdb_names) if staticmethod), and
remove the temporary instance creation.

579-604: Toolchain search may match unintended versions.

The method iterates all toolchain-* directories and returns the first match. If multiple toolchain versions are installed (e.g., toolchain-xtensa-esp-elf and toolchain-xtensa-esp32-elf), the first one encountered in os.listdir() order (undefined) is returned. The platform.py uses specific toolchain names from MCU_TOOLCHAIN_CONFIG (e.g., "toolchain-xtensa-esp-elf", "toolchain-riscv32-esp"), but this method doesn't prioritize them.

Consider sorting the directories to prefer newer or canonical names, or filtering by exact expected package names based on architecture.

📝 Suggested improvement to prioritize toolchain packages
     # Search in PlatformIO packages
     home = os.path.expanduser("~")
     pio_packages = os.path.join(home, ".platformio/packages")
     
     if os.path.isdir(pio_packages):
-        for pkg_dir in os.listdir(pio_packages):
+        # Sort directories to prefer newer toolchain naming conventions
+        pkg_dirs = sorted(os.listdir(pio_packages), reverse=True)
+        for pkg_dir in pkg_dirs:
             if pkg_dir.startswith("toolchain-"):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 579 - 604,
_find_toolchain_in_path currently returns the first toolchain-* dir found which
can pick an unintended version; update it to prioritize matching package names
and newer versions by (1) deriving expected package names from
MCU_TOOLCHAIN_CONFIG (or a small hardcoded list of canonical names) and using
those to filter pkg_dir values so exact matches (e.g.,
"toolchain-xtensa-esp-elf") are checked first, (2) otherwise sort the remaining
toolchain-* directories in a deterministic order (e.g., reverse lexical or
version-aware sort) so newer versions are preferred, and (3) then iterate this
prioritized list to search bin/<tool_name> (respecting IS_WINDOWS) and return on
first hit; refer to function _find_toolchain_in_path and MCU_TOOLCHAIN_CONFIG
when making these changes.

1467-1468: Blind exception catch is acceptable but could be narrowed.

The except Exception at line 1467 is flagged by Ruff (BLE001). While this is a fallback scenario where the warning is informative, consider catching more specific exceptions (OSError, struct.error) to avoid masking unexpected errors.

📝 Optional fix
-    except Exception as e:
+    except (OSError, struct.error) as e:
         sys.stderr.write("Warning: Could not detect ELF architecture, defaulting to Xtensa: %s\n" % e)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@monitor/filter_exception_decoder.py` around lines 1467 - 1468, Replace the
broad "except Exception as e" used when detecting ELF architecture with a narrow
exception tuple like "except (OSError, struct.error) as e" (and add "import
struct" if missing) so only expected parsing/IO errors are caught; keep the
existing sys.stderr.write warning unchanged and do not swallow other unexpected
exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 1593-1598: The code may duplicate the final_line when a RISC-V
dump was already processed; change the logic around decoder._is_riscv /
decoder._feed_riscv_line(final_line.rstrip('\n')) so that if _feed_riscv_line
returns True (and you append the trace from decoder._invoke_gdb_backtrace()),
you do not also call output_buffer.append(final_line); otherwise (when
_feed_riscv_line returns False or decoder._is_riscv is False) still append
final_line as raw text. Use the existing symbols decoder._is_riscv,
decoder._feed_riscv_line, decoder._invoke_gdb_backtrace, and
output_buffer.append(final_line) to locate and adjust the branching.

---

Nitpick comments:
In `@monitor/filter_exception_decoder.py`:
- Around line 1478-1481: The code creates an Esp32ExceptionDecoder instance only
to call _find_toolchain_in_path, which couples toolchain lookup to the class;
change _find_toolchain_in_path into a module-level function or a `@staticmethod`
on Esp32ExceptionDecoder, then update callers (e.g., the code using decoder =
Esp32ExceptionDecoder(); addr2line_path =
decoder._find_toolchain_in_path(addr2line_names); gdb_path =
decoder._find_toolchain_in_path(gdb_names)) to call the new function directly
(either _find_toolchain_in_path(addr2line_names) /
_find_toolchain_in_path(gdb_names) if module-level, or
Esp32ExceptionDecoder._find_toolchain_in_path(addr2line_names) /
Esp32ExceptionDecoder._find_toolchain_in_path(gdb_names) if staticmethod), and
remove the temporary instance creation.
- Around line 579-604: _find_toolchain_in_path currently returns the first
toolchain-* dir found which can pick an unintended version; update it to
prioritize matching package names and newer versions by (1) deriving expected
package names from MCU_TOOLCHAIN_CONFIG (or a small hardcoded list of canonical
names) and using those to filter pkg_dir values so exact matches (e.g.,
"toolchain-xtensa-esp-elf") are checked first, (2) otherwise sort the remaining
toolchain-* directories in a deterministic order (e.g., reverse lexical or
version-aware sort) so newer versions are preferred, and (3) then iterate this
prioritized list to search bin/<tool_name> (respecting IS_WINDOWS) and return on
first hit; refer to function _find_toolchain_in_path and MCU_TOOLCHAIN_CONFIG
when making these changes.
- Around line 1467-1468: Replace the broad "except Exception as e" used when
detecting ELF architecture with a narrow exception tuple like "except (OSError,
struct.error) as e" (and add "import struct" if missing) so only expected
parsing/IO errors are caught; keep the existing sys.stderr.write warning
unchanged and do not swallow other unexpected exceptions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f4201c35-cf1a-48a1-93e2-05122addaf02

📥 Commits

Reviewing files that changed from the base of the PR and between 187e0bf and 80cf4c9.

📒 Files selected for processing (2)
  • monitor/README_STANDALONE.md
  • monitor/filter_exception_decoder.py

Comment thread monitor/filter_exception_decoder.py Outdated
@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

✅ Actions performed

Full review triggered.

@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 8, 2026

✅ Actions performed

Full review triggered.

@Jason2866 Jason2866 merged commit 2e3938b into develop Mar 8, 2026
1 check passed
@Jason2866 Jason2866 deleted the cli_decoder branch March 8, 2026 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant