Enhance exception decoder to decode rom function calls #320
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 You can disable this status message by setting the WalkthroughAdds context-aware ESP32 backtrace decoding with ROM ELF discovery and addr2line-based address decoding; integrates automatic ROM ELF tool installation into platform package configuration when the exception decoder monitor filter is enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Build System
participant Platform as platform.py
participant ToolMgr as ToolPackageManager
participant Decoder as filter_exception_decoder
participant Addr2Line as addr2line
Build->>Platform: configure_default_packages()
Platform->>Platform: _check_exception_decoder_filter(variables)
alt decoder present
Platform->>ToolMgr: _configure_rom_elfs_for_exception_decoder()
ToolMgr->>ToolMgr: install tool-esp-rom-elfs
else decoder absent
Platform->>Platform: log skip
end
Note over Decoder: During serial monitor processing
Decoder->>Decoder: is_backtrace_context(line)
alt in backtrace context
Decoder->>Decoder: extract address matches
loop each address
Decoder->>Addr2Line: decode_address(addr, app_elf)
alt addr decoded in app
Decoder->>Decoder: build_backtrace(app result)
else try ROM
Decoder->>ToolMgr: find_rom_elf(chip_name)
Decoder->>Addr2Line: decode_address(addr, rom_elf)
Decoder->>Decoder: build_backtrace(rom result, "in ROM")
end
end
Decoder->>Decoder: strip_project_dir(trace)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@Xylopyrographer If you want try this one. Added rom elfs to decode calls in rom |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monitor/filter_exception_decoder.py (1)
209-235: Harden path setup: handle missing keys and add addr2line fallback discovery
- Accessing
data["prog_path"]can raiseKeyError.- If replacing
-gcc/-clangfails, tryshutil.whichfallbacks (addr2lineor toolchain-prefixed).@@ - # Locate firmware ELF file - self.firmware_path = data["prog_path"] + # Locate firmware ELF file + self.firmware_path = data.get("prog_path") + if not self.firmware_path: + sys.stderr.write( + "%s: prog_path missing in build metadata; rebuild the project?\n" + % self.__class__.__name__ + ) + return False if not os.path.isfile(self.firmware_path): @@ - cc_path = data.get("cc_path", "") + cc_path = data.get("cc_path", "") if "-gcc" in cc_path: path = cc_path.replace("-gcc", "-addr2line") if os.path.isfile(path): self.addr2line_path = path elif "-clang" in cc_path: # Support for Clang toolchain path = cc_path.replace("-clang", "-addr2line") if os.path.isfile(path): self.addr2line_path = path + # Fallbacks + if not self.addr2line_path: + try: + import shutil + # Try plain addr2line on PATH + self.addr2line_path = shutil.which("addr2line") + except Exception: + self.addr2line_path = None @@ if not self.addr2line_path: sys.stderr.write( "%s: disabling, failed to find addr2line.\n" % self.__class__.__name__ ) return False
🧹 Nitpick comments (7)
monitor/filter_exception_decoder.py (7)
45-47: Annotate class-level regex/maps with ClassVar to satisfy Ruff (RUF012) and clarify intentMark regexes/maps as ClassVar and type them. Also import typing.Pattern. This is non-behavioral and keeps linters quiet.
@@ -from platformio.project.config import ProjectConfig +from platformio.project.config import ProjectConfig +from typing import ClassVar, Pattern @@ class Esp32ExceptionDecoder(DeviceMonitorFilterBase): - ADDR_PATTERN = re.compile(r"((?:0x[0-9a-fA-F]{8}:0x[0-9a-fA-F]{8}(?: |$))+)") - ADDR_SPLIT = re.compile(r"[ :]") - PREFIX_RE = re.compile(r"^ *") + ADDR_PATTERN: ClassVar[Pattern[str]] = re.compile(r"((?:0x[0-9a-fA-F]{8}:0x[0-9a-fA-F]{8}(?: |$))+)") + ADDR_SPLIT: ClassVar[Pattern[str]] = re.compile(r"[ :]") + PREFIX_RE: ClassVar[Pattern[str]] = re.compile(r"^ *") @@ - BACKTRACE_KEYWORDS = re.compile( + BACKTRACE_KEYWORDS: ClassVar[Pattern[str]] = re.compile( @@ - CHIP_NAME_MAP = { + CHIP_NAME_MAP: ClassVar[dict[str, str]] = {Also applies to: 51-63, 66-75
108-137: Docstring param name is missing; clarifydataand return typeMinor doc fix improves readability and tools.
- Args: - Build metadata dictionary containing board and MCU information + Args: + data: Build metadata dictionary containing board and MCU information. @@ - Returns: - str: Chip name (e.g., "esp32", "esp32s3") or "esp32" as fallback + Returns: + str: Chip name (e.g., "esp32", "esp32s3") or "esp32" as fallback.
360-377: Only decode PC addresses; drop empties to cut useless addr2line callsCurrently both PC and SP tokens are returned; SP lookups always fail and waste subprocess calls. Return only even-indexed tokens (PCs) and filter empties.
- addresses = self.ADDR_SPLIT.split(addresses_str) - size = len(addresses) - while size > 1 and self.is_address_ignored(addresses[size-1]): - size -= 1 - return addresses[:size] + tokens = [t for t in self.ADDR_SPLIT.split(addresses_str) if t] + # PCs are at even indices in PC:SP pairs + pcs = [t for i, t in enumerate(tokens) if i % 2 == 0 and not self.is_address_ignored(t)] + return pcs
348-359: Broaden zero-address detection to include 0x0 and 0Catches more “null” representations without changing behavior.
- return address in ("", "0x00000000") + if not address: + return True + a = address.lower() + return a in ("0x00000000", "0x0", "0")
473-491: Make path stripping OS-agnostic and avoid off-by-one slicingHandle Windows backslashes and optional trailing separator via regex.
- while True: - idx = trace.find(self.project_dir) - if idx == -1: - break - trace = trace[:idx] + trace[idx + len(self.project_dir) + 1 :] - return trace + sep = re.escape(os.sep) + pattern = re.compile(rf"{re.escape(self.project_dir)}{sep}?") + return pattern.sub("", trace)
378-410: Subprocess input is controlled; still, sanitize defensively to satisfy S603-likesAddresses already come from
ADDR_PATTERN, but add a simple guard to fail fast on unexpected tokens.- args = [self.addr2line_path, u"-fipC", u"-e", elf_path, addr] + if not re.fullmatch(r"0x[0-9a-fA-F]+", addr): + return None + args = [self.addr2line_path, "-fipC", "-e", elf_path, addr]
411-472: Optional: batch addr2line calls per trace for speedYou invoke
addr2lineper address.addr2lineaccepts multiple addresses; batching can cut subprocess overhead significantly on long backtraces. If interested, I can draft a minimaldecode_addresses([...], elf_path)helper and wire it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
monitor/filter_exception_decoder.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
monitor/filter_exception_decoder.py
66-75: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
187-187: Do not catch blind exception: Exception
(BLE001)
251-251: Consider moving this statement to an else block
(TRY300)
394-394: subprocess call: check for execution of untrusted input
(S603)
406-406: Consider moving this statement to an else block
(TRY300)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
66-75: Consider annotating withtyping.ClassVar.For better type safety and to follow Python best practices, consider annotating this class-level constant dictionary.
Apply this diff:
+from typing import ClassVar + class Esp32ExceptionDecoder(DeviceMonitorFilterBase): """ PlatformIO device monitor filter for decoding ESP32 exception backtraces. ... # Chip name mapping for ROM ELF files - CHIP_NAME_MAP = { + CHIP_NAME_MAP: ClassVar[dict[str, str]] = { "esp32": "esp32", "esp32s2": "esp32s2", "esp32s3": "esp32s3", "esp32c2": "esp32c2", "esp32c3": "esp32c3", "esp32c6": "esp32c6", "esp32h2": "esp32h2", "esp32p4": "esp32p4", }Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
monitor/filter_exception_decoder.py(7 hunks)platform.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-29T11:35:26.182Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting `self.packages[tl_install_name]["optional"] = True` after version verification is the intended design pattern.
Applied to files:
platform.json
🪛 Ruff (0.14.0)
monitor/filter_exception_decoder.py
66-75: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
260-260: Consider moving this statement to an else block
(TRY300)
403-403: subprocess call: check for execution of untrusted input
(S603)
415-415: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (11)
monitor/filter_exception_decoder.py (10)
35-41: Excellent documentation!The class docstring clearly explains the filter's purpose and functionality.
50-63: Comprehensive backtrace detection pattern.The regex pattern covers all major ESP32 error scenarios, which will help avoid false positives on random hex values in normal output.
77-106: Well-structured initialization with context tracking.The new state variables for backtrace context tracking are properly initialized and will help reduce false positives.
233-243: Good addition of Clang toolchain support.The fallback logic properly handles both GCC and Clang-based toolchains.
245-259: Excellent ROM ELF integration with informative logging.The setup properly attempts to find ROM ELF files and provides clear feedback about success or failure, allowing the filter to work even without ROM support.
269-311: Smart context-aware processing to reduce false positives.The stateful context tracking with keyword detection, line counting, and empty-line termination ensures addresses are only decoded within actual exception dumps.
343-345: Clean integration of context checking.The guard clause prevents unnecessary address decoding on lines outside backtrace contexts.
387-418: Solid address decoding implementation with proper error handling.The subprocess call is safe because addresses are validated by regex to be hex format. The "?? ??:0" check correctly identifies failed lookups.
446-470: Excellent dual-ELF decoding with clear ROM indication.The fallback from application ELF to ROM ELF is well-implemented, and the "in ROM" suffix helps users distinguish ROM code from application code. The split on " at " has a sensible fallback for unexpected formats.
482-499: Clear documentation for path stripping.The added docstring explains the purpose of removing project directory prefixes for readability.
platform.json (1)
181-187: Appropriate change to make ROM ELF package required.Making
tool-esp-rom-elfsrequired ensures full decoding functionality is available since the enhanced exception decoder now depends on ROM ELF files for decoding ROM addresses. This aligns with the runtime requirements of the decoder.
Refactor ROM ELF file pattern matching and sorting logic.
Removed unused import statement for 'fs'.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
monitor/filter_exception_decoder.py (1)
107-135: Fix incomplete docstring parameter documentation.The Args section on line 115 is missing the parameter name.
Apply this diff:
Args: - Build metadata dictionary containing board and MCU information + data: Build metadata dictionary containing board and MCU information
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
65-74: Consider annotating mutable class attributes withClassVar.The
CHIP_NAME_MAPdictionary is a class-level constant that should be annotated withtyping.ClassVarto clarify it's not an instance attribute.Apply this diff:
+from typing import ClassVar + class Esp32ExceptionDecoder(DeviceMonitorFilterBase): """ PlatformIO device monitor filter for decoding ESP32 exception backtraces. ... """ NAME = "esp32_exception_decoder" # More specific pattern for PC:SP pairs in backtraces ADDR_PATTERN = re.compile(r"((?:0x[0-9a-fA-F]{8}:0x[0-9a-fA-F]{8}(?: |$))+)") ADDR_SPLIT = re.compile(r"[ :]") PREFIX_RE = re.compile(r"^ *") # Patterns that indicate we're in an exception/backtrace context BACKTRACE_KEYWORDS = re.compile( r"(Backtrace:|" r"abort\(\) was called at PC|" r"Guru Meditation Error:|" r"panic'ed|" r"register dump:|" r"Stack smashing protect failure!|" r"CORRUPT HEAP:|" r"assertion .* failed:|" r"Debug exception reason:|" r"Undefined behavior of type)", re.IGNORECASE ) # Chip name mapping for ROM ELF files - CHIP_NAME_MAP = { + CHIP_NAME_MAP: ClassVar[dict] = { "esp32": "esp32", "esp32s2": "esp32s2", "esp32s3": "esp32s3", "esp32c2": "esp32c2", "esp32c3": "esp32c3", "esp32c6": "esp32c6", "esp32h2": "esp32h2", "esp32p4": "esp32p4", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
monitor/filter_exception_decoder.py(7 hunks)platform.json(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
monitor/filter_exception_decoder.py
65-74: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
272-272: Consider moving this statement to an else block
(TRY300)
415-415: subprocess call: check for execution of untrusted input
(S603)
427-427: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-15, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (ubuntu-latest, examples/espidf-exceptions)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
🔇 Additional comments (11)
platform.json (1)
183-183: LGTM! Required package aligns with decoder enhancements.Making
tool-esp-rom-elfsrequired is appropriate given that the exception decoder now relies on ROM ELF files for decoding ROM function calls. This ensures the package is always available for the enhanced backtrace decoding functionality.monitor/filter_exception_decoder.py (10)
19-19: LGTM! Imports support ROM ELF discovery.The
globandToolPackageManagerimports are appropriately used in thefind_rom_elfmethod for locating ROM ELF files.Also applies to: 27-27
34-40: LGTM! Clear and comprehensive class documentation.The enhanced docstring effectively describes the filter's purpose and capabilities.
76-105: LGTM! Initialization properly sets up context tracking.The enhanced
__call__method correctly initializes the backtrace context tracking state and ROM ELF path, with clear documentation.
137-213: LGTM! ROM ELF discovery implementation is robust.The method effectively addresses previous review concerns:
- Uses
ToolPackageManagerfor package access- Implements multiple glob patterns for flexible file matching
- Sorts by numeric revision with proper handling of non-revisioned files
- Checks for empty results before accessing the list
- Catches specific exception types rather than bare
ExceptionThe implementation is solid and handles edge cases appropriately.
215-279: LGTM! Enhanced setup with ROM ELF discovery and Clang support.The method properly locates firmware, addr2line tool, and ROM ELF files, with appropriate logging at each stage. The addition of Clang toolchain support (lines 245-249) is a useful enhancement.
281-323: LGTM! Context detection prevents false positives.The context detection methods effectively limit address decoding to actual backtrace sections, avoiding false matches on random hex values in normal output. The state management with line counting and empty line detection is well-designed.
325-367: LGTM! Efficient context-aware processing integration.The
rxmethod cleanly integrates context checking before processing lines, improving accuracy without significant overhead.
369-397: LGTM! Address filtering logic is clear and correct.The helper methods properly filter out invalid and trailing null addresses with clear documentation.
399-430: LGTM! Address decoding implementation is correct.The method properly invokes
addr2lineand handles failures gracefully. The static analysis warnings (S603 subprocess, TRY300 else block) are false positives in this context—the executable path and arguments come from controlled build configuration, not untrusted user input.
432-511: LGTM! Dual-ELF decoding strategy is well-implemented.The
build_backtracemethod elegantly implements the app-ELF-first, ROM-ELF-fallback strategy with clear "in ROM" annotations. Thestrip_project_dirutility improves readability of decoded paths. Both methods handle edge cases appropriately.
Added methods to check and configure ESP32 exception decoder.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
monitor/filter_exception_decoder.py (3)
65-74: AddClassVarannotation for class-level constant.The
CHIP_NAME_MAPdictionary is a class-level constant that should be annotated withtyping.ClassVarto indicate it's not an instance attribute.Apply this diff:
+from typing import ClassVar + class Esp32ExceptionDecoder(DeviceMonitorFilterBase): """ PlatformIO device monitor filter for decoding ESP32 exception backtraces. This filter automatically decodes memory addresses from ESP32 crash dumps into human-readable function names and source code locations using addr2line. It supports both application code and ROM addresses via ESP ROM ELF files. """ NAME = "esp32_exception_decoder" # More specific pattern for PC:SP pairs in backtraces ADDR_PATTERN = re.compile(r"((?:0x[0-9a-fA-F]{8}:0x[0-9a-fA-F]{8}(?: |$))+)") ADDR_SPLIT = re.compile(r"[ :]") PREFIX_RE = re.compile(r"^ *") # Patterns that indicate we're in an exception/backtrace context BACKTRACE_KEYWORDS = re.compile( r"(Backtrace:|" r"abort\(\) was called at PC|" r"Guru Meditation Error:|" r"panic'ed|" r"register dump:|" r"Stack smashing protect failure!|" r"CORRUPT HEAP:|" r"assertion .* failed:|" r"Debug exception reason:|" r"Undefined behavior of type)", re.IGNORECASE ) # Chip name mapping for ROM ELF files - CHIP_NAME_MAP = { + CHIP_NAME_MAP: ClassVar[dict] = { "esp32": "esp32", "esp32s2": "esp32s2", "esp32s3": "esp32s3", "esp32c2": "esp32c2", "esp32c3": "esp32c3", "esp32c6": "esp32c6", "esp32h2": "esp32h2", "esp32p4": "esp32p4", }
245-272: Clang support and ROM ELF integration look good.The addition of Clang toolchain support and ROM ELF discovery is well-implemented with clear informative messages.
For improved code clarity, consider moving the
return Truestatement to anelseblock as suggested by static analysis:if self.rom_elf_path: sys.stderr.write( "%s: ROM ELF found at %s\n" % (self.__class__.__name__, self.rom_elf_path) ) else: sys.stderr.write( "%s: ROM ELF not found for chip %s, ROM addresses will not be decoded\n" % (self.__class__.__name__, chip_name) ) - - return True except PlatformioException as e: sys.stderr.write( "%s: disabling, exception while looking for addr2line: %s\n" % (self.__class__.__name__, e) ) return False + else: + return True
399-430: Address decoding logic is sound.The method correctly handles address decoding with proper error handling and detection of failed lookups ("?? ??:0").
Static analysis raised two points:
- S603 (subprocess security): The subprocess call uses validated paths and addresses. The
addr2line_pathcomes from build metadata andaddris a hex string. This is safe in context.- TRY300 (style): Consider moving
return outputto anelseblock for clarity:# Check if address was found in ELF if output == "?? ??:0": return None - - return output except subprocess.CalledProcessError: return None + else: + return outputplatform.py (1)
86-91: Reconsider adding ROM ELFs to common IDF packages.Adding
tool-esp-rom-elfstoCOMMON_IDF_PACKAGES(line 90) will install it for all ESP-IDF builds, even when the exception decoder filter is not configured. Since line 835 already conditionally installs it via_configure_rom_elfs_for_exception_decoder()when the decoder is enabled, adding it here may be redundant and increase installation size unnecessarily.Consider removing it from
COMMON_IDF_PACKAGESand relying solely on the conditional installation:COMMON_IDF_PACKAGES = [ "tool-cmake", "tool-ninja", "tool-scons", - "tool-esp-rom-elfs" ]This ensures ROM ELFs are only installed when actually needed (i.e., when
esp32_exception_decoderis inmonitor_filters).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
monitor/filter_exception_decoder.py(7 hunks)platform.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
monitor/filter_exception_decoder.py
65-74: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
272-272: Consider moving this statement to an else block
(TRY300)
415-415: subprocess call: check for execution of untrusted input
(S603)
427-427: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-15, examples/arduino-zigbee-light)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
🔇 Additional comments (11)
monitor/filter_exception_decoder.py (8)
19-19: LGTM! Imports and documentation are clear.The new imports (
globfor file pattern matching andToolPackageManagerfor package discovery) are properly utilized in thefind_rom_elfmethod. The enhanced class docstring clearly explains the filter's purpose.Also applies to: 27-27, 33-40
76-105: LGTM! Context tracking initialization is well-designed.The backtrace context tracking mechanism with
in_backtrace_context,lines_since_context, andmax_context_lines=50provides a good balance between capturing relevant traces and avoiding false positives on random hex values.
107-135: LGTM! Chip detection logic is robust.The method correctly tries multiple strategies to detect the chip type (board name, then MCU) with a sensible fallback to "esp32". The docstring is well-formatted and complete.
137-213: LGTM! ROM ELF discovery is well-implemented.The method correctly uses
ToolPackageManager().get_package()to access the ROM ELF package, handles multiple file naming patterns, sorts by numeric revision for compatibility, and includes proper error handling with empty list checks and specific exception catching.
281-323: LGTM! Context-aware processing is well-designed.The context detection mechanism effectively limits address decoding to actual exception backtraces, avoiding false positives on random hex values in normal output. The logic for entering/exiting context (backtrace keywords, empty lines, max line count) is sound.
355-367: LGTM! Context filtering integration is correct.The integration of
should_process_line()ensures that only lines within a backtrace context are processed, effectively reducing false positives.
458-482: LGTM! ROM address decoding is well-implemented.The two-step decode process (application ELF first, then ROM ELF) is the correct approach. The "in ROM" suffix clearly indicates which addresses were decoded from ROM code, with a reasonable fallback when the output format doesn't contain " at ".
494-511: LGTM! Documentation enhancement is clear.The enhanced docstring clearly explains the method's purpose of improving readability by showing relative paths.
platform.py (3)
670-686: LGTM! Filter detection handles both list and string formats.The method correctly handles
monitor_filtersin both list format and comma-separated string format, making it robust across different configuration styles.
688-702: LGTM! Conditional ROM ELF installation is well-implemented.The method correctly detects when the exception decoder filter is configured and conditionally installs the ROM ELF package only when needed, with clear logging.
835-835: LGTM! Integration point is appropriate.Calling
_configure_rom_elfs_for_exception_decoder()after framework configuration and before other tools is the right placement, allowing the exception decoder to work with any framework.
|
@coderabbitai The suggestion to remove tool-esp-rom-elfs from COMMON_IDF_PACKAGES is wrong since espidf.py needs this packages for correct setup! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 41064 🏁 Script executed: Length of output: 107287 🏁 Script executed: Length of output: 17917 🏁 Script executed: Length of output: 5301 @Jason2866, thank you for the correction! You're absolutely right. I understand now that
The current implementation is correct:
This ensures the package is available for both ESP-IDF framework builds and for Arduino framework builds that use the exception decoder. Thank you for clarifying this! ✏️ Learnings added
|
Added support for esp32c5 in exception patterns.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
monitor/filter_exception_decoder.py (1)
66-76: Consider adding ClassVar annotation for class-level constants.The
CHIP_NAME_MAPdictionary is a class-level constant that could benefit from aClassVarannotation for clarity, though this is a minor style suggestion.+from typing import ClassVar + class Esp32ExceptionDecoder(DeviceMonitorFilterBase): # ... - # Chip name mapping for ROM ELF files - CHIP_NAME_MAP = { + # Chip name mapping for ROM ELF files + CHIP_NAME_MAP: ClassVar[dict] = { "esp32": "esp32",Same applies to
BACKTRACE_KEYWORDSif desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/arduino-usb-keyboard/platformio.ini(1 hunks)monitor/filter_exception_decoder.py(7 hunks)platform.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#0
File: :0-0
Timestamp: 2025-10-19T17:30:42.368Z
Learning: The tool-esp-rom-elfs package must remain in COMMON_IDF_PACKAGES in platform.py because espidf.py requires it to set the ESP_ROM_ELF_DIR environment variable for the ESP-IDF build system, independent of the exception decoder monitor filter usage.
📚 Learning: 2025-10-19T17:30:42.368Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#0
File: :0-0
Timestamp: 2025-10-19T17:30:42.368Z
Learning: The tool-esp-rom-elfs package must remain in COMMON_IDF_PACKAGES in platform.py because espidf.py requires it to set the ESP_ROM_ELF_DIR environment variable for the ESP-IDF build system, independent of the exception decoder monitor filter usage.
Applied to files:
platform.pymonitor/filter_exception_decoder.py
🪛 Ruff (0.14.0)
monitor/filter_exception_decoder.py
66-76: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
274-274: Consider moving this statement to an else block
(TRY300)
417-417: subprocess call: check for execution of untrusted input
(S603)
429-429: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-15, examples/espidf-ulp)
- GitHub Check: build (macos-15, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-coap-server)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
🔇 Additional comments (13)
examples/arduino-usb-keyboard/platformio.ini (1)
14-14: LGTM! Example configuration correctly enables the exception decoder.This demonstrates how users should configure the new ROM-aware exception decoder in their projects.
platform.py (3)
670-686: LGTM! Filter detection logic correctly handles both list and string formats.The method properly normalizes monitor_filters from either format and checks for the decoder presence.
688-702: LGTM! ROM ELF installation correctly handles both Arduino and ESP-IDF frameworks.For ESP-IDF builds, ROM ELFs are installed via
COMMON_IDF_PACKAGES. For Arduino-only builds, this method ensures ROM ELFs are installed when the decoder is active. This aligns with the architecture confirmed in the PR discussion.Based on learnings.
835-835: LGTM! ROM ELF configuration properly integrated into the package setup flow.The placement ensures ROM ELFs are available when the decoder is enabled, regardless of framework choice.
monitor/filter_exception_decoder.py (9)
78-107: LGTM! Initialization properly sets up context tracking and ROM ELF support.The new state variables for backtrace context tracking and ROM ELF path are correctly initialized.
109-137: LGTM! Chip detection logic correctly prioritizes board name, then MCU.The fallback to "esp32" is reasonable for maximum compatibility.
139-215: LGTM! ROM ELF discovery robustly handles multiple naming patterns with proper error handling.The numeric revision sorting and empty-list check address previous review concerns. The method correctly relies on the package being pre-installed by platform.py.
217-281: LGTM! Path setup comprehensively locates all required tools including ROM ELFs.The addition of Clang toolchain support and clear logging for ROM ELF status improves the robustness of the decoder. The optional ROM ELF behavior (log warning but continue) is appropriate since ROM decoding is an enhancement, not a requirement.
283-325: LGTM! Context-aware processing prevents false positives on random hex values.The state machine correctly enters backtrace context on keywords, processes lines within the context window, and exits on empty lines or line limit. This is a significant improvement over unconditional address decoding.
327-369: LGTM! Main processing loop correctly integrates context-aware filtering.The addition of
should_process_linecheck ensures addresses are only decoded within crash dump contexts, significantly reducing false positives.
401-432: LGTM! Address decoding properly handles addr2line invocation and output parsing.The method correctly identifies when an address cannot be resolved (multiple "??" variants) and returns None for clean fallback behavior.
434-494: LGTM! Backtrace building implements smart cascading decode: app ELF first, ROM ELF fallback.The "in ROM" annotation clearly distinguishes ROM addresses from application code, making crash analysis more intuitive. The format parsing correctly handles both simple and complex addr2line output.
496-513: LGTM! Project directory stripping improves output readability.The method correctly removes absolute path prefixes to show only relative paths in the decoded output.
Removed debug log for esp32_exception_decoder configuration.
|
@Xylopyrographer Can you please try this version if it still works? This one takes care of context and adds decoding of rom functions. Use with: |
|
@Jason2866 Confirming |
2 participants
Description:
Related issue (if applicable): fixes #
Checklist:
Summary by CodeRabbit
New Features
Chores
Bug Fixes