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

Disable non-IRAM ISRs while cache is disabled #3579

Closed
igrr opened this issue Sep 6, 2017 · 4 comments
Closed

Disable non-IRAM ISRs while cache is disabled #3579

igrr opened this issue Sep 6, 2017 · 4 comments

Comments

@igrr
Copy link
Member

igrr commented Sep 6, 2017

A lot of issues with interrupt handlers occur because interrupt handler code happens to be placed into cache. Cache is not accessible while flash is read/written. This causes illegal instruction exceptions which are hard to understand and debug for many users.

Investigate if it is possible to automatically disable interrupts which have ISRs not in IRAM, while flash cache is disabled (similar to the approach used in ESP32 SDK).

If not possible, as minimum, sanity-check all ISR handler function pointers and assert when function pointer is not in IRAM.

@devyte
Copy link
Collaborator

devyte commented Sep 9, 2017

@igrr
Could you please give more details about the mentioned approaches?
Also, exactly which "interrupt" handlers need to be in IRAM? pin ISRs? Ticker timer handlers? others?

I didn't realize until just now that having to mark the ISR with this attribute also means having to mark all functions in all code paths executed from within the ISR as well. I have been facing odd reboots every so often, and this could very well explain why. However, in my case I'm calling std container methods....

Every function that gets the ICACHE_RAM_ATTR reduces available mem. Instead of adding the ICACHE_RAM_ATTR tag to everything, why don't we instead schedule an os task from the ISR, which then calls the user callback? You could then do whatever you want from within the callback, except delays and yield, of course. The obvious downside is that there would likely be some overhead between the scheduling and the task execution, but I think this would save RAM and pretty much eliminate all crash cases.
Is this possible?

@igrr
Copy link
Member Author

igrr commented Sep 9, 2017

All interrupt handlers which can be called while flash access is in progress must be in IRAM (and all code paths called from these handlers). Ticker is not affected as it is dispatched from a scheduler task.

Dispatching interrupt callbacks from a task is possible, but as you mention, such callbacks would run only when the sketch code yields to the scheduler.

Another approach would be to mask interrupts before flash access starts and to unmask them when it is complete. In that case, ISRs would run as soon as flash access ends, even if the sketch does not yield. And interrupt handler code does not need to be IRAM in this case.

@devyte
Copy link
Collaborator

devyte commented Nov 14, 2018

I'd love to have this, maybe as a configurable option to choose between high latency/ease of use or low latency/IRAM use.
However, I think only @igrr knows what needs to be done, and right now there is no ETA.
Removing milestone.

@devyte devyte removed this from the 2.5.0 milestone Nov 14, 2018
@earlephilhower
Copy link
Collaborator

The current design punts on this and requires that all IRQ handlers to be in IRAM. The IRQ disable was removed from around flash ops (it was causing PWM hiccups and serial port overflows). The checks that @igrr were suggesting were a good first step, but ignore the functions that the IRQ handler might call (i.e. FP support, printf, etc.) since it's basically impossible to trace those.

Closing as a won't do, but @igrr is always free to repoen if he's unhappy with the decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants