Leave only some parts of SDK in iram #1710
Conversation
| *(.init.literal) | ||
| *(.init) | ||
|
|
||
| *sdk/esp_iot_sdk_*lib/lib*.a:*(.literal .text) |
There was a problem hiding this comment.
Could I request a comment here stating that we're intentionally excluding libwps.a even though it claims to have iram'd functions?
| *libnet80211.a:*(.literal .text) | ||
| *libphy.a:*(.literal .text) | ||
| *libpp.a:*(.literal .text) | ||
| *libgcc.a:*(.literal .text) |
There was a problem hiding this comment.
I don't think this list is complete. Have a look at the output of this:
nodemcu-firmware/sdk/esp_iot_sdk_v2.0.0/lib$ xtensa-lx106-elf-objdump -h *.a|grep -v '2\*\*0' | grep -v irom|grep -E 'format|.text|archive'
It seems the following libs also have some things in IRAM:
libat.a(don't care, we don't use this)- libcrypto.a
- libdriver.a (not sure if we're using any of this by now?)
- libespnow.a (not relevant yet, but might be worth listing here anyway)
- libmesh.a (as above)
liblwip_536.a(crikey, espconn_buf.o has nearly 1.3k in IRAM! Lots of other bits too...)libpwm.a(we have our own pwm driver last I checked)- libwpa.a
libwps.a(intentionally excluded)
There was a problem hiding this comment.
This list is incomplete for "all libs that have something in iRAM" but complete for "all libs that should have something in iRAM".
All libs that you listed above is called in flash-cache-safe ways. Originally in #1566 I was tested firmware carefully for hashing, WPA/WEP connections, and more ways, and doesn't found any issues with it. But later we choose the way to keep all SDK libs in iRAM.
Rationale of my way is to keep iRAM as clean as possible. Sure, originally Espressif wasn't just include "safe unaligned access" patch in core library, so I think there was something similar in their internal testing process (if it was generally), so after tons of changes, them kept listed libs in iRAM just for historical reasons.
There was a problem hiding this comment.
Ah, sorry I forgot the testing you did back on that PR. And yeah, I'm not against kicking things out of IRAM if they don't have to be there, I'd just like it to be a very conscious decision, with a note about each lib we've done that to left in there.
Regarding unaligned accesses though, the constraints are the same for mapped flash and IRAM, but it's all a bit tangential to the main discussion :)
There was a problem hiding this comment.
So okay, I was added a note about it. I think this is enough to go.
Also I propose to include SDK upgrade guide (to check iRAM/iROM contents) in contributing.md
|
Did you want Marcel or Arnim to have a look over this first, or are you happy for me to merge? And did you want to get the SDK upgrade not in here too? |
|
I think one review is enough, we have roughly tested this before. |
Me? I have absolutely zero clue what you guys are doing here 😄 I'm just happy you fixed it - whatever that means. |
Amendment for #1562/#1566, follow-up #1694 comment.
As we decided, for example,
libwpsis safe to put it intoirom(although Espressif marked some parts as "iram-forced"). And we cannot fit it intoiramanyway (becauseiramsegment size overflow).So I bring back my original proposal to keep only some parts of SDK in iram. For now this is
main,net80211,phy,ppandgcclibs (they're used in bootup process, in interrupts, and in other ways where iROM is unavailable). For another parts it is safe to put them in flash and free some instruction RAM.