flipperdevices / flipperzero-firmware

Flipper Zero firmware source code
https://flipperzero.one
GNU General Public License v3.0
12.99k stars 2.74k forks source link

Custom crash/assert messages for FAPs #3629

Open CookiePLMonster opened 7 months ago

CookiePLMonster commented 7 months ago

Describe the enhancement you're suggesting.

Description

The current implementations of furi_check, furi_assert and furi_crash allow the user to specify a custom crash message. This message is then displayed in serial logs, however, firmware-specified messages also come with an exclusive feature - they are stored in the backup register and displayed on reboot. Only a pointer is preserved, which means that for obvious reasons, this cannot be done for errors resided in the FAP.

We've been brainstorming this with @Willy-JL and @kbembedded on Discord and came up with an idea that could use some more eyes on - because maybe it's reasonable, or maybe it's not viable and cannot be accepted. Either way, I'd be curious to hear an opinion about it.

The idea would be to repurpose a few more backup registers (STM32WB55 has 20, Flipper currently uses 7) to store a short (12-16 characters) arbitrary error message, so it can display on screen regardless whether it comes from firmware or FAP.

Example

A revised "crash flow" accepting arbitrary 16-character error messages would then go as follows:

  1. Application calls furi_crash("I crashed! Help!");.
  2. In __furi_crash_implementation, a pointer range check is done as usual. If the error string pointer resides inside a firmware or is one of the in-built magic check/assert messages, set the top bit of that pointer. This will act as a marker for furi_hal_rtc_get_fault_data, signaling that FuriHalRtcRegisterFaultData stores a pointer and not a C string.
  3. If the string resides outside of the firmware, treat FuriHalRtcRegisterFaultData, FuriHalRtcRegisterFaultData2, FuriHalRtcRegisterFaultData3 and FuriHalRtcRegisterFaultData4 (added as part of this enhancement) as a char[16] buffer and strncpy the custom crash message into it. Since only ASCII is allowed, the top bit of FuriHalRtcRegisterFaultData remains cleared.
  4. On reboot, furi_hal_rtc_get_fault_data checks the top bit and returns a pointer to the in-firmware error string, or a pointer to the arbitrary message stored in the 4 backup registers. This might require the message to be copied out to a local static char[17] or something similar.

Benefits and risks

Pros:

  1. Arbitrary messages can aid FAP debugging by the clever use of formatted messages - for example, one could devise a short assertion message with a line number and (part) of the filename.
  2. FAPs can opt to show user-friendly crash messages for some critical points of failure.
  3. No API version bump needed.

Cons:

  1. Slightly increased code complexity.
  2. Increased usage of the backup registers.

Alternatives

If backup registers are out of the question, is there perhaps a different place to store the message in? A writable section of the flash that the firmware could strncpy the error message to? A file in the internal storage?

Anything else?

No response

skotopes commented 6 months ago

You don't need backup registers for that, RAM2 region can be preserved on reboot. To be honest I've been thinking about that long before faps arrived. It's kinda doable, but kinda useless. I'll accept PR if there will be any, but I'm not super happy with backup registers being used.

CookiePLMonster commented 6 months ago

You don't need backup registers for that, RAM2 region can be preserved on reboot.

Oh, that sounds great - any examples of that in the firmware?

It's kinda doable, but kinda useless.

I agree for those devs who have a devboard, as you can then just break on a specific assertion - but for a devboard-less development, unique assertion/check messages could be insanely helpful. And even if the dev has a devboard, their testers may not and so with unique error messages they could make better reports.

but I'm not super happy with backup registers being used.

Given the alternative of preserving a region of RAM2, I agree - I was just unaware this is possible at all.

skotopes commented 6 months ago

You don't need backup registers for that, RAM2 region can be preserved on reboot.

Oh, that sounds great - any examples of that in the firmware?

It's kinda doable, but kinda useless.

I agree for those devs who have a devboard, as you can then just break on a specific assertion - but for a devboard-less development, unique assertion/check messages could be insanely helpful. And even if the dev has a devboard, their testers may not and so with unique error messages they could make better reports.

but I'm not super happy with backup registers being used.

Given the alternative of preserving a region of RAM2, I agree - I was just unaware this is possible at all.

There are no examples and I think some research need: there were some option bytes that were controlling it.

CookiePLMonster commented 6 months ago

Either way this sounds very promising! Worth checking if maybe RAM2 is already not clearing on reboot, and realistically assertions/checks don't need to survive a power cycle (which backup registers do, if I understand correctly).

That said, with the current setup of stashing a pointer inside the backup register, assertions should survive a power cycle, but with this change, they wouldn't. I guess it makes sense to just check if the message comes from RAM2 or firmware flash, and only display it if the backed up pointer is from the flash.