feat(espcoredump): Include boot time and unix time in header (IDFGH-12792) #13770
Conversation
👋 Hello nebkat, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
github-actions
bot
changed the title
| @@ -146,6 +146,8 @@ typedef struct _core_dump_header_t { | |||
| uint32_t tcb_sz; /*!< Size of a TCB, in bytes */ | |||
| uint32_t mem_segs_num; /*!< Number of memory segments */ | |||
| uint32_t chip_rev; /*!< Chip revision */ | |||
| uint64_t boot_time; /*!< Boot time, in microseconds */ | |||
| uint64_t unix_time; /*!< Unix time, in microseconds */ | |||
There was a problem hiding this comment.
To avoid changing the header format (which requires changing the COREDUMP_VERSION...), could we keep this information in an ELF info segment? That way, the change would not cause compatibility issues even if an older version of esp-coredump package is used to decode the new core dump binary.
(Of course, this feature would be specific to the ELF format, but BIN format is present for backwards compatibility only, so not adding the feature there is okay.)
| #define COREDUMP_VERSION_BIN 0 | ||
| #define COREDUMP_VERSION_ELF 1 | ||
| #define COREDUMP_VERSION_BIN 1 | ||
| #define COREDUMP_VERSION_ELF 2 |
There was a problem hiding this comment.
That's not correct, unfortunately — this changes the definition of COREDUMP_VERSION_BIN_LEGACY a few lines below, meaning that the coredumps in legacy format won't be encoded correctly, and older versions of esp-coredump won't be able to read them.
| dump_hdr.boot_time = esp_timer_get_time(); | ||
|
|
||
| struct timeval tv; | ||
| gettimeofday(&tv, NULL); |
There was a problem hiding this comment.
I'm not sure it is safe to call gettimeofday from the panic handler. Internally, gettimeofday may attempt to acquire a mutex (e.g. here and a couple other places), which in general might not be possible in the panic handler. (Panic handler might be invoked from a high-priority ISR, such as an interrupt WDT ISR).
There was a problem hiding this comment.
Perhaps the best alternative I can suggest is to handle this at the application level.
You can designate a static variable with COREDUMP_DRAM_ATTR to make sure it gets saved in the coredump. Then, set up a periodic timer to save the gettimeofday output into that variable. If a crash happens, you will get the latest value of that variable inside the dump.
Unfinished and untested but creating draft to gather feedback. Will create corresponding patch for esp-coredump python.
Core dumps do not contain any form of timestamp to help identify when a crash occurred.
This patch adds two new fields to the core dump header - boot time and unix time: