esp-backtrace: Add `coredump` feature #2672

bjoernQ · 2024-12-04T12:53:50Z

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

I have updated existing examples or added new ones (if applicable).
I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
My changes were added to the CHANGELOG.md in the proper section.
I have added necessary changes to user code to the Migration Guide.
My changes are in accordance to the esp-rs API guidelines

Extra:

I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Adds the features coredump and coredump-all which will make the panic handler to raise an exception (via ecall/syscall) and changes the exception handler to create a coredump.

Testing

Add the coredump feature to esp-backtrace in examples, change one of the examples to panic.

You can use https://github.com/bjoernQ/espflash-coredump and a recent espfalsh commit (or copy paste, convert from hex to binary)

MabezDev

Just a first round of review, I think I'll have some more thoughts later.

Generally looks good! I'm just wondering whether all the coredump stuff belongs inside esp-backtrace, it looks fairly generic. Maybe it might be better to split it out?

MabezDev · 2024-12-04T15:00:57Z

esp-backtrace/src/lib.rs

-    println!("");
-    println!("");
-    println!("");
+    println!("{}", defmt::Display2Format(info));


We shouldn't do this unless we really have to, it pulls in core::fmt machinery

This isn't new, just the diff is weird.

https://docs.rs/defmt/0.3.10/defmt/trait.Format.html#impl-Format-for-PanicInfo%3C'_%3E

MabezDev · 2024-12-04T15:05:53Z

esp-backtrace/src/coredump.rs

+    writer.writer.write(&reg_info_bytes);
+}
+
+pub trait Writer {


Maybe we should use embedded-io::Write here instead of defining our own trait? Maybe it makes dumping the core dump through some other means other than serial possible.

Was thinking if this is a good idea - only reason to have this trait is the dependency count but I think embedded-io doesn't pull transitive dependencies

unstable esp-hal already pulls in embedded-io

👍 true - I guess my concerns about increasing build times is not a real world problem in general

MabezDev · 2024-12-04T15:06:43Z

esp-backtrace/src/lib.rs

@@ -11,6 +11,30 @@ use esp_println as _;

 const MAX_BACKTRACE_ADDRESSES: usize = 10;

+#[cfg(feature = "esp32")]
+const RAM: (u32, u32) = (0x3FFA_E000, 0x4000_0000);


Maybe we should define these in esp-metadata, and use them from there. We also have a similar set of const's in the soc module of esp-hal

That's probably a good thing - also depending on esp-metadata is not too bad (in general I'd like to not have too much dependencies here)

bjoernQ · 2024-12-04T16:39:47Z

I'm just wondering whether all the coredump stuff belongs inside esp-backtrace, it looks fairly generic. Maybe it might be better to split it out?

t.b.h. I implemented that functionality in its own library initially (just had features instead of target_arch) and copied the code

I was a bit concerned about a bump in build time (same reason for not using embedded-io and esp-metadata - ok I didn't even think about esp-metadata before :) )

bjoernQ · 2024-12-05T09:04:46Z

I think extracting the coredump generation into its own crate makes sense but where should it live? In the esp-hal repo as esp-coredump? I guess, yes? I don't expect a lot of churn there so it shouldn't be too much of an additional burden. But I can also have it in my personal GitHub space

bjoernQ · 2024-12-09T13:41:08Z

esp-hal/src/soc/mod.rs

@@ -37,6 +37,11 @@ pub(crate) fn psram_range() -> Range<usize> {
    }
 }

+/// The lower bound of the system's DRAM (Data RAM) address space.
+const SOC_DRAM_LOW: usize = esp_config::esp_config_int!(usize, "REGION-DRAM-START");


I think (ab)using the esp-config macro should be fine

bjoernQ · 2024-12-09T13:41:59Z

esp-metadata/devices/esp32.toml

@@ -75,3 +75,5 @@ symbols = [
    "pm_support_touch_sensor_wakeup",
    "ulp_supported",
 ]
+
+memory = [{ name = "dram", start = 0x3FFA_E000, end = 0x4000_0000 }]


we can and should define all memory regions here later and even use them in e.g. the linker scripts

(But that is definitely out of scope for this PR)

bjoernQ · 2024-12-09T13:43:22Z

esp-hal/src/soc/esp32c6/mod.rs

-    pub const SOC_DRAM_LOW: usize = 0x4080_0000;
-    /// The upper address boundary for system DRAM.
-    pub const SOC_DRAM_HIGH: usize = 0x4088_0000;
+    /// The lower bound of the system's DRAM (Data RAM) address space.


maybe all of these constants should come from esp-metadata some day

I like the idea 👍

bjoernQ · 2024-12-09T13:44:53Z

skip-changelog because of the not-user-facing changes in esp-hal

bjoernQ · 2024-12-09T13:45:14Z

esp-backtrace/Cargo.toml

@@ -2,7 +2,7 @@
 name         = "esp-backtrace"
 version      = "0.14.2"
 edition      = "2021"
-rust-version = "1.76.0"
+rust-version = "1.79.0"


needed by esp-metadata

bugadani · 2024-12-09T13:53:07Z

esp-metadata/src/lib.rs

+
+        // Define env-vars for all memory regions
+        for memory in self.memory() {
+            println!(


Should we also generate has_<memory_region>_region config symbols?

esp-metadata/src/lib.rs

bjoernQ · 2025-03-27T07:27:29Z

The definition of memory regions via esp-metadata is filed separately in #3300

Given there is no progress here and since I had "a few days" to think about this PR, I'm considering closing this

I'm considering creating an independent panic/exception handler crate providing this functionality or extending the functionality here via the callback mechanisms

I have a few more ideas which would broaden the scope of this crate which (after thinking about it) shouldn't be done here.

esp-backtrace should focus on the things it's meant to do

Converted to DRAFT now and going to close this in a few days

bugadani · 2025-03-27T08:29:29Z

esp-backtrace should focus on the things it's meant to do

Do you intend to remove the panic/exception handling parts of esp-backtrace? We could probably turn unhandled exceptions into panics by either the runtime crates or esp-hal, and build up from there - it wouldn't be a bad idea IMHO.

bjoernQ · 2025-03-27T08:36:20Z

Do you intend to remove the panic/exception handling parts of esp-backtrace? We could probably turn unhandled exceptions into panics by either the runtime crates or esp-hal, and build up from there - it wouldn't be a bad idea IMHO.

Haven't really thought about it t.b.h. - but "turn unhandled exceptions into panics by either the runtime crates or esp-hal" (I'd vote for esp-hal) sounds like an interesting idea

bjoernQ · 2025-03-27T16:28:07Z

Converted to DRAFT now and going to close this in a few days

Waiting for #3300 - then I'll close this

bjoernQ · 2025-04-03T09:03:21Z

Closing

There is a stand-alone panic/exception handler here: https://github.com/bjoernQ/esp-coredump

bjoernQ marked this pull request as ready for review December 4, 2024 13:12

MabezDev requested changes Dec 4, 2024

View reviewed changes

bjoernQ force-pushed the esp-backtrace/coredump branch from 61817c6 to 3661c21 Compare December 9, 2024 08:44

bjoernQ commented Dec 9, 2024

View reviewed changes

bjoernQ added the skip-changelog No changelog modification needed label Dec 9, 2024

bjoernQ commented Dec 9, 2024

View reviewed changes

bugadani reviewed Dec 9, 2024

View reviewed changes

esp-metadata/src/lib.rs Show resolved Hide resolved

bjoernQ added 11 commits December 18, 2024 07:54

Minor refactoring

6b5bb56

coredump for RV and Xtensa

4b826a7

Fix warnings

17827e0

Minor cleanup

899482d

Add coredump-all

d82d028

CHANGELOG.md

58aea9c

Use embedded-io

ae947c3

Update defmt

63b0b99

Define DRAM region in esp-metadata

9bd6a42

FMT

321a5bd

Define has_<memory_region>_region for memory regions

ac4b839

bjoernQ force-pushed the esp-backtrace/coredump branch from c13c544 to ac4b839 Compare December 18, 2024 07:06

bjoernQ mentioned this pull request Mar 26, 2025

Define DRAM memory region in esp-metadata #3300

Merged

6 tasks

bjoernQ marked this pull request as draft March 27, 2025 07:18

bjoernQ closed this Apr 3, 2025

esp-backtrace: Add coredump feature #2672

esp-backtrace: Add coredump feature #2672

Uh oh!

Conversation

bjoernQ commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

Uh oh!

MabezDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernQ Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernQ commented Dec 4, 2024

Uh oh!

bjoernQ commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjoernQ commented Dec 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bugadani Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bjoernQ commented Mar 27, 2025

Uh oh!

bugadani commented Mar 27, 2025

Uh oh!

bjoernQ commented Mar 27, 2025

Uh oh!

bjoernQ commented Mar 27, 2025

Uh oh!

bjoernQ commented Apr 3, 2025

Uh oh!

Uh oh!

esp-backtrace: Add `coredump` feature #2672

esp-backtrace: Add `coredump` feature #2672

bjoernQ commented Dec 4, 2024 •

edited

Loading

bjoernQ Dec 4, 2024 •

edited

Loading

bugadani Dec 9, 2024 •

edited

Loading