`pgm_read_float_unaligned()` ignores the second word #8083

jjsuwa-sys3175 · 2021-05-29T12:28:26Z

Basic Infos

This issue complies with the issue POLICY doc.
I have read the documentation at readthedocs and the issue is not addressed there.
I have tested that the issue is present in current master branch (aka latest git).
I have searched the issue tracker for a similar issue.
If there is a stack dump, I have decoded it.
I have filled out all fields below.

Platform

Hardware: [other (ESP-12 compatible)] (AE-ESP-WROOM-02: see https://akizukidenshi.com/catalog/g/gK-09758/)
Core Version: [bf6f3e7]
Development Env: [Arduino IDE]
Operating System: [Windows]

Settings in IDE

Module: [Generic ESP8266 Module]
Flash Mode: [qio]
Flash Size: [4MB]
lwip Variant: [v2 Lower Memory]
Reset Method: [nodemcu]
Flash Frequency: [80Mhz]
CPU Frequency: [160MHz]
Upload Using: [SERIAL]
Upload Speed: [115200]

Problem Description

pgm_read_float_unaligned() in 'tools/xtensa-lx106-elf/xtensa-lx106-elf/include/sys/pgmspace.h' should use pgm_read_dword_with_offset() instead of pgm_read_with_offset(), as per pgm_read_dword_unaligned().

MCVE Sketch

void setup() {
  // put your setup code here, to run once:

  static const float SPECIMEN = 3.14159265359F;
  static uint8_t __attribute__((aligned(4))) buffer[8];
  memset(buffer, 0, sizeof(buffer));

  uint8_t* unaligned_location = &buffer[1];
  memcpy(unaligned_location, &SPECIMEN, sizeof(SPECIMEN));

  Serial.begin(115200);
  Serial.print("\n\n"
               "** pgm_read_float_unaligned() test **\n"
               "specimen: ");
  Serial.println(SPECIMEN, 6);
  Serial.print("readout:  ");
  Serial.println(pgm_read_float_unaligned(unaligned_location), 6);
}

void loop() {
  // put your main code here, to run repeatedly:

}

Debug Messages

** pgm_read_float_unaligned() test **
specimen: 3.141593
readout:  0.000000

With the abovementioned way,

** pgm_read_float_unaligned() test **
specimen: 3.141593
readout:  3.141593

The text was updated successfully, but these errors were encountered:

d-a-v · 2021-05-30T07:48:34Z

Do you intend to propose a Pull-Request ?

jjsuwa-sys3175 · 2021-05-30T07:58:16Z

@d-a-v :

Do you intend to propose a Pull-Request ?

perhaps if an agreement is formed; there may be some meaning that I don't know in the current behavior :)

dok-net · 2021-05-30T09:43:23Z

@d-a-v I was almost going to do that yesterday, but I'm fairly certain that tools/xtensa-lx106-elf/xtensa-lx106-elf/include/sys/pgmspace.h gets downloaded from another repo, according to package/package_esp8266com_index.template.json (@earlephilhower ?) by tools/get.py, and would be patched there. Not quite obvious how to go about that in the right way, probably the right step is to enter an issue there? It may need to be communicated upstream, eventually, as well.
https://github.com/earlephilhower/esp-quick-toolchain

Also, maybe doubles need to be considered, too?

sizes
sizeof(uint32_t) = 4
sizeof(float) = 4
sizeof(double) = 8

dok-net · 2021-05-30T09:51:36Z

@earlephilhower By the way, my IDE has in option "13"
Non-32-Bit Access, which doesn't fail to baffle me each time, am I right that what's meant, albeit slightly verbose:
Non-32-Bit Aligned Access? What do you think, should I make a PR?

jjsuwa-sys3175 · 2021-05-30T11:54:23Z

@dok-net :

https://github.com/earlephilhower/esp-quick-toolchain

https://github.com/earlephilhower/newlib-xtensa is more appropriate

Also, maybe doubles need to be considered, too?

like as below, if use of pgm_read_dword_with_offset() is appropriate:

static inline double pgm_read_double_unaligned(const void* addr) {
  union {
    double d;
    uint32_t i[2];
  } u;
  pgm_read_dword_with_offset(addr + 0, u.i[0]);
  pgm_read_dword_with_offset(addr + 4, u.i[1]);
  return u.d;
}

this emits about 15 or 16 machine insns, thus it may not be inlining

dok-net · 2021-05-30T12:28:42Z

https://github.com/earlephilhower/newlib-xtensa is more appropriate

I don't think so; here's a naive patch, anyway. Please review thoroughly on your own schedule:

--- pgmspace.h.orig	2021-05-30 14:09:09.107518600 +0200
+++ pgmspace.h	2021-05-30 14:16:29.943471300 +0200
@@ -112,19 +112,31 @@
 /* The ASM block doesn't care the type, so just pass in what C thinks is a float and return in custom fcn. */
 static inline float pgm_read_float_unaligned(const void* addr) {
   float res;
-  pgm_read_with_offset(addr, res);
+  pgm_read_dword_with_offset(addr, res);
   return res;
 }
 
+static inline double pgm_read_double_unaligned(const void* addr) {
+  union {
+    double d;
+    uint32_t i[2];
+  } u;
+  pgm_read_dword_with_offset(addr + 0, u.i[0]);
+  pgm_read_dword_with_offset(addr + 4, u.i[1]);
+  return u.d;
+}
+
 #define pgm_read_byte(addr)                pgm_read_byte_inlined(addr)
 #define pgm_read_word_aligned(addr)        pgm_read_word_inlined(addr)
 #ifdef __cplusplus
     #define pgm_read_dword_aligned(addr)   (*reinterpret_cast<const uint32_t*>(addr))
     #define pgm_read_float_aligned(addr)   (*reinterpret_cast<const float*>(addr))
+    #define pgm_read_double_aligned(addr)  (*reinterpret_cast<const double*>(addr))
     #define pgm_read_ptr_aligned(addr)     (*reinterpret_cast<const void* const*>(addr))
 #else
     #define pgm_read_dword_aligned(addr)   (*(const uint32_t*)(addr))
     #define pgm_read_float_aligned(addr)   (*(const float*)(addr))
+    #define pgm_read_double_aligned(addr)  (*(const double*)(addr))
     #define pgm_read_ptr_aligned(addr)     (*(const void* const*)(addr))
 #endif
 
@@ -148,11 +160,13 @@
     #define pgm_read_word(a)   pgm_read_word_unaligned(a)
     #define pgm_read_dword(a)  pgm_read_dword_unaligned(a)
     #define pgm_read_float(a)  pgm_read_float_unaligned(a)
+    #define pgm_read_double(a) pgm_read_double_unaligned(a)
     #define pgm_read_ptr(a)    pgm_read_ptr_unaligned(a)
 #else
     #define pgm_read_word(a)   pgm_read_word_aligned(a)
     #define pgm_read_dword(a)  pgm_read_dword_aligned(a)
     #define pgm_read_float(a)  pgm_read_float_aligned(a)
+    #define pgm_read_double(a) pgm_read_double_aligned(a)
     #define pgm_read_ptr(a)    pgm_read_ptr_aligned(a)
 #endif
 
@@ -160,11 +174,13 @@
 #define pgm_read_word_near(addr)        pgm_read_word(addr)
 #define pgm_read_dword_near(addr)       pgm_read_dword(addr)
 #define pgm_read_float_near(addr)       pgm_read_float(addr)
+#define pgm_read_double_near(addr)      pgm_read_double(addr)
 #define pgm_read_ptr_near(addr)         pgm_read_ptr(addr)
 #define pgm_read_byte_far(addr)         pgm_read_byte(addr)
 #define pgm_read_word_far(addr)         pgm_read_word(addr)
 #define pgm_read_dword_far(addr)        pgm_read_dword(addr)
 #define pgm_read_float_far(addr)        pgm_read_float(addr)
+#define pgm_read_double_far(addr)       pgm_read_double(addr)
 #define pgm_read_ptr_far(addr)          pgm_read_ptr(addr)
 
 #define _SFR_BYTE(n) (n)

earlephilhower · 2021-05-31T15:46:45Z

There is no pgm_read_double in the official Arduino.h, so I would not add one here.

I'll update newlib and throw out a new toolchain build w/the fix.

earlephilhower · 2021-05-31T15:52:53Z

diff --git a/newlib/libc/sys/xtensa/sys/pgmspace.h b/newlib/libc/sys/xtensa/sys/pgmspace.h
index 40f077d3d..16d3efeb2 100644
--- a/newlib/libc/sys/xtensa/sys/pgmspace.h
+++ b/newlib/libc/sys/xtensa/sys/pgmspace.h
@@ -112,7 +112,7 @@ static inline uint16_t pgm_read_word_inlined(const void* addr) {
 /* The ASM block doesn't care the type, so just pass in what C thinks is a float and return in custom fcn. */
 static inline float pgm_read_float_unaligned(const void* addr) {
   float res;
-  pgm_read_with_offset(addr, res);
+  pgm_read_dword_with_offset(addr, res);
   return res;
 }

And output

09:12:15.087 -> 
09:12:15.087 -> ** pgm_read_float_unaligned() test **
09:12:15.087 -> specimen: 3.141593
09:12:15.087 -> readout:  3.141593

Was only reading out 16 bits worth of data, not 32, and corrupting the output. See esp8266/Arduino#8083

jjsuwa-sys3175 · 2021-05-31T16:02:24Z

LGTM, thx @earlephilhower

dok-net · 2021-05-31T16:55:14Z

There is no pgm_read_double in the official Arduino.h, so I would not add one here.

I beg to differ:

mattairtech/ArduinoCore-samd@8b7398d :

Added Stream::parseDouble() to Stream.cpp, and pgm_read_double(), pgm_read_double_near(), and pgm_read_double_far() to avr/pgmspace.h.

Edit: OK, sorry, too fast, this of course is just another person's fork. ... Still researching....
Edit 2: pgm_read_float isn't used much (I find one occurrence in ESP8266 Core for Arduino), so pgm_read_double won't be missed much, but: does it make much effort, is there any cost associated?

dok-net · 2021-05-31T16:55:57Z

@devyte You keep referring to SAMD Arduino for 32bit etc matters, can you give an opinion on pgm_read_double?

earlephilhower · 2021-05-31T17:02:00Z

The Arduino API (such as it is) does not have such a macro:
https://github.com/arduino/ArduinoCore-API/blob/master/api/deprecated-avr-comp/avr/pgmspace.h

dok-net · 2021-05-31T17:13:18Z

I know, but why care about AVR on the ESPs, as I suggested, @devyte refers to SAMD in situations like this and I think that is appropriate. Why not further the state of the art, this ESP8266 core is compatible, a superset anyway, why restrict ourselves and the users, just while you are at it already?

Fixes esp8266#8083

dok-net · 2021-05-31T17:47:06Z

@earlephilhower It's a matter of need so far. On ATMEGA, sizeof(double) == sizeof(float) == 4. On the Due specifically, it's sizeof(double) == 8, but the Due apparently doesn't have alignment restrictions. So on the ESP there's sizeof(double) == 8, and alignment restrictions, don't you think that the pgm_read_double is a natural helper in that situation?

earlephilhower · 2021-05-31T18:39:03Z

The point was that it seems that the whole pgm_read_XXX stuff is deprecated since Arduino is moving forward w/Cortex chips which all can read from flash just fine w/o special magic. AVRs are Harvard architecture so need different instructions to read from flash vs. RAM.

The Xeon's already paid for, though, so WTH. Fired off another build and will up the new tools JSON.

diff --git a/newlib/libc/sys/xtensa/sys/pgmspace.h b/newlib/libc/sys/xtensa/sys/pgmspace.h
index 16d3efeb2..6a6549e44 100644
--- a/newlib/libc/sys/xtensa/sys/pgmspace.h
+++ b/newlib/libc/sys/xtensa/sys/pgmspace.h
@@ -116,15 +116,28 @@ static inline float pgm_read_float_unaligned(const void* addr) {
   return res;
 }
 
+/* Use union evil magic to allow writing to 2 halves of double 8-byte quantity w/o generating a GCC warning. */
+static inline double pgm_read_double_unaligned(const void* addr) {
+  union {
+    double res;
+    uint32_t i[2];
+  } u;
+  pgm_read_dword_with_offset(addr, u.i[0]);
+  pgm_read_dword_with_offset(addr + 4, u.i[1]);
+  return u.res;
+}
+
 #define pgm_read_byte(addr)                pgm_read_byte_inlined(addr)
 #define pgm_read_word_aligned(addr)        pgm_read_word_inlined(addr)
 #ifdef __cplusplus
     #define pgm_read_dword_aligned(addr)   (*reinterpret_cast<const uint32_t*>(addr))
     #define pgm_read_float_aligned(addr)   (*reinterpret_cast<const float*>(addr))
+    #define pgm_read_double_aligned(addr)  (*reinterpret_cast<const double*>(addr))
     #define pgm_read_ptr_aligned(addr)     (*reinterpret_cast<const void* const*>(addr))
 #else
     #define pgm_read_dword_aligned(addr)   (*(const uint32_t*)(addr))
     #define pgm_read_float_aligned(addr)   (*(const float*)(addr))
+    #define pgm_read_double_aligned(addr)  (*(const double*)(addr))
     #define pgm_read_ptr_aligned(addr)     (*(const void* const*)(addr))
 #endif
 
@@ -148,11 +161,13 @@ static inline uint32_t pgm_read_dword_unaligned(const void *addr) {
     #define pgm_read_word(a)   pgm_read_word_unaligned(a)
     #define pgm_read_dword(a)  pgm_read_dword_unaligned(a)
     #define pgm_read_float(a)  pgm_read_float_unaligned(a)
+    #define pgm_read_double(a) pgm_read_double_unaligned(a)
     #define pgm_read_ptr(a)    pgm_read_ptr_unaligned(a)
 #else
     #define pgm_read_word(a)   pgm_read_word_aligned(a)
     #define pgm_read_dword(a)  pgm_read_dword_aligned(a)
     #define pgm_read_float(a)  pgm_read_float_aligned(a)
+    #define pgm_read_double(a) pgm_read_double_aligned(a)
     #define pgm_read_ptr(a)    pgm_read_ptr_aligned(a)
 #endif
 
@@ -160,11 +175,13 @@ static inline uint32_t pgm_read_dword_unaligned(const void *addr) {
 #define pgm_read_word_near(addr)        pgm_read_word(addr)
 #define pgm_read_dword_near(addr)       pgm_read_dword(addr)
 #define pgm_read_float_near(addr)       pgm_read_float(addr)
+#define pgm_read_double_near(addr)      pgm_read_double(addr)
 #define pgm_read_ptr_near(addr)         pgm_read_ptr(addr)
 #define pgm_read_byte_far(addr)         pgm_read_byte(addr)
 #define pgm_read_word_far(addr)         pgm_read_word(addr)
 #define pgm_read_dword_far(addr)        pgm_read_dword(addr)
 #define pgm_read_float_far(addr)        pgm_read_float(addr)
+#define pgm_read_double_far(addr)       pgm_read_double(addr)
 #define pgm_read_ptr_far(addr)          pgm_read_ptr(addr)
 
 #define _SFR_BYTE(n) (n)

Update toolchain to fix pgm_read_float_unaligned Fixes #8083

earlephilhower self-assigned this May 31, 2021

earlephilhower added a commit to earlephilhower/newlib-xtensa that referenced this issue May 31, 2021

Fix pgm_read_float_unaligned

cd538f9

Was only reading out 16 bits worth of data, not 32, and corrupting the output. See esp8266/Arduino#8083

earlephilhower added a commit to earlephilhower/Arduino that referenced this issue May 31, 2021

Update toolchain to fix pgm_read_float_unaligned

58653a2

Fixes esp8266#8083

earlephilhower mentioned this issue May 31, 2021

Update toolchain to fix pgm_read_float_unaligned #8091

Merged

d-a-v closed this as completed in #8091 Jun 4, 2021

d-a-v pushed a commit that referenced this issue Jun 4, 2021

Update toolchain to fix pgm_read_float_unaligned (#8091)

114a726

Update toolchain to fix pgm_read_float_unaligned Fixes #8083

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`pgm_read_float_unaligned()` ignores the second word #8083

`pgm_read_float_unaligned()` ignores the second word #8083

jjsuwa-sys3175 commented May 29, 2021

d-a-v commented May 30, 2021

jjsuwa-sys3175 commented May 30, 2021

dok-net commented May 30, 2021 •

edited

Loading

dok-net commented May 30, 2021

jjsuwa-sys3175 commented May 30, 2021

dok-net commented May 30, 2021 •

edited

Loading

earlephilhower commented May 31, 2021

earlephilhower commented May 31, 2021 •

edited

Loading

jjsuwa-sys3175 commented May 31, 2021

dok-net commented May 31, 2021 •

edited

Loading

dok-net commented May 31, 2021

earlephilhower commented May 31, 2021

dok-net commented May 31, 2021

dok-net commented May 31, 2021 •

edited

Loading

earlephilhower commented May 31, 2021

pgm_read_float_unaligned() ignores the second word #8083

pgm_read_float_unaligned() ignores the second word #8083

Comments

jjsuwa-sys3175 commented May 29, 2021

Basic Infos

Platform

Settings in IDE

Problem Description

MCVE Sketch

Debug Messages

d-a-v commented May 30, 2021

jjsuwa-sys3175 commented May 30, 2021

dok-net commented May 30, 2021 • edited Loading

dok-net commented May 30, 2021

jjsuwa-sys3175 commented May 30, 2021

dok-net commented May 30, 2021 • edited Loading

earlephilhower commented May 31, 2021

earlephilhower commented May 31, 2021 • edited Loading

jjsuwa-sys3175 commented May 31, 2021

dok-net commented May 31, 2021 • edited Loading

dok-net commented May 31, 2021

earlephilhower commented May 31, 2021

dok-net commented May 31, 2021

dok-net commented May 31, 2021 • edited Loading

earlephilhower commented May 31, 2021

`pgm_read_float_unaligned()` ignores the second word #8083

`pgm_read_float_unaligned()` ignores the second word #8083

dok-net commented May 30, 2021 •

edited

Loading

dok-net commented May 30, 2021 •

edited

Loading

earlephilhower commented May 31, 2021 •

edited

Loading

dok-net commented May 31, 2021 •

edited

Loading

dok-net commented May 31, 2021 •

edited

Loading