Adds core dump support via CrashCatcher #52
Conversation
salkinium
force-pushed
the
feature/crash_catcher
branch
2 times, most recently
from
79bbfc8 to
002aec5
Compare
| @@ -15,7 +15,7 @@ import os | |||
|
|
|||
| def init(module): | |||
| module.parent = "platform" | |||
| module.name = "fault.cortex" | |||
| module.name = "fault.minimal" | |||
There was a problem hiding this comment.
Is there a reason for renaming this from cortex to minimal?
This module is completely ARM Cortex-M specific.
There was a problem hiding this comment.
So is fault.coredump. I'd prefer to not encode the device id into the module name, since they themselves decide which devices to support. Maybe with a different platform the fault.minimal module could be implemented differently (Cortex-A, RiscV, Xtensa) but with the same functionality.
There was a problem hiding this comment.
Btw, this is how all of modm:platform:** modules work, the modm:platform:gpio module for AVRs implements the same functionality as the one for STM32, but they don't conflict, since only one is enabled per platform.
|
Fault handler use cases:
Always useful: optional user callback to set the device into a safe configuration. I cannot see a usecase for the "wait" mode right now. |
| const uint16_t *pMemory = (const uint16_t *) pvMemory; | ||
| for (size_t ii = 1; ii <= elementCount; ii++) | ||
| { | ||
| uint16_t val = *pMemory++; |
There was a problem hiding this comment.
To be pedantic, dereferencing pMemory here is undefined behaviour. According to language rules commonly referred to as "strict aliasing rules" pMemory has to point to an actual (u)int16_t object to be safe to dereference (https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing). Although every sensible compiler will probably generate the expected code, this pitfall is easy to avoid here. There is an exception to these rules for (unsigned) char and std::byte which may alias any type. If I am not overlooking something, it is possible to define pMemory as uint8_t* and to pass it to dumpMemory directly.
There was a problem hiding this comment.
If I am not overlooking something, it is possible to define pMemory as uint8_t* and to pass it to dumpMemory directly.
Some memories can only be read with 16- or 32-bit accesses (mostly the peripherals), hence this annoyingly duplicate code. I can't change the const void *pvMemory, since that's the callback API from CrashCatcher, but how would you do this correctly?
There was a problem hiding this comment.
how would you do this correctly?
To me it does not make sense at all, to apply these rules to memory-mapped IO. The rules deal with accessing C++ objects in memory through a pointer of a different type. Applying the concept of a C++ object to some piece of hardware is totally nonsensical. There is no existing C++ object in memory that the compiler knows of. Hence, the only practical solution is to leave the code as it is.
However I don't think there is any guarantee that the intended 16-bit access will actually be performed as one. I haven't looked much into the assembly output of arm compilers but on modern x86 platforms simple byte copying operations in loops often turn into fancy AVX instructions.
Using a pointer to volatile for the read would clearly state to the compiler: I want a 16 bit read and I want it now. I suppose adding volatile would not impede wanted optimizations. As the pointer itself is not volatile but the data it points to, operations on the pointer will still be optimized. I compiled a few toy examples for cortex-m4 with -Os and the resulting assembly looked almost identical regardless of whether volatile is added or not.
There was a problem hiding this comment.
Good observation!
Another way to get around UB could be to add a read16 function coded in assembly that includes the correct load instruction.
There was a problem hiding this comment.
Added it to the TODO list in the new PR.
This allows for easier sharing of common settings between different build script generators.
This allows specifying common options like project name and build path together and not having to duplicate them for both generators.
Adds these behaviors on hard fault: - reset device. - force breakpoint. - loop forever. - flash LED every 1s. Removes switching to process stack, so on stack overflow, only the first three behaviors will work.
Selecting all modules through :platform:** is not possible anymore with the introduction of conflicting :platform:fault.* modules.
salkinium
force-pushed
the
feature/crash_catcher
branch
from
002aec5 to
3770838
Compare
salkinium
force-pushed
the
develop
branch
4 times, most recently
from
3da698d to
01092cf
Compare
|
Closed in favor of #210. |
This replaces the xpcc UART debug functionality on HardFault with something way more useful: post-mortem debugging via CrashDebug.
This relatively simple application just uses CrashCatcher's
HexDumpbehavior to dump all RAM via UART, which can take a lot of time, since the memories are quite large and UART quite slow at 115200 Baud.Since the dump can take a long time, I would implement a custom dump process similar to HexDump, but with a optional user callback to set the device into a safe configuration.
cc @rleh @strongly-typed