Fix iram/irom section contents

[djphoenix]

Fixes #1562

Also fixes HW timer mode from NMI to FRC1 because NMI causes reboot

Verified that node and PWM module works properly, not checked another parts. Tests needed.

[devsaurus]

I also experienced that running hw_timer on NMI can yield unstable results, thus I'd confirm to change it to FRC1 for pwm and perf modules.

The linker magic is beyond my capabilities, though. Anybody feels like having a look?

[jmattsson]

I'm not really back on board yet, but... can this PR be summarised thusly?

• Switch pwm/perf timer from NMI to FRC (ok with me, the RTOS SDKs only expose the FRC anyway)
• Increase the irom section size to 832k. Probably okay, but we might want to consider introducing a post-build check to ensure the combined file size is <1meg. We'll spend lots of time chasing imaginary issues otherwise I suspect (having done just that on another platform in the past)
• Change the names of sections used in sections.h. Not sure what this gains us?
• Move all SDK rodata back into RAM. Am I reading that correctly? If so, no, that would not be a good idea. If recent SDK(s) have added constants that are access while the flash isn't mapped, those (and only those) should be moved into RAM. The whole reason we're using the exception handler to emulate byte-wise access to the mapped flash is so that we can keep constants out of our precious RAM.

[djphoenix]

@jmattsson

• Increase irom to 832k is safe because just math. Total image size limited to sum of sections. Also, because irom starts at 0x10000 (and nothing goes after irom), the safe size for irom is 0xF0000 (1M - start), and currently this is limited at 0xD0000.
• Names in sections.h are changed for just consistency, so I will revert them back if needed.
• Nope, there are another things:

1. Move .text before .irom
2. Leave in text only interrupt vectors, part of SDK libs (main, net80211, ...), and parts marked with TEXT_SECTION_ATTR.
3. Fix rodata contents, leave only SDK parts (as noted in pt.2), and also parts marked with RAM_CONST_SECTION_ATTR.
4. At the end, .irom0.text collects all of remaining parts of .text and .rodata, because they are (potentially) safe to place in flash-cache.

[jmattsson]

Ah, I was thinking that the SPIFFS part might overflow the meg boundary, but even if it does that would of course not be an issue.

Riiight, I'd missed that the .irom0.text was now hoovering up more of the SDK's .text symbols. I'm not near a build system, how much of the SDK have you managed to move into ROM, roughly? Do you have before/after figures?

You should be able to leave all the SDK's .rodata in ROM however, no need to hoist that back into RAM unless something has changed since ~1.4 when I did my analysis of that. And if there is something that needs to be in ROM, just move that, not all/most the SDKs rodata please.

[djphoenix]

As I see, SPIFFS location is calculating from top of irom (with align), so they're should never overlap. Correct me if I wrong.

So, there are no numbers, but before RAM-text was contain as much code as possible, so... there was problems. Now there are only main.a, net80211.a, phy.a, pp.a and gcc.a. So maybe something of them can be moved outside RAM, but main, net80211, phy and pp may be changed by espressif in any time (we don't have sources, right), so safer to keep them in RAM anyway. I do not know what of them really used before flash cache enabled.

And about rodata... I was verified that libs (listed above) using their rodata, and do not really know how and when them can access it. So, for upgrade safety, I prefer to leave them in RAM. Moving all of them into ROM may cause unstable crashes and more difficult issues.

[jmattsson]

Alright, I'm back at my workstation now and have run through this stuff. Apologies for the delay.

As-is, this PR costs us 112 bytes of RAM due to SDK constants being moved back into DRAM. They're currently sitting in ROM just fine; no rodata is being used by IRAM'd code (any constants used by such code are in .literal). You are correct that it would be safer/future-proof to have them DRAM in case the SDK changes behaviour, but since we're so tight on RAM I'd rather deal with it if it becomes a problem.

There is also no need to spell out which SDK libs to pull .text sections from. The SDK libs all differentiate between IRAM and IROM sections. NodeMCU builds with -ffunction-sections and doesn't put anything into .text unless explicitly requested. The only library we use which isn't "ROM-clean" is libc, which is already explicitly loaded into IROM. I'm happy to switch that logic around to what you have, but I'd much prefer to see *sdk/esp_iot_sdk_*lib/lib*.a:*(.literal .text) used to pull in the SDK libs in that case.

TL;DR: The below diff is all it takes to go from 43016 bytes of heap back to 43128 bytes:

diff --git i/ld/nodemcu.ld w/ld/nodemcu.ld
index cc5942b..303ac93 100644
--- i/ld/nodemcu.ld
+++ w/ld/nodemcu.ld
@@ -103,12 +103,7 @@ SECTIONS
     *(.init.literal)
     *(.init)

-    *libmain.a:*(.literal .text)
-    *libnet80211.a:*(.literal .text)
-    *libphy.a:*(.literal .text)
-    *libpp.a:*(.literal .text)
-    *libgcc.a:*(.literal .text)
-
+    *sdk/esp_iot_sdk_*lib/lib*.a:*(.literal .text)
     *(.iram.text .iram0.text)

     *(.stub .gnu.warning .gnu.linkonce.literal.* .gnu.linkonce.t.*.literal .gnu.linkonce.t.*)
@@ -141,12 +136,6 @@ SECTIONS
   {
     _rodata_start = ABSOLUTE(.);

-    *libmain.a:*(.rodata .rodata.*)
-    *libnet80211.a:*(.rodata .rodata.*)
-    *libphy.a:*(.rodata .rodata.*)
-    *libpp.a:*(.rodata .rodata.*)
-    *libgcc.a:*(.rodata .rodata.*)
-    
     *(.rodata.dram .rodata.dram*)

     *(.gnu.linkonce.r.*)

And to be technically correct: the flash cache gets disabled and enabled a lot during run-time, it's not just the window at start-up when the cache is off. Whenever flash is written or erased the cache gets turned off, and there may be other circumstances too. It's largely academic though - ensuring IRAM'd code doesn't have ROM dependency is key regardless.

[djphoenix]

Agree for .rodata, but not code-sections. I think many SDK libs can safely go to cache (so more iram left for performance-critical usercode), such as crypto/ssl/wpa/etc... Only main, pp, phy and gcc should leave in iram because them used at startup (and, so, in interrupts and other cache-disabled situations). So net80211 leaved in iram for better performance of wifi (no numbers again, but found some issues when it in cache).

[jmattsson]

That's what I'm trying to say - the SDK libraries already largely do load into IROM. If you xtensa-lx106-elf-objdump -t /path/to/sdk/lib/lib*.a you'll see that most functions are already placed in the .irom0.text section.

If you want to see which libraries actually have functions which go (and need to go) into IRAM, you can use e.g.

cd /path/to/sdk/lib;
for l in *.a; do echo === $l; xtensa-lx106-elf-objdump -t $l | grep .text|grep -v irom|grep F;done

(That also shows that libssl.a has one function which it needs to have in IROM btw. Some other libs do too, but we're not currently using those.)

The key point is that the SDK libs already do differentiate between IROM and IRAM code, and not honouring that is a quicker recipe for weirdness and crashes than the having the .rodata in ROM :) Placing SDK lib .text and .literal into IRAM is both needed and safe in the sense that it won't bloat the IRAM.

[djphoenix]

OK, back to work.

There is some numbers (for "all modules" build):

1. My config (only main, net80211, phy, pp, gcc in iram, same in rodata):

Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00007321  40100000  40100000  00000aec  2**2
  1 .data         0000096c  3ffe8000  3ffe8000  000000e0  2**4
  2 .rodata       000000a0  3ffe896c  3ffe896c  00000a4c  2**2
  3 .bss          00007200  3ffe8a10  3ffe8a10  00000af0  2**4
  4 .irom0.text   00084bb4  40210000  40210000  00008000  2**12

2. Nothing in rodata:

Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00007321  40100000  40100000  00000a7c  2**2
  1 .data         0000096c  3ffe8000  3ffe8000  000000e0  2**4
  2 .rodata       00000030  3ffe896c  3ffe896c  00000a4c  2**2
  3 .bss          00007200  3ffe89a0  3ffe89a0  00000a80  2**4
  4 .irom0.text   00084c24  40210000  40210000  00008000  2**12

112 bytes moved from rodata to irom. OK, it looks like it keeps to work.
3. Your patch for all SDK libs in iram:

Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00007379  40100000  40100000  00000a7c  2**2
  1 .data         0000096c  3ffe8000  3ffe8000  000000e0  2**4
  2 .rodata       00000030  3ffe896c  3ffe896c  00000a4c  2**2
  3 .bss          00007200  3ffe89a0  3ffe89a0  00000a80  2**4
  4 .irom0.text   00084bd4  40210000  40210000  00008000  2**12

text segment grows by 88 bytes. Not critical, but I'm not sure that we need.

Anyway I agree for rodata. Will apply your patch and push back. For text... So, you are maintainer, you have a choose how we will done this. Anyway it is better than before.

[djphoenix]

Pushed back the changed. Also tested that it works on sdk200 branch. Yay!

[jmattsson]

👍

@marcelstoer I see you removed the milestone, so I'll leave you to merge when you think it's suitable.