Skip to content

Unaligned access support for pgm_read_word/dword #5692

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Apr 11, 2019
Merged

Unaligned access support for pgm_read_word/dword #5692

merged 10 commits into from
Apr 11, 2019

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Jan 29, 2019 •
edited
Loading

Two versions of the multibyte pgm_read_* macros are now available: pgm_read_*_aligned() and pgm_read_*_unaligned(). The _aligned() versions are the original ones that require machine-dependent alignment of 2- and 4-byte values and compile into 1-2 instructions, while the _unaligned() versions have no such restriction (but compile to 15x the instructions and take ~4x to ~15x the cycles)).

The standard pgm_read_(dword/word) macros are defined to the unaligned() versions to provide Arduino compatibility out of the box.

It looks to be a minimal impact on core code because we don't have a lot of 16- or 32-bit ints in PROGMEM anyway, but a final scrub of core to hardcode _aligned() would probably be in order.

Fixes #5628

@earlephilhower earlephilhower added this to the 2.6.0 milestone Jan 29, 2019
@earlephilhower earlephilhower mentioned this pull request Jan 29, 2019
Closed
@earlephilhower
Copy link
Collaborator Author

The core proper doesn't use pgm_read_word or pgm_read_dword except for one spot in WiFiClientSecureBearSSL (which is not time critical and is simply copying a ~20 word array to RAM containing the supported encryption signatures).

earle@server:~/Arduino/hardware/esp8266com/esp8266$ grep -r -e pgm_read_dword -e pgm_read_word | cut -f1 -d: | sort -u
Binary file package/versions/2.5.0-dev-nightly+20190119/esp8266-2.5.0-dev-nightly+20190119/tools/sdk/lib/libbearssl.a matches
Binary file tools/sdk/lib/libbearssl.a matches
libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp
tests/host/sys/pgmspace.h
tools/sdk/libc/xtensa-lx106-elf/include/sys/pgmspace.h
tools/sdk/ssl/bearssl/inc/pgmspace.h
tools/sdk/ssl/bearssl/src/codec/pemdec.c
tools/sdk/ssl/bearssl/src/ec/ec_prime_i15.c
tools/sdk/ssl/bearssl/src/ssl/ssl_hs_client.c
tools/sdk/ssl/bearssl/src/ssl/ssl_hs_server.c
tools/sdk/ssl/bearssl/src/ssl/ssl_keyexport.c
tools/sdk/ssl/bearssl/src/x509/pkey_decoder.c
tools/sdk/ssl/bearssl/src/x509/skey_decoder.c
tools/sdk/ssl/bearssl/src/x509/x509_decoder.c
tools/sdk/ssl/bearssl/src/x509/x509_minimal.c
tools/sdk/ssl/bearssl/T0/T0Comp.cs
tools/xtensa-lx106-elf.bak/share/info/gcc.info
tools/xtensa-lx106-elf.bak/xtensa-lx106-elf/include/sys/pgmspace.h
tools/xtensa-lx106-elf/share/info/gcc.info
tools/xtensa-lx106-elf/xtensa-lx106-elf/include/sys/pgmspace.h

So, given that, I think there's no need for change in the Arduino core proper.

BearSSL does use pgm_read_wrd, though, but it has its own private copy of pgmspace.h (since it builds outside the SDK with its own Makefile). So no change is needed there, either, as long as the old, only-aligned private copy is used there.

Newlib has no use for pgm_read_word/_dword, either, and only need the updated header for future builds (actually already in the repo):

earle@server:~/src/esp-quick-toolchain/repo$ grep -r -e pgm_read_word -e pgm_read_dword newlib/
newlib/.git/COMMIT_EDITMSG:Make the pgm_read_word and _dword macros support reading unaligned
newlib/newlib/libc/include/sys/pgmspace.h:    #define pgm_read_word(addr)             (*reinterpret_cast<const uint16_t*>(addr))
newlib/newlib/libc/include/sys/pgmspace.h:    #define pgm_read_dword(addr)            (*reinterpret_cast<const uint32_t*>(addr))
newlib/newlib/libc/include/sys/pgmspace.h:    #define pgm_read_word(addr)             (*(const uint16_t*)(addr))
newlib/newlib/libc/include/sys/pgmspace.h:    #define pgm_read_dword(addr)            (*(const uint32_t*)(addr))
newlib/newlib/libc/include/sys/pgmspace.h:#define pgm_read_word_near(addr)        pgm_read_word(addr)
newlib/newlib/libc/include/sys/pgmspace.h:#define pgm_read_dword_near(addr)       pgm_read_dword(addr)
newlib/newlib/libc/include/sys/pgmspace.h:#define pgm_read_word_far(addr)         pgm_read_word(addr)
newlib/newlib/libc/include/sys/pgmspace.h:#define pgm_read_dword_far(addr)        pgm_read_dword(addr)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:static inline uint16_t pgm_read_word_inlined(const void* addr) {
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:#define pgm_read_word_aligned(addr)        pgm_read_word_inlined(addr)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    #define pgm_read_dword_aligned(addr)   (*reinterpret_cast<const uint32_t*>(addr))
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    #define pgm_read_dword_aligned(addr)   (*(const uint32_t*)(addr))
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:__attribute__((optimize("-O3"), always_inline)) static inline uint32_t pgm_read_dword_unaligned(const void *addr) {
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    return (float)pgm_read_dword_unaligned(addr);
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    return (void *)pgm_read_dword_unaligned(addr);
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:__attribute__((optimize("-O3"), always_inline)) static inline uint16_t pgm_read_word_unaligned(const void *addr) {
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    return pgm_read_dword_unaligned(addr) & 0xffff;
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    #define pgm_read_word(a)   pgm_read_word_unaligned(a)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    #define pgm_read_dword(a)  pgm_read_dword_unaligned(a)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    #define pgm_read_word(a)   pgm_read_word_aligned(a)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:    #define pgm_read_dword(a)  pgm_read_dword_aligned(a)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:#define pgm_read_word_near(addr)        pgm_read_word(addr)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:#define pgm_read_dword_near(addr)       pgm_read_dword(addr)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:#define pgm_read_word_far(addr)         pgm_read_word(addr)
newlib/newlib/libc/sys/xtensa/sys/pgmspace.h:#define pgm_read_dword_far(addr)        pgm_read_dword(addr)

Given all that, I'd say the code is pretty low risk to the base repo. Other people's libraries, though, would need to be examined...

@earlephilhower earlephilhower changed the title Unaligned access support for pgm_read_word/dword - Not for merge yet! Unaligned access support for pgm_read_word/dword Jan 31, 2019
@earlephilhower
Copy link
Collaborator Author

Link to #5733, this should include fix for that one plus some form of testing of the obscure macros like that one...

@earlephilhower
Allow users to use aligned-only via a #define
15f3dec 
Adding -DPGM_READ_UNALIGNED=0 or #define PGM_READ_UNALIGNED 0 will
change the default at compile-time to only aligned (faster, but less
compatible) macro implementations.

Default is still to allow unaligned accesses.
@earlephilhower
Merge branch 'master' into unaligned
ecd2b44
@earlephilhower
Merge branch 'master' into unaligned
801f127
@earlephilhower
Merge branch 'master' into unaligned
3842952
@earlephilhower
Merge branch 'master' into unaligned
41bd909
@earlephilhower
Merge branch 'master' into unaligned
c9bdb1d
@earlephilhower earlephilhower merged commit 885276e into esp8266:master Apr 11, 2019
@earlephilhower earlephilhower deleted the unaligned branch April 11, 2019 19:24
@d-a-v d-a-v modified the milestones: 2.6.0, 2.5.1 Apr 11, 2019
@luc-github
Copy link
Contributor

Other people's libraries, though, would need to be examined...

Hi @earlephilhower for what I can see here Bodmer/TFT_eSPI#348 TFT_eSPI library is affected by this change

@maxint-rd
Copy link

maxint-rd commented Oct 2, 2019 •
edited
Loading

Hmm... My MmlMusic library reads tones as float values from PROGMEM and is also affected.
pgm_read_float worked fine in core 2.5.0, but doesn't in later versions. This broke my library.
Using the defines mentioned in issue #5628 fixed the problem, but I guess it's better to use pgm_read_float_aligned(). The default define uses the _unaligned version, but as I noticed then the floats are not properly read.

@earlephilhower
Copy link
Collaborator Author

@maxint-rd please open a new issue with MCVE to get that looked into.

maxint-rd added a commit to maxint-rd/MmlMusic that referenced this pull request Oct 2, 2019
@maxint-rd
Fix incorrect reading of floats in ESP8266 cores 2.5.1 and higher
13d7330 
ESP8266 cores 2.5.1 and 2.5.2 have an alignment issue causing incorrect reading of floats in PROGMEM
See esp8266/Arduino#5628 and esp8266/Arduino#5692
By default the 2.5.1 and 2.5.2 cores use pgm_read_float_unaligned() but then the PROGMEM floats are not properly read.
The read frequencies were way to high resulting in crackling noise or high pitched sound.
Added a conditional redefine for ESP8266 environment which fixes properly reading the float values.
@maxint-rd maxint-rd mentioned this pull request Oct 3, 2019
Closed
6 tasks
@maxint-rd
Copy link

Okay, I just tested the behavior in an MCVE and opened a new issue (#6590).
Please note that perhaps my issue is caused by misunderstanding. In that case please let me know.

@Bodmer Bodmer mentioned this pull request Apr 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-a-v d-a-v d-a-v left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5.1
Development

Successfully merging this pull request may close these issues.

Misaligned multi-byte formats not supported in pgm_read_*
4 participants
@earlephilhower @luc-github @maxint-rd @d-a-v