Implement coredump analysis targets with CLI support. #301
Conversation
Add custom targets for coredump analysis with CLI support.
|
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 a coredump analysis capability invoking esp-coredump via a new coredump_analysis function, registers two SCons custom targets (“coredump” and “coredump-only”), configures ESP-IDF-related environment variables, supports CLI args after “--”, updates Python deps to include esp-coredump, and duplicates the coredump function and targets within builder/main.py. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant SCons as SCons/Env
participant CA as coredump_analysis()
participant FS as Filesystem
participant Proc as subprocess(esp-coredump)
Dev->>SCons: Invoke target "coredump" or "coredump-only"
alt coredump
SCons->>FS: Ensure `$BUILD_DIR/${PROGNAME}.elf` exists
end
SCons->>CA: Call coredump_analysis(target, source, env)
rect rgb(245, 250, 255)
note right of CA: Parse CLI args after `--`\nUse custom_esp_coredump_args/corefile or defaults
CA->>FS: Locate ELF (build dir or project root)
FS-->>CA: ELF found / not found
end
alt ELF found
rect rgb(235, 255, 235)
note over CA: Ensure ESP-IDF and rom-elfs tool\nSet IDF_PATH / ESP_ROM_ELF_DIR if available
CA->>Proc: Run `esp-coredump` with constructed args/env
Proc-->>CA: Exit code
alt Exit != 0
CA-->>SCons: Warn non-zero exit / provide guidance
else Success
CA-->>SCons: Complete successfully
end
end
else ELF missing
CA-->>SCons: Emit error guidance about missing ELF or corefile
end
note over SCons: coredump targets and function appear twice (duplicate registrations)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/main.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
builder/main.py
653-653: Unused function argument: target
(ARG001)
653-653: Unused function argument: source
(ARG001)
704-704: subprocess call: check for execution of untrusted input
(S603)
712-712: Do not catch blind exception: Exception
(BLE001)
⏰ 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/tasmota)
- GitHub Check: build (macos-15, examples/espidf-ulp-riscv)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (macos-15, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
Refactor command construction to include default parameters and handle extra arguments from platformio.ini.
Set up ESP-IDF environment variables for coredump execution.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
builder/frameworks/espidf.py(1 hunks)builder/main.py(2 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-10T19:13:25.599Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/penv_setup.py
📚 Learning: 2025-09-23T14:15:00.476Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#0
File: :0-0
Timestamp: 2025-09-23T14:15:00.476Z
Learning: In the pioarduino/platform-espressif32 project, espidf.py creates and manages its own separate IDF virtual environment (accessed via get_python_exe() from get_idf_venv_dir()). This IDF venv is completely isolated from the penv setup, so different cryptography versions between penv_setup.py and espidf.py don't cause conflicts as they exist in separate virtual environments.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-09-23T14:15:00.476Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#0
File: :0-0
Timestamp: 2025-09-23T14:15:00.476Z
Learning: In the pioarduino/platform-espressif32 project, espidf.py creates an extra/separate virtual environment from the penv setup, so cryptography version differences between penv_setup.py and espidf.py do not cause conflicts since they operate in isolated environments.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
builder/frameworks/espidf.py
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#225
File: builder/main.py:99-108
Timestamp: 2025-07-13T20:03:29.695Z
Learning: In the ESP32 platform builder, the subprocess call to get site-packages directory in setup_python_paths() function is intentionally left without error handling because if this basic Python operation fails, it indicates a fundamental system issue that would cause the entire build process to fail anyway.
Applied to files:
builder/frameworks/espidf.py
🪛 Ruff (0.13.1)
builder/main.py
653-653: Unused function argument: target
(ARG001)
653-653: Unused function argument: source
(ARG001)
723-723: subprocess call: check for execution of untrusted input
(S603)
731-731: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (19)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (macos-15, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/espidf-blink)
- GitHub Check: build (macos-15, examples/arduino-NimBLE-ext_client)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (1)
builder/main.py (1)
693-705: Default coredump invocation fails without the required core file.Line 703 assembles
esp_coredumpwithout a--core/-cargument (and with the subcommand after--chip), sopio run -t coredumpcurrently exits immediately with “the following arguments are required: --core”. Please surface a usable default core file (config option or conventional filenames) and omit execution when none is found. This guarantees the documented target works out of the box.if cli_args: cmd.extend(cli_args) # ELF file should be at the end as positional argument if not any(arg.endswith('.elf') for arg in cli_args): cmd.append(elf_file) else: - # Default arguments if none provided - # Parameters from platformio.ini - extra_args = env.GetProjectOption("custom_esp_coredump_args", "") - if extra_args: - # Parse extra args and add ELF file at the end - cmd.extend(shlex.split(extra_args)) - cmd.append(elf_file) - else: - # Use defaults: --chip <mcu> info_corefile <elf_file> - cmd.extend(["--chip", mcu, "info_corefile", elf_file]) + extra_args = env.GetProjectOption("custom_esp_coredump_args", "") + if extra_args: + cmd.extend(shlex.split(extra_args)) + cmd.append(elf_file) + else: + core_path = env.GetProjectOption("custom_esp_coredump_corefile", "") + if not core_path: + default_bin = Path(get_project_dir()) / "coredump.bin" + default_b64 = Path(get_project_dir()) / "coredump.b64" + if default_bin.is_file(): + core_path = str(default_bin) + elif default_b64.is_file(): + core_path = str(default_b64) + if not core_path: + print( + "Error: No coredump file specified. Provide one via CLI " + "(-- --core <path>) or set custom_esp_coredump_corefile." + ) + return + core_args = ["info_corefile", "--chip", mcu, "--core", core_path] + if core_path.endswith(".b64"): + core_args.extend(["--core-format", "b64"]) + cmd.extend(core_args + [elf_file])
Removed unused ROM_ELFS_DIR and related environment variables.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
builder/main.py (5)
653-666: Silence ARG001 by marking SCons action params unused.Rename target/source to underscores to avoid “unused argument” warnings without changing behavior.
-def coredump_analysis(target, source, env): +def coredump_analysis(_target, _source, env):
687-704: Make default run robust: auto-pick core file if present, add port if known.Today the default path reads the coredump from flash (no --core), which fails when no device is attached or auto-detection picks the wrong one. Prefer a local core file when available, infer core-format from extension, and include --port if UPLOAD_PORT is set; still fall back to reading from flash if no file found. This matches esp-coredump’s CLI where is positional ELF and --core/-c is optional. (docs.espressif.com)
- else: - # Default arguments if none provided - # Parameters from platformio.ini - extra_args = env.GetProjectOption("custom_esp_coredump_args", "") - if extra_args: - # Parse extra args and add ELF file at the end - cmd.extend(shlex.split(extra_args)) - cmd.append(elf_file) - else: - # Use defaults: --chip <mcu> info_corefile <elf_file> - cmd.extend(["--chip", mcu, "info_corefile", elf_file]) + else: + # Default arguments if none provided + # Parameters from platformio.ini + extra_args = env.GetProjectOption("custom_esp_coredump_args", "") + if extra_args: + args = shlex.split(extra_args) + cmd.extend(args) + # Ensure ELF is last positional if not present + if not any(a.endswith(".elf") for a in args): + cmd.append(elf_file) + else: + # Prefer an explicit core file if configured or present; else read from flash + core_file = env.GetProjectOption("custom_esp_coredump_corefile", "") + if not core_file: + for name in ("coredump.bin", "coredump.b64"): + cand = Path(get_project_dir()) / name + if cand.is_file(): + core_file = str(cand) + break + + # Global options + cmd.extend(["--chip", mcu]) + upload_port = env.subst("$UPLOAD_PORT") + if upload_port: + cmd.extend(["--port", upload_port]) + + # Subcommand and arguments + cmd.append("info_corefile") + if core_file: + cmd.extend(["--core", core_file]) + if core_file.lower().endswith(".b64"): + cmd.extend(["--core-format", "b64"]) + # ELF is the required positional + cmd.append(elf_file)
737-742: Parse boolean project option correctly.PlatformIO options come in as strings; "false" currently evaluates truthy. Normalize to a real boolean.
- if env.GetProjectOption("custom_esp_coredump_verbose", False): + if str(env.GetProjectOption("custom_esp_coredump_verbose", "false")).lower() in ("1","true","yes","on"):
744-748: Add a timeout to prevent hanging runs.Long-running esp-coredump sessions can stall. Add a generous timeout (or make it configurable).
- result = subprocess.run(cmd, check=False, capture_output=False, env=coredump_env) + timeout_sec = int(str(env.GetProjectOption("custom_esp_coredump_timeout_sec", "600"))) + result = subprocess.run(cmd, check=False, capture_output=False, env=coredump_env, timeout=timeout_sec)
749-755: Narrow exception handling.Catching bare Exception obscures actionable errors. Limit to OSError/SubprocessError and print stderr when available.
- except Exception as e: - print(f"Error: Failed to run coredump analysis: {e}") + except (OSError, subprocess.SubprocessError) as e: + print(f"Error: Failed to run coredump analysis: {e}") print(f'Make sure esp-coredump is installed: uv pip install --python "{PYTHON_EXE}" esp-coredump')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
builder/main.py(2 hunks)builder/penv_setup.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T19:13:25.599Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
builder/penv_setup.py
🪛 Ruff (0.13.1)
builder/main.py
653-653: Unused function argument: target
(ARG001)
653-653: Unused function argument: source
(ARG001)
718-718: Do not catch blind exception: Exception
(BLE001)
727-727: Do not catch blind exception: Exception
(BLE001)
744-744: subprocess call: check for execution of untrusted input
(S603)
752-752: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (11)
- GitHub Check: build (macos-15, examples/espidf-blink)
- GitHub Check: build (macos-15, examples/arduino-wifiscan)
- GitHub Check: build (macos-15, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
🔇 Additional comments (3)
builder/main.py (2)
1038-1057: Targets look good.The coredump and coredump-only targets are registered as expected and align with the new analysis helper.
709-729: Remove unnecessary install_package guard
install_packageis provided byPlatformBase(inherited byEspressif32Platform), so it’s always available—no extra availability check needed.Likely an incorrect or invalid review comment.
builder/penv_setup.py (1)
58-60: Approve dependencies Verified esp-idf-size latest on PyPI is 1.7.1 and esp-coredump is 1.14.0; the>=1.6.1and>=1.14.0specifiers are valid.
Refactor coredump command construction to handle extra arguments and core file detection more robustly.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Removed '--ng' argument from esp_idf_size command.
Checklist:
Summary by CodeRabbit
New Features
Chores