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

IRtext.{h,cpp} strings are not in PROGMEM #1614

Closed
mcspr opened this issue Sep 23, 2021 · 3 comments · Fixed by #1623
Closed

IRtext.{h,cpp} strings are not in PROGMEM #1614

mcspr opened this issue Sep 23, 2021 · 3 comments · Fixed by #1623
Assignees

Comments

@mcspr
Copy link
Contributor

mcspr commented Sep 23, 2021

Version/revision of the library used

2.7.20

Describe the bug

Currently used definition const PROGMEM char* X = "...stuff..."; is a no-op and simply refers to the string literal pool loaded in the RAM, it should be explicitly declared as const char X[] PROGMEM = "...stuff..." in the .cpp if the idea is to store things both in the flash and bound to a single object file

// Constant text to be shared across all object files.
// This means there is only one copy of the character/string/text etc.

const PROGMEM char *kAllProtocolNamesStr =

Although, extern const char X[]; in the header does not work with arrays as the size is not known at compilation time and const char* X can't be used instead b/c it is a different type. One alternative is redirecting through a functionk, e.g. const char* protocols() { return &kAllProotoclNamesStr[0]; }, but one needs to be aware that this is a PSTR and only pgm_... reads work.

To Reproduce / example code

Running on the current esp8266 Core

#include <Arduino.h>
#include <IRtext.h>

const char teststring[] PROGMEM = "test\x00test\x00test";

void setup() {
    Serial.begin(115200);
    Serial.printf("%p\n", kAllProtocolNamesStr);
    Serial.printf("%p\n", &teststring[0]);
}

void loop() {
}

Will print 0x3ffxxxxx instead of something in the 0x402xxxxx range of the teststring[] var, where the flash strings are usually stored with the esp8266

Using something like gdb will also show the location when the -g option is set in the build flags line
(gdb.exe can be found with the gcc.exe and the rest of the toolchain, platformio.ini env modified build_flags = -g)

$ xtensa-lx106-elf-gdb.exe .\.pio\build\test\firmware.elf
(gdb) p /a kAllProtocolNamesStr
$1 = 0x3ffe8817
(gdb) p /a teststring
$2 = {0x74, 0x65, 0x73, 0x74, 0x0, 0x74, 0x65, 0x73, 0x74, 0x0, 0x74, 0x65, 0x73, 0x74, 0x0}
(gdb) p /a &teststring[0]
$3 = 0x4027a6e5 <_ZL10teststring>
(gdb)
@crankyoldgit crankyoldgit self-assigned this Sep 24, 2021
@crankyoldgit
Copy link
Owner

Xref #1493

@mcspr
Copy link
Contributor Author

mcspr commented Sep 28, 2021

Although, extern const char X[]; in the header does not work with arrays as the size is not known at compilation time and const char* X can't be used instead b/c it is a different type

Correction, as noticed here:
https://chromium.googlesource.com/chromium/src/+/HEAD/url/url_constants.h

What turned out to be the issue is that the extern const char VAR[]; needs to be in both object files. e.g.

// a.cpp
extern const char arr[];
const char arr[] __attribute__((section("\".whatever\""))) = "hello world";
// b.cpp
#include <iostream>

extern const char arr[];

int main() {
    std::cout << arr << '\n';
}
$ g++ -c -o a.o a.cpp
$ g++ -c -o b.o b.cpp
$ g++ -o run a.o b.o
$ ./run
hello world
$ size -A run | grep whatever
.whatever                 12   4202512

sizeof(arr) still does not work though, as it is still basically a simple pointer; but, at least it is possible to lose the additional const char* const arrPtr { &arr[0] };


Still, there's the issue with using this as C string. I was trying out a bit more 'modernized' API intended to replace the usage of PSTR(...) / F(...), and try to make it very similar to std::string_view. Unlike those macros, it can be used in the global scope (in .cpp only though, not in .h b/c section would be broken on esp32 as-is, or at least the last time I tried building). Small proxy / view struct also means it is pretty easy to have helper methods using ..._P funcs that work with the object directly and allowing to be very similar to the String API:

#include <Arduino.h>

// adjust accordingly (private members, const-only accessors, startsWith, substring, operator String(), etc.)
struct ConstStringPtr {
    const char* ptr;
    size_t len;

    template <size_t Size>
    explicit ConstStringPtr(const char (&buffer)[Size]) :
        ptr(&buffer[0]),
        len(Size - 1)
    {}
};

static_assert(alignof(ConstStringPtr) == alignof(char*), "");

// aligned() is a must, otherwise it is 1
#define MY_PGM_STRING(VALUE)\
    ([]() {\
        static const char __pstr__[] __attribute__((aligned(4))) PROGMEM = (VALUE);\
        return ConstStringPtr{__pstr__};\
    })()

const auto A = MY_PGM_STRING("aaaaaaa");
const auto B = MY_PGM_STRING("bbbbbbb");
const auto C = MY_PGM_STRING("ccccccc");

void loop() {
}

void setup() {
    Serial.begin(115200);
    Serial.printf("%.*s (%p, %u), %.*s (%p, %u), %.*s (%p, %u)\n",
        A.len, A.ptr, A.ptr, A.len,
        B.len, B.ptr, B.ptr, B.len,
        C.len, C.ptr, C.ptr, C.len);
    // %s still works, but now length is known even without the strlen_P
}

crankyoldgit added a commit that referenced this issue Oct 4, 2021
Finally convince the compiler to store the text strings into flash.

Saves approx 1k of Global ram, and 1-1.5k of Flash space.
Plus an overall reduction of ~1k on the resulting bin file size.

e.g.
* IRMQTTServer example code:
  - Before:
    RAM:   [=====     ]  53.2% (used 43560 bytes from 81920 bytes)
    Flash: [=====     ]  53.1% (used 554500 bytes from 1044464 bytes)
    Bin file size = 558656
  - After:
    RAM:   [=====     ]  52.0% (used 42572 bytes from 81920 bytes)
    Flash: [=====     ]  53.0% (used 553448 bytes from 1044464 bytes)
    Bin file size = 557600
* IRrecvDumpV2 example code:
  - Before:
    RAM:   [====      ]  37.0% (used 30348 bytes from 81920 bytes)
    Flash: [====      ]  35.4% (used 369644 bytes from 1044464 bytes)
    Bin file size = 373792
  - After:
    RAM:   [====      ]  35.9% (used 29372 bytes from 81920 bytes)
    Flash: [====      ]  35.2% (used 368036 bytes from 1044464 bytes)
    Bin file size = 372192

Fixes #1614
Fixes #1493
crankyoldgit added a commit that referenced this issue Oct 8, 2021
Finally convince the compiler to store the text strings into flash.

Saves approx 2k of Global ram, for a trade-off of between ~0-0.5k of extra flash space used.

e.g. _(updated)_
* IRMQTTServer example code:
  - Before:
RAM:   [=====     ]  54.1% (used 44344 bytes from 81920 bytes)
Flash: [=====     ]  54.2% (used 566209 bytes from 1044464 bytes)
Bin file size = 570368
  - After:
RAM:   [=====     ]  51.3% (used 41992 bytes from 81920 bytes)
Flash: [=====     ]  54.2% (used 566201 bytes from 1044464 bytes)
Bin file size = 570352

* IRrecvDumpV2 example code:
  - Before:
RAM:   [====      ]  37.9% (used 31044 bytes from 81920 bytes)
Flash: [====      ]  35.6% (used 372025 bytes from 1044464 bytes)
Bin file size = 376176
  - After:
RAM:   [====      ]  35.5% (used 29072 bytes from 81920 bytes)
Flash: [====      ]  35.7% (used 372525 bytes from 1044464 bytes)
Bin file size = 376672

Fixes #1614
Fixes #1493

Co-authored with @mcspr
crankyoldgit added a commit that referenced this issue Nov 19, 2021
_v2.8.0 (20211119)_

**[Bug Fixes]**
- Fix compilation issue when using old 8266 Arduino Frameworks. (#1639 #1640)
- Fix potential security issue with `scrape_supported_devices.py` (#1616 #1619)

**[Features]**
- SAMSUNG_AC
  - Change `clean` setting to a toggle. (#1676 #1677)
  - Highest fan speed is available without Powerful setting. (#1675 #1678)
  - Change `beep` setting to a toggle. (#1669 #1671)
  - Fix Beep for AR12TXEAAWKNEU (#1668 #1669)
  - Add support for Horizontal Swing & Econo (#1277 #1667)
  - Add support for On, Off, & Sleep Timers (#1277 #1662)
  - Fix power control. Clean-up code & bitmaps from Checksum changes. (#1277 #1648 #1650)
- HAIER_AC176/HAIER_AC_YRW02
  - Add support A/B unit setting (#1672)
  - Add support degree Fahrenheit (#1659)
  - Add support `Lock` function (#1652)
  - Implement horizontal swing feature (#1641)
  - Implement Quiet setting. (#1634 #1635)
- Basic support for Airton Protocol (#1670 #1681)
- HAIER_AC176: Add Turbo and Quiet settings (#1634)
- Gree: Add `SwingH` & `Econo` control. (#1587 #1653)
- MIRAGE
  - Add experimental detailed support. (#1573 #1615)
  - Experimental detailed support for KKG29A-C1 remote. (#1573 #1660)
- ELECTRA_AC: Add support for "IFeel" & Sensor settings. (#1644 #1645)
- Add Russian translation (#1649)
- Add Swedish translation (#1627)
- Reduce flash space used. (#1633)
- Strings finally in Flash! (#1493 #1614 #1623)
- Add support for Rhoss Idrowall MPCV 20-30-35-40 A/C protocol (#1630)
- Make `IRAc::opmodeToString()` output nicer for humans. (#1613)
- TCL112AC/TEKNOPOINT: Add support for `GZ055BE1` model (#1486 #1602)
- Support for Arris protocol. (#1598)
- SharpAc: Allow position control of SwingV (#1590 #1594)

**[Misc]**
- HAIER_AC176/HAIER_AC_YRW02
  - Replace some magic numbers with constants (#1679)
  - Small fix `Quiet` and `Turbo` test (#1674)
  - Fix `IRHaierAC176::getTemp()` return value description (#1663)
- Security Policy creation and changes. (#1616 #1617 #1618 #1621 #1680)
- IRrecvDumpV2/3: Update PlatformIO envs for missing languages (#1661)
- IRMQTTServer
  - Use the correct string for Fan mode in Home Assistant. (#1610 #1657)
  - Move a lot of the strings/text to flash. (#1638)
- Minor code style improvements. (#1656)
- Update Supported Devices
  - HAIER_AC176 (#1673)
  - LG A/C (#1651 #1655)
  - Symphony (#1603 #1605)
  - Epson (#1574 #1601)
  - GREE (#1587 #1588)
  - SharpAc (#1590 #1591)
- Add extra tests for LG2 protocol (#1654)
- Fix parameter expansion in several macros.
- Move some strings to `IRtext.cpp` & `locale/default.h` (#1637)
- RHOSS: Move include and defines to their correct places (#1636)
- Make makefile only build required files when running `run-%` target (#1632)
- Update Portuguese translation (#1628)
- Add possibility to run specific test case (#1625)
- Change `googletest` library ignore (#1626)
- Re-work "Fan Only" strings & matching. (#1610)
- Address `C0209` pylint warnings. (#1608)
crankyoldgit added a commit that referenced this issue Nov 19, 2021
## _v2.8.0 (20211119)_

**[Bug Fixes]**
- Fix compilation issue when using old 8266 Arduino Frameworks. (#1639 #1640)
- Fix potential security issue with `scrape_supported_devices.py` (#1616 #1619)

**[Features]**
- SAMSUNG_AC
  - Change `clean` setting to a toggle. (#1676 #1677)
  - Highest fan speed is available without Powerful setting. (#1675 #1678)
  - Change `beep` setting to a toggle. (#1669 #1671)
  - Fix Beep for AR12TXEAAWKNEU (#1668 #1669)
  - Add support for Horizontal Swing & Econo (#1277 #1667)
  - Add support for On, Off, & Sleep Timers (#1277 #1662)
  - Fix power control. Clean-up code & bitmaps from Checksum changes. (#1277 #1648 #1650)
- HAIER_AC176/HAIER_AC_YRW02
  - Add support A/B unit setting (#1672)
  - Add support degree Fahrenheit (#1659)
  - Add support `Lock` function (#1652)
  - Implement horizontal swing feature (#1641)
  - Implement Quiet setting. (#1634 #1635)
- Basic support for Airton Protocol (#1670 #1681)
- HAIER_AC176: Add Turbo and Quiet settings (#1634)
- Gree: Add `SwingH` & `Econo` control. (#1587 #1653)
- MIRAGE
  - Add experimental detailed support. (#1573 #1615)
  - Experimental detailed support for KKG29A-C1 remote. (#1573 #1660)
- ELECTRA_AC: Add support for "IFeel" & Sensor settings. (#1644 #1645)
- Add Russian translation (#1649)
- Add Swedish translation (#1627)
- Reduce flash space used. (#1633)
- Strings finally in Flash! (#1493 #1614 #1623)
- Add support for Rhoss Idrowall MPCV 20-30-35-40 A/C protocol (#1630)
- Make `IRAc::opmodeToString()` output nicer for humans. (#1613)
- TCL112AC/TEKNOPOINT: Add support for `GZ055BE1` model (#1486 #1602)
- Support for Arris protocol. (#1598)
- SharpAc: Allow position control of SwingV (#1590 #1594)

**[Misc]**
- HAIER_AC176/HAIER_AC_YRW02
  - Replace some magic numbers with constants (#1679)
  - Small fix `Quiet` and `Turbo` test (#1674)
  - Fix `IRHaierAC176::getTemp()` return value description (#1663)
- Security Policy creation and changes. (#1616 #1617 #1618 #1621 #1680)
- IRrecvDumpV2/3: Update PlatformIO envs for missing languages (#1661)
- IRMQTTServer
  - Use the correct string for Fan mode in Home Assistant. (#1610 #1657)
  - Move a lot of the strings/text to flash. (#1638)
- Minor code style improvements. (#1656)
- Update Supported Devices
  - HAIER_AC176 (#1673)
  - LG A/C (#1651 #1655)
  - Symphony (#1603 #1605)
  - Epson (#1574 #1601)
  - GREE (#1587 #1588)
  - SharpAc (#1590 #1591)
- Add extra tests for LG2 protocol (#1654)
- Fix parameter expansion in several macros.
- Move some strings to `IRtext.cpp` & `locale/default.h` (#1637)
- RHOSS: Move include and defines to their correct places (#1636)
- Make makefile only build required files when running `run-%` target (#1632)
- Update Portuguese translation (#1628)
- Add possibility to run specific test case (#1625)
- Change `googletest` library ignore (#1626)
- Re-work "Fan Only" strings & matching. (#1610)
- Address `C0209` pylint warnings. (#1608)
@crankyoldgit
Copy link
Owner

FYI, the changes mentioned above have now been included in the new v2.8.0 release of the library.

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

Successfully merging a pull request may close this issue.

2 participants