Make unique sections for ICACHE_* variables #5117

jjsuwa · 2018-09-10T14:25:15Z

Similar to PROGMEM changes, make the name of each ICACHE_* variable/fuction
unique to avoid issues with section conflicts.

Fixes #5115

Added .irom0.text.* and .iram.text.* to ldscript.

devyte · 2018-09-10T14:39:38Z

@jjsuwa how is this different from #5116?

earlephilhower · 2018-09-10T14:42:45Z

I think it's the same, now. It seems he committed the fix he suggested and re submitted the patch as a new one. He did find it, so I don't mind if you merge this and drop mine. It would have been a bear to find if he didn't! I'm on my phone so can't double check the PR, so please use due diligence...

jjsuwa · 2018-09-10T14:43:52Z

@jjsuwa how is this different from #5116?

ldscript.

In #5116, ICACHE_FLASH_ATTRed items are all matched by .irom0.text.* but not
.irom0.text, and ICACHE_RAM_ATTR's same.

earlephilhower

Good catch on the linker script.

Similar to PROGMEM changes, make the name of each ICACHE_* variable/fuction unique to avoid issues with section conflicts. Fixes #5115

wemos · 2018-09-12T08:11:44Z

@jjsuwa Could you test it and upload compiled code to board?
I test on simple "Blink", the code compiled successfully, but upload to esp8266 board, it can't work.

It work well on previous commit 9151930

I think it may be some wrong with this commit.

jjsuwa · 2018-09-12T09:02:43Z

@wemos Could you test it and upload compiled code to board?

board: ESP-WROOM-02 breakout PCB (Akizuki AE-ESP-WROOM-02)
commit: 622569c

#include <Ticker.h>

Ticker ticker;  // uses ICACHE_RAM_ATTR internally

// ICACHE_FLASH_ATTR... used by the framework everywhere

#define TICK_FREQUENCY 10

void setup() {
  pinMode(LED_BUILTIN, OUTPUT);
  digitalWrite(LED_BUILTIN, LOW);
  Serial.begin(115200);
  delay(250);
  Serial.print(F("\n\ntest sketch...\n"));  // uses ICACHE_RODATA_ATTR
  ticker.attach_ms((1000 / 2) / TICK_FREQUENCY, []() {
    static int state = 0;
    digitalWrite(LED_BUILTIN, state ^= 1);
  });
}

void loop() {
}

The above seems to work properly (IO2 pin generates 10Hz).

OK, more complicated tasks...

#include <ESP8266WiFi.h>
#include <ESP8266mDNS.h>
#include <ESP8266WebServer.h>

#define WIFI_SSID     "your-ssid"
#define WIFI_PASSWORD "********"
#define MDNS_HOSTNAME "avahi"

static ESP8266WebServer webServer(80);

void setup() {
  Serial.begin(115200);
  delay(250);
  Serial.print(F("\n\n"));
  WiFi.mode(WIFI_STA);
  WiFi.begin(String(F(WIFI_SSID)).c_str(), String(F(WIFI_PASSWORD)).c_str());
  for (; ; delay(500)) {
    if (WiFi.status() == WL_CONNECTED)
      break;
    Serial.print('.');
  }
  MDNS.begin(String(F(MDNS_HOSTNAME)).c_str());
  MDNS.addService(F("http"), F("tcp"), 80);
  webServer.onNotFound([]() {
    webServer.send(404);
  });
  webServer.on(F("/"), HTTP_GET, []() {
    webServer.sendHeader(F("Location"), String(F("http://" MDNS_HOSTNAME ".local/index.html")));
    webServer.send(302);
  });
  webServer.on(F("/index.html"), HTTP_GET, []() {
    webServer.send(200, F("text/html"), F("<!DOCTYPE html><html>"
                                          "<head><meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\"/><title>WiFi HTTP server test</title></head>"
                                          "<body><h1>WiFi HTTP server test</h1><p>hello world</p></body>"
                                          "</html>"));
  });
  webServer.begin();
}

#include <cont.h>

class Continuation {
  private:
    cont_t _context;
    inline bool _isReturned() {
      // belonging to the implementation of 'cores/esp8266/cont.S',
      // and might be changed in the future.
      return _context.pc_ret == 0 && _context.pc_yield == 0;
    }
  public:
    Continuation() {
      cont_init(&_context);
    }
    ~Continuation() {
      if (!_isReturned()) {
        DEBUGV("%s(): yielded but not returned yet\n", __func__);
      }
    }
    bool run(void (* const pfn)()) {
      if (!pfn) {
        return false;
      }
      cont_run(&_context, pfn);
      if (cont_check(&_context)) {  // stack frame corruption
        panic();
      }
      if (_isReturned()) {  // pfn() has been returned normally
        cont_init(&_context);  // re-init cont_t due to convenience
        return false;
      }
      return true;
    }
    void yield() {
      cont_yield(&_context);
    }
    static uint32_t getDeadlineMicroseconds(unsigned int const usec) {
      return ESP.getCycleCount() + ESP.getCpuFreqMHz() * usec;
    }
    static uint32_t getDeadline(unsigned int const msec) {
      return getDeadlineMicroseconds(msec * 1000U);
    }
    bool yieldUntil(uint32_t const deadline) {
      if ((int32_t)(deadline - ESP.getCycleCount()) > 0) {
        this->yield();
        return true;
      }
      return false;
    }
    void delayMicroseconds(unsigned int const usec) {
      uint32_t const deadline = getDeadlineMicroseconds(usec);
      do {
      } while (yieldUntil(deadline));
    }
    void delay(unsigned int const msec) {
      delayMicroseconds(msec * 1000U);
    }
};

static Continuation context;

static void continuationDemoFunc() {
  Serial.print(F("** enter continuationDemoFunc() **\n"));
  for (int i = 0; i < 50; ) {
    Serial.print(F("some "));
    context.delay(50);
    Serial.printf_P(PSTR("work #%d "), i);
    context.delay(50);
    Serial.print(F("in progress... "));
    context.delay(333);
    if ((++i & 7) == 0) {
      Serial.print('\n');
    }
  }
  Serial.print(F("\n** leave continuationDemoFunc() **\n"));
  // returned normally...
}

void loop() {
  // in case of continuous cyclic processing, can omit retval checking
  // (normal return implies re-init self internally)
  context.run(continuationDemoFunc);

  // place more other codes if you want
  webServer.handleClient();
}

bulky (200+ per second) HTTP requests served via WiFi,
mDNS responder,
another 'continuation' pseudo-thread demo (see serial log)

All seem to be fine.

With the changes in PR esp8266#5117 , blink and other examples compile but the 8266 gets stuck in a reset loop. Undo the changes from that commit.

earlephilhower · 2018-09-12T15:18:08Z

@jjsuwa , @wemos , @devyte - I can reproduce the failure @wemos shows. With this change the blink.ino example fails to work, as well as some BearSSL tests I've tried, too.

There is some kind of interaction with the linker change and the way the final BIN gets generated and you end up in an infinite reset loop.

Until that's figured out, I say we remove these changes since it's breaking things. I've put in a PR #5130 that reverses this.

When we figure it out, we can re-apply with whatever add'l fixes needed. My guess is the image layout got moved and the init code is not ending up at the right spot (since link is clean it's not like we're missing the function...it's just not where it should be).

With the changes in PR #5117, blink and other examples compile but the 8266 gets stuck in a reset loop. Undo the changes from that commit until we can figure out the root cause and fix it.

jjsuwa · 2018-09-12T15:58:41Z

@earlephilhower - I can reproduce the failure @wemos shows. With this change the blink.ino example fails to work,

But i can't reprod on libraries\esp8266\examples\Blink\Blink.ino... mysterious.

commit: 622569c
IDE settings: Generic ESP8266 Module, 160 MHz, Flash, nodemcu, 26 MHz, 80 MHz, QIO, 4M(1M SPIFFS), 2, v2 Lower Memory, Disabled, NoAssert-NDEBUG, Only Sketch, 921600

earlephilhower · 2018-09-12T16:07:33Z

I believe you, @jjsuwa , but it seems like for other configurations of debug options/modules/etc. it fails. Need to get to the bottom of it and add whatever add'l tweaks are required to make it stable on all settings.

I'm running D1Mini, 4M no spiffs, Debug port=Serial, Debug level=all, lwip2lomem, vtables=flash, 80mhz.

It may be related to debugging, then. Can you try your config w/debug port=serial, debug=all?

jjsuwa · 2018-09-12T16:16:36Z

@earlephilhower - Can you try your config w/debug port=serial, debug=all?

4M(no SPIFFS)
SSL+TLS_MEM+HTTP_CLIENT+HTTP_SERVER+CORE+WIFI+HTTP_UPDATE+UPDATER+OTA+OOM
Serial

but no difference (Blink.ino runs properly), hmm...

jjsuwa · 2018-09-12T16:42:00Z

fact:

Both .irom0.text* and .iram.text* affect code arrangement but .irom.text* not (const data).

hypothesis:

Distance from .irom0.text.* to other .irom*s (or text*s) may be changed worse (possibly too far)

proposal experiment:

apply the below to 622569c

-     *(.irom0.literal .irom.literal .irom.text.literal .irom0.text .irom0.text.* .irom.text .irom.text.*)
+     *(.irom0.literal .irom.literal .irom.text.literal .irom0.text.* .irom0.text .irom.text .irom.text.*)

-    *.cpp.o(.iram.text .iram.text.*)
+    *.cpp.o(.iram.text.* .iram.text)

-    *.c.o(.iram.text .iram.text.*)
+    *.c.o(.iram.text.* .iram.text)

earlephilhower · 2018-09-12T16:43:43Z

Prior WiFi state may also be part of the issue. My prior testing tests executed WiFi.begin(bad ssid, bad pass); and I can see in debug logs it's trying to reconnect (and failing), even in the silly blink.ino sketch. That's one thing that's not reset by upload or code changes..."hidden state."

earlephilhower · 2018-09-12T16:48:05Z

Looks like you posted while I was typing, didn't see it. Let me try your changes and see. The linker should fix up any short branches that break. Or at least give an error. But maybe not...

jjsuwa · 2018-09-12T16:50:34Z

@earlephilhower - That's one thing that's not reset by upload or code changes..."hidden state."

OK, Erase Flash: "All Flash Contents"... but works fine. ;)

earlephilhower · 2018-09-12T17:03:52Z

No joy yet.

Tools->Full flash erase: Same failure
Change in order of linker patterns: Same failure

The only difference I think is left is the generic vs. WemosD1 variant. I'll need to dig into the variant code a bit later.

jjsuwa · 2018-09-12T17:05:55Z

Oh, Windows Update bashes me... time to shut my PC down :)

conclusion:

These results suggest there is a hidden/potential problem with ICACHE_* link mechanism, I guess.

earlephilhower · 2018-09-13T00:17:35Z

I rebuilt and tried blink by swapping out the ICACHE_* macros one by one. ICACHE_RODATA_ATTR can be uniqified, but uniquifying either the _RAM_ or _FLASH_ cause the crash errors.

jjsuwa · 2018-09-13T06:11:21Z

Interesting...

In my envs, uniqified three ICACHE_s w/o .irom0.text.* and .iram.text.* (such as #5116) cause infinite reset loop. That is why I committed #5117.

earlephilhower · 2018-09-13T14:56:54Z

I actually just used the #defines in the PR to test (ie I just replaced the original section attribute with these new unique names one by one).

earlephilhower · 2018-09-18T20:26:51Z

Just found the problem here, @jjsuwa. There was a generated eagle.app.v6.common.ld file present in my tools/sdk/ld directory because my Arduino core was installed prior to #4618 .

@d-a-v, you git-removed the "eagle.app.v6.common.ld" but it seems like @wemos and I and others still have it around, even after git updates. It's now generated in the /tmp/arduino_build_* tree, so the original patch works fine unless you have the old obsolete one in the ld dir, which people with long-running dev systems will. Do you have a good idea how to get around this? Should we delete any old .ld file there unconditionally? Need to merge whatever way we use to fix this with any update so the problem @wemos and I had won't spread to others.

d-a-v · 2018-09-18T22:50:44Z

@earlephilhower it must have been tough to find this one!
I don't see any other way than removing it from platform.txt scripts.
The generated one is in build dir (/tmp on linux).
edit: your idea to rename it is better

jjsuwa · 2018-09-19T00:04:23Z

@earlephilhower - Just found the problem here, @jjsuwa.

Good job :)

Similar to PROGMEM changes, make the name of each ICACHE_* variable/fuction unique to avoid issues with section conflicts. Also rename the generated LD linker script to avoid issue with older copies of the eagle.app.v6.common.ld which were generated by the build process in a global directory before being moved to the {build.path}. The linker would use the older, generated *.ld file instead of the generated one, which would lead to runtime failures on some systems and cause the VTABLE location to not correspond to the IDE menu selection. Fixes esp8266#5115, and is an update to esp8266#5117 and esp8266#5116.

* Move ICACHE_* to unique sections, local LD script Similar to PROGMEM changes, make the name of each ICACHE_* variable/fuction unique to avoid issues with section conflicts. Also rename the generated LD linker script to avoid issue with older copies of the eagle.app.v6.common.ld which were generated by the build process in a global directory before being moved to the {build.path}. The linker would use the older, generated *.ld file instead of the generated one, which would lead to runtime failures on some systems and cause the VTABLE location to not correspond to the IDE menu selection. Fixes #5115, and is an update to #5117 and #5116. * Update boards.txt.py and platform.io build

earlephilhower mentioned this pull request Sep 11, 2018

Make unique sections for ICACHE_* variables #5116

Closed

earlephilhower approved these changes Sep 11, 2018

View reviewed changes

Make unique sections for ICACHE_* variables

54d94d8

Similar to PROGMEM changes, make the name of each ICACHE_* variable/fuction unique to avoid issues with section conflicts. Fixes #5115

devyte merged commit 622569c into esp8266:master Sep 11, 2018

jjsuwa deleted the iram_names branch September 11, 2018 17:44

earlephilhower added a commit to earlephilhower/Arduino that referenced this pull request Sep 12, 2018

Undo 5117 changes to ICACHE macros

1cde667

With the changes in PR esp8266#5117 , blink and other examples compile but the 8266 gets stuck in a reset loop. Undo the changes from that commit.

earlephilhower mentioned this pull request Sep 12, 2018

Undo 5117 changes to ICACHE macros #5130

Merged

earlephilhower mentioned this pull request Sep 19, 2018

Move ICACHE_* to unique sections, local LD script #5150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make unique sections for ICACHE_* variables #5117

Make unique sections for ICACHE_* variables #5117

jjsuwa commented Sep 10, 2018

devyte commented Sep 10, 2018

earlephilhower commented Sep 10, 2018

jjsuwa commented Sep 10, 2018 •

edited

Loading

earlephilhower left a comment

wemos commented Sep 12, 2018

jjsuwa commented Sep 12, 2018 •

edited

Loading

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 13, 2018 •

edited

Loading

jjsuwa commented Sep 13, 2018

earlephilhower commented Sep 13, 2018

earlephilhower commented Sep 18, 2018

d-a-v commented Sep 18, 2018 •

edited

Loading

jjsuwa commented Sep 19, 2018

Make unique sections for ICACHE_* variables #5117

Make unique sections for ICACHE_* variables #5117

Conversation

jjsuwa commented Sep 10, 2018

devyte commented Sep 10, 2018

earlephilhower commented Sep 10, 2018

jjsuwa commented Sep 10, 2018 • edited Loading

earlephilhower left a comment

Choose a reason for hiding this comment

wemos commented Sep 12, 2018

jjsuwa commented Sep 12, 2018 • edited Loading

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 12, 2018

jjsuwa commented Sep 12, 2018

earlephilhower commented Sep 13, 2018 • edited Loading

jjsuwa commented Sep 13, 2018

earlephilhower commented Sep 13, 2018

earlephilhower commented Sep 18, 2018

d-a-v commented Sep 18, 2018 • edited Loading

jjsuwa commented Sep 19, 2018

jjsuwa commented Sep 10, 2018 •

edited

Loading

jjsuwa commented Sep 12, 2018 •

edited

Loading

earlephilhower commented Sep 13, 2018 •

edited

Loading

d-a-v commented Sep 18, 2018 •

edited

Loading