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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esp-backtrace: Add `coredump` feature #2672

esp-backtrace: Add `coredump` feature #2672

bjoernQ commented Dec 4, 2024 •

edited

Loading

MabezDev left a comment

MabezDev Dec 4, 2024

bugadani Dec 4, 2024

bugadani Dec 4, 2024

MabezDev Dec 4, 2024

bjoernQ Dec 4, 2024

bugadani Dec 4, 2024

bjoernQ Dec 4, 2024 •

edited

Loading

MabezDev Dec 4, 2024

bjoernQ Dec 4, 2024

bjoernQ commented Dec 4, 2024

bjoernQ commented Dec 5, 2024

bjoernQ Dec 9, 2024

bjoernQ Dec 9, 2024

bjoernQ Dec 9, 2024

JurajSadel Dec 10, 2024

bjoernQ commented Dec 9, 2024

bjoernQ Dec 9, 2024

bugadani Dec 9, 2024 •

edited

Loading

esp-backtrace: Add coredump feature #2672

Are you sure you want to change the base?

esp-backtrace: Add coredump feature #2672

Conversation

bjoernQ commented Dec 4, 2024 • edited Loading

Thank you for your contribution!

Submission Checklist 📝

Extra:

Pull Request Details 📖

Description

Testing

MabezDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoernQ Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoernQ commented Dec 4, 2024

bjoernQ commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoernQ commented Dec 9, 2024

Choose a reason for hiding this comment

bugadani Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

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