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

Allow byte access to PROGMEM without pgm_read macro #6978

Closed

Conversation

earlephilhower
Copy link
Collaborator

This PR pulls in code from @pvvx's esp8266web server at
https://github.com/pvvx/esp8266web (licensed in the public domain)
to allow reads of 8 and 16-bit values out of flash without the need
for pgm_read_byte/word macros. It installs an exception handler that
decodes the faulting instruction and simulates is properly and returns
immediately to user code. That said, it is significantly slower (20x?)
than using the pgm_read macros properly.

@earlephilhower
Copy link
Collaborator Author

earlephilhower commented Jan 3, 2020

This works, but doesn't yet have code to fall-through to the real unhandled exception handler in the case when it can't actually fix the code (i.e. a read from NULL)

const char text[32] PROGMEM = "abcdefghijklmn";

void setup() {
  Serial.begin(115200);
  char a;
  for (auto i=0; i<8; i++) {
    auto a=text[i];
    Serial.printf("%d: '%c'\n", i, a);
  }

}

void loop() {
}

Gives

SDK:2.2.2-dev(38a443e)/Core:2.6.3-17-gfa5040d5=20603017/lwIP:STABLE-2_1_2_RELEASE/glue:1.2-16-ge23a07e/BearSSL:89454af
0: 'a'
1: 'b'
2: 'c'
3: 'd'
4: 'e'
5: 'f'
6: 'g'
7: 'h'

This PR pulls in code from @pvvx's esp8266web server at
https://github.com/pvvx/esp8266web (licensed in the public domain)
to allow reads of 8 and 16-bit values out of flash without the need
for pgm_read_byte/word macros.  It installs an exception handler that
decodes the faulting instruction and simulates is properly and returns
immediately to user code.  That said, it is significantly slower (20x?)
than using the pgm_read macros properly.
@earlephilhower earlephilhower changed the title WIP - Allow byte access to PROGMEM without pgm_read macro Allow byte access to PROGMEM without pgm_read macro Jan 3, 2020
@earlephilhower
Copy link
Collaborator Author

All working now. *(NULL) will generate the proper fault:

const char text[32] PROGMEM = "abcdefghijklmn";

void setup() {
  Serial.begin(115200);
  Serial.printf("addr=%p\n", text);
  for (auto i=0; i<8; i++) {
    auto a=text[i];
    Serial.printf("%d: '%c'\n", i, a);
  }
  Serial.printf("Waiting...will access NULL in a sec\n");
  delay(1000);
  Serial.printf("*(0) = %d", *(int*)0);
}

void loop() {
}

Gives

0: 'a'
1: 'b'
2: 'c'
3: 'd'
4: 'e'
5: 'f'
6: 'g'
7: 'h'
Waiting...will access NULL in a sec

Exception (28):
epc1=0x40201085 epc2=0x00000000 epc3=0x00000000 excvaddr=0x00000000 depc=0x00000000

>>>stack>>>

ctx: cont
sp: 3ffffe00 end: 3fffffc0 offset: 01a0
3fffffa0:  3fffdad0 00000000 3ffee34c 40201914  
3fffffb0:  feefeffe feefeffe 3ffe84e4 40100c99  
<<<stack<<<

And the crash stack decodes properly to the faulting line:

Exception 28: LoadProhibited: A load referenced a page mapped with an attribute that does not permit loads
PC: 0x40201085: setup() at /home/earle/Arduino/sketch_jan02a/sketch_jan02a.ino line 12
EXCVADDR: 0x00000000

Decoding stack results
0x40201914: loop_wrapper() at /home/earle/Arduino/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_main.cpp line 179

Move the ROM defintions into esp8266_undefined.h, and add ifdef
bracketing around the whole header (it's included multiple times).

Remove some unneeded, left-over code and comments.

Use __asm to make compatible with --std=c++17

Requires 162 bytes of IRAM for the handler.
@dirkmueller
Copy link
Contributor

I am missing the rationale on why this should be done, especially with no compile time switch to turn it off (or why it is on by default in the first place).

To me it sounds like to hide bugs (missing too call _P variant somewhere) with introducing a performance problem that will be very difficult to find.

At least the option to turn this of needs to bet there, and imho it should be off by default

@earlephilhower
Copy link
Collaborator Author

Good comments, @dirkmueller. I can see why we'd want to allow for removing it via the menus (besides the ~150 bytes of IRAM).

It's a matter of reducing friction and making it easier for new users. We're used to it, but having separate I and D spaces is not very common and leads to confusion with new users. You and I get it, but someone coming from a RPi or who's only over coded in a HLL hasn't a clue.

Today, this is a bug:

const char text[] PROGMEM = "The big red dog";
strcpy(dest, text);

With this patch it's no longer a bug, it''ll do what it says, albeit slower than if you used strcpy_P. Taking a crash and making it work (but somewhat slower) eliminates a whole class of potential gotchas.

The LUA guys handle it by using -mforce-l32 compilation which makes every single access a pgm_read_byte equivalent, even RAM accesses.

Espressif handles it in later SDKs by including essentially the same handler (pre3.0 had one). So, as we move to an updated SDK we're going to have it whether we want to or not (unless we forcibly overwrite it).

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 6, 2020

While I agree with the benefits, I also tend to agree with @dirkmueller when it comes to hidden loss of performances.
I anyway wouldn't be pleased to not having this feature,
@dirkmueller @earlephilhower what do you think of emitting a message on console (once per boot) when this feature is effectively used ?

@dirkmueller
Copy link
Contributor

I guess that's dangerous from an exception handler... Anyway I am good as long as there is a way to turn this off at compile time

@d-a-v
Copy link
Collaborator

d-a-v commented Jan 6, 2020

True, as @earlephilhower told me.
But not dangerous with scheduled functions, as he quickly added. Which I +1.

Adds a menu option to disable the handler (default is enabled for
beginning users).

Add a single debug printout when it is invoked via a scheduled function
warning that performance may suffer.
@earlephilhower
Copy link
Collaborator Author

I've added a menu option (default on) to disable the handler and a single printout when in debug mode. I think a link to a "how to use progmem" page would be good to dump as well, but I haven't found a good one...

@earlephilhower
Copy link
Collaborator Author

I'm closing this for now. The response has been pretty consistently unhappy. In the future SDKs, where this is implemented by the blobs, we'll need a different method to remove it than this, so we should look at it fresh when that happens.

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

Successfully merging this pull request may close these issues.

None yet

3 participants