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

Evaluate need for the IRAM_ATTR attribute #328

Closed
josesimoes opened this issue May 25, 2018 · 6 comments · Fixed by nanoframework/nf-interpreter#718
Closed

Evaluate need for the IRAM_ATTR attribute #328

josesimoes opened this issue May 25, 2018 · 6 comments · Fixed by nanoframework/nf-interpreter#718

Comments

@josesimoes
Copy link
Member

josesimoes commented May 25, 2018

There are a number of functions decorated with this attribute.
According to ESP32 documentation this is only required for debugging and/or for specific ISR handlers.
Adding to this a function with this attribute seems to have a high negative impact in the code performance.

@AdrianSoundy, @MatthiasJentsch care to comment/act? 😉

@josesimoes josesimoes added this to the Initial Preview milestone May 25, 2018
@josesimoes josesimoes changed the title Evaluate need of IRAM_ATTR attribute Evaluate need for the IRAM_ATTR attribute May 25, 2018
@MatthiasJentsch
Copy link
Contributor

I'm not sure why these functions have the IRAM_ATTR. But I can take a look at these functions next week. Maybe @AdrianSoundy have a hint why they are decorated with IRAM_ATTR.

@MatthiasJentsch
Copy link
Contributor

I've removed all IRAM_ATTR and my simple blinky app is still working. So I've really no idea why 18 functions are decorated with IRAM_ATTR. I think we can remove all these attributes at least for production code. Maybe it helps with OpenOCD/JTAG debugging if some or all of these functions have the IRAM_ATTR set. http://esp-idf.readthedocs.io/en/latest/api-guides/jtag-debugging/index.html

@josesimoes
Copy link
Member Author

It sure is a requirement for debug. But it looks like that's required for a few other situation, like some ISR.

@MatthiasJentsch
Copy link
Contributor

For the debugging stuff you can use the two hardware breakpoints. You don't need the IRAM_ATTR. All the 18 functions that are decorated with IRAM_ATTR are not ISR's. I detail we have:

  • nanoBooter / main.c / main_task - not an ISR
  • nanoCLR / app_main.c / main_task and receiver_task - both not ISR's
  • 3 HRESULT functions in nanoCLR / CLRStartup.cpp - also not ISR's
  • 12 Library function - also not ISR's

Again, I think we can remove all the IRAM_ATTR attributes. I can't think a situation where we need it.

@josesimoes
Copy link
Member Author

So I guess we all in agreement that those can go away. @AdrianSoundy?

@josesimoes
Copy link
Member Author

Fixed with nanoframework/nf-interpreter#718.

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

Successfully merging a pull request may close this issue.

2 participants