Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix exception handler, add assert, reduce heap usage #4187

Merged
merged 9 commits into from
Mar 18, 2018

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Jan 17, 2018

---edit---
Move all exception strings to IRAM and out of both PMEM (illegal) and add
output of any assert() failinf conditions.

The exception handler may be called while the SPI interface is in a bad
state. This means no PROGMEM reads are allowed, and all data and functions
used must be in system RAM or IRAM.

Add a new helper macro, ets_printf_P(), which places a constant string in
IRAM and copies it to the stack before calling the real ets_printf().
This makes the code simpler to read as no unwieldy combinations of
ets_putc/ets_printf/... are required to output anything.

The old handler also mistakenly used PSTR() strings in some places, so
fix those with this patch as well.

Gives back ~180 bytes of heap to every sketch built as the exception handler
is always included an application.
---old---
Move the constant strings for stack dumps and exception/reset cause
from RODATA into PROGMEM and adjust the printout functions accordingly.
Gives back ~128 bytes of heap to every sketch built as these are always
included.

@igrr
Copy link
Member

igrr commented Jan 18, 2018

Is it assumed that the cache is not disabled when crash happens? What about Exception 0, which usually indicates a cache disabled error?

@earlephilhower
Copy link
Collaborator Author

Ah, crud.

One day I'm going to have to find a good reference manual for this chip. Doing the only exceptions I know how to generate on-demand (WDT, Load/Store) it seemed fine, but there are obviously others.

I don't think I ever saw a way for I$ to be disabled. If that's possible, then either this patch should be killed or the strings should move into IRAM (defining a new, local ISTR() macro with the proper section attribute).

That said, there are already PSTR()s in the existing routines (but for panics or WDT). Should they be undone?

@igrr
Copy link
Member

igrr commented Jan 18, 2018

One option is to check if the cache is disabled when entering the exception handler. If it is, stop SPI operation, if any, and re-enable the cache. That's a bit tricky however, and I don't have hardware at the moment to test the idea.
Will revisit next week.

@earlephilhower
Copy link
Collaborator Author

@igrr For the savings, I'm not sure it's worth the grief. Maybe just abandon this and then put in a new PR to remove the existing PSTR() uses?

Move all exception strings to IRAM and out of both PMEM (illegal) and add
output of any assert() failinf conditions.

The exception handler may be called while the SPI interface is in a bad
state.  This means no PROGMEM reads are allowed, and all data and functions
used must be in system RAM or IRAM.

Add a new helper macro, ets_printf_P(), which places a constant string in
IRAM and copies it to the stack before calling the real ets_printf().
This makes the code simpler to read as no unwieldy combinations of
ets_putc/ets_printf/... are required to output anything.

The old handler also mistakenly used PSTR() strings in some places, so
fix those with this patch as well.

Gives back ~180 bytes of heap to every sketch built as the exception handler
is always included an application.
@earlephilhower
Copy link
Collaborator Author

@igrr This is a redesign/combo of this #4187 and the ASSERT changes of #4066.

It removes any access to the SPI bus on an exception (there were quite a few spots), and instead makes sure everything needed to handle an exception is in IRAM. It also adds in the assert condition to an assert-fail exception output in a way that doesn't interfere with the ESP exception decoder. Overall, I think it's much cleaner than what was there, and it moves about 180 bytes out of the heap.

I'll update #4066 to only remove those assert warnings, and not (as a side effect, almost) touch assertion handling.

@earlephilhower earlephilhower changed the title Move exception strings to PROGMEM Fix exception handler, add assert, reduce heap usage Jan 19, 2018
@earlephilhower
Copy link
Collaborator Author

Latest merge dumps 'Assertion '0' failed' in the code for #4480, and prints prettier the actual assert condition in all cases, like
Panic /home/earle/Arduino/excp/excp.ino:43 unsigned char changeBuffer(unsigned int): Assertion 'dogs > cats' failed.

@earlephilhower earlephilhower requested a review from igrr March 9, 2018 05:24
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly an expert in the details of what happens post-mortem, but at first glance the code changes make sense, so approving.

@igrr igrr merged commit 855b03c into esp8266:master Mar 18, 2018
@earlephilhower earlephilhower deleted the exception_to_flash branch March 18, 2018 04:23
bryceschober pushed a commit to bryceschober/Arduino that referenced this pull request Apr 5, 2018
Move all exception strings to IRAM and out of both PMEM (illegal) and add
output of any assert() failinf conditions.

The exception handler may be called while the SPI interface is in a bad
state.  This means no PROGMEM reads are allowed, and all data and functions
used must be in system RAM or IRAM.

Add a new helper macro, ets_printf_P(), which places a constant string in
IRAM and copies it to the stack before calling the real ets_printf().
This makes the code simpler to read as no unwieldy combinations of
ets_putc/ets_printf/... are required to output anything.

The old handler also mistakenly used PSTR() strings in some places, so
fix those with this patch as well.

Gives back ~180 bytes of heap to every sketch built as the exception handler
is always included an application.

(cherry picked from commit 855b03c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants