`pgm_read_float_unaligned()` ignores the second word

[jjsuwa-sys3175]

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

[d-a-v]

Do you intend to propose a Pull-Request ?

[jjsuwa-sys3175]

@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]

@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]

@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]

@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]

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]

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]

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

[jjsuwa-sys3175]

LGTM, thx @earlephilhower

[dok-net]

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]

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

[earlephilhower]

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]

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?