[P016] DecodeType UNKNOWN was not on the web interface (decodetypes UNKNOWN and UNUSED were mixed up!) #4907

uwekaditz · 2023-12-11T12:45:56Z

NEW: For using any IR message in the [P016] Command map the protocol RAW needs to be set
FIX: uint64ToString() in a debug message in PLUGIN_TEN_PER_SECOND can not handle decode_type_t::UNKNOWN (-1)
@tonhuisman I'm not sure if DECODE_HASH should get its own protocol?

TD-er · 2023-12-11T13:05:08Z

src/_P016_IR.ino

+                }
+                else {
+                  Log += F("\' type: 0x");
+                  Log += uint64ToString(results.decode_type);


You don't have a base argument here, so it isn't a proper hex value and thus the leading 0x in the string is a bit misleading to say the least.

In ESPEasy src/src/Helpers/StringConverter_Numerical.h there are ull2String and ll2String to print uint64_t and int64_t respectively.

Also IMHO there should be no negative hex values, so better not mark it as a hex formatted value (leading 0x)

tonhuisman · 2023-12-29T19:19:43Z

@tonhuisman I'm not sure if DECODE_HASH should get its own protocol?

Hm, though I've done quite some work on this plugin and the IR library, I'm absolutely no expert on the matter, so that question should probably be redirected to the maintainers of the IRremoteESP8266 library.

uwekaditz · 2024-01-15T20:46:06Z

@TD-er and @tonhuisman:
After the last two merges any command that is an oledframedcmd results with
line 226 ExecuteCommand_all(EventValueSource::Enum::VALUE_SOURCE_SYSTEM, CommandLines[i].Command);
in P016_data_struct::ExecuteCode() in an exception and a restart of the whole system.
I didn't change anything and before merging everything run fine.
Note:
The function P016_data_struct::ExecuteCode() is called within the PLUGIN_TEN_PER_SECOND but it was no problem before.

I have already tried

String(CommandLines[i].Command).c_str() in the function call -> still exception
checked that all commands used in my [P016] setting are correctly terminated by a zero (0x00)
running another command (e.g. TaskValueSet,Temp,TempA,1.0) -> runs fine
using a different EventValueSource (e.g. EventValueSource::Enum::VALUE_SOURCE_SERIAL)-> still exception
tested each oledframedcmd used in my [P016] setting in Tools->Command and Submit -> runs fine

What is wrong?

tonhuisman · 2024-01-15T21:12:10Z

line 226 ExecuteCommand_all(EventValueSource::Enum::VALUE_SOURCE_SYSTEM, CommandLines[i].Command);
in P016_data_struct::ExecuteCode() in an exception and a restart of the whole system.

Can you get a stacktrace of that exception? (When using VSCode/PIO, using the Monitor feature, it will try to catch that when the unit crashes, though it may take a few attempts for the monitor to actually grab the stacktrace...)

As reported here: #4907 (comment)

tonhuisman · 2024-01-15T21:42:44Z

There seem to be 2 potential issues:

The parsing of the command doesn't decently handle an empty command (spaces only, f.e.) (fixed by TD-er PR, you can cherry-pick that)
The plugin code doesn't a) check the vector size when checking for commands, and b) if the command is really not empty

Size check could be fixed by this change: (line 222 of P016_data_struct.cpp)

      if ((i < CommandLines.size()) && (CommandLines[i].Command[0] != 0)) {

The empty command check could be done at saving the commands, didn't try to fix that yet

…command is really not empty

uwekaditz · 2024-01-18T20:32:39Z

I did the changes @tonhuisman proposed and merged the latest mega (Fix crash in command arg parsing with nullptr string) into this PR.
The system stll crashes if any oledframedcmd is execute by receiving a defined IR code.

For me it looks like a stack overflow because the crash dump not even fits in the VS terminal window.
I captured one crash, but I'm not sure if the decoding started at the beginning of the dump because I can't see the beginning.
Here is also the result after building

RAM:   [======    ]  56.8% (used 46536 bytes from 81920 bytes)
Flash: [========= ]  90.7% (used 947056 bytes from 1044464 bytes)

Crash dump.txt

tonhuisman · 2024-01-18T20:43:24Z

For me it looks like a stack overflow because the crash dump not even fits in the VS terminal window.

That's a very plausible cause, as both P016 and P036 are using quite a lot of memory and stack.
One way of reducing the size of P016 is to copy CustomIR-sample.h to CustomIR.h (this file is in .gitignore, so you can't commit it by accident), disable any unneeded IR protocols and create a Custom IR build. That could improve things.

uwekaditz · 2024-01-18T21:15:21Z

For me it looks like a stack overflow because the crash dump not even fits in the VS terminal window.

That's a very plausible cause, as both P016 and P036 are using quite a lot of memory and stack. One way of reducing the size of P016 is to copy CustomIR-sample.h to CustomIR.h (this file is in .gitignore, so you can't commit it by accident), disable any unneeded IR protocols and create a Custom IR build. That could improve things.

I always have my own CustomIR.h, only DECODE_HASH and DECODE_NEC are set.
CustomIR.h.txt

tonhuisman · 2024-01-18T21:18:56Z

Would it be possible for you to test this on an ESP32, to determine if it's a code-issue or 'simply' stack related?

tonhuisman · 2024-01-18T21:24:38Z

Is your oledframedcmd command directly executed from the IR plugin, or have you moved it to rules, by using event,thisIRevent as an intermediate?

TD-er · 2024-01-18T21:29:05Z

For me it looks like a stack overflow because the crash dump not even fits in the VS terminal window.

That's a very plausible cause, as both P016 and P036 are using quite a lot of memory and stack. One way of reducing the size of P016 is to copy CustomIR-sample.h to CustomIR.h (this file is in .gitignore, so you can't commit it by accident), disable any unneeded IR protocols and create a Custom IR build. That could improve things.

I always have my own CustomIR.h, only DECODE_HASH and DECODE_NEC are set. CustomIR.h.txt

This file doesn't have P036 enabled?

Which command do you give, how it is it being processed? (e.g received via MQTT, from rules, from HTTP-GET, via serial?)

uwekaditz · 2024-01-19T18:14:37Z

For me it looks like a stack overflow because the crash dump not even fits in the VS terminal window.

That's a very plausible cause, as both P016 and P036 are using quite a lot of memory and stack. One way of reducing the size of P016 is to copy CustomIR-sample.h to CustomIR.h (this file is in .gitignore, so you can't commit it by accident), disable any unneeded IR protocols and create a Custom IR build. That could improve things.

I always have my own CustomIR.h, only DECODE_HASH and DECODE_NEC are set. CustomIR.h.txt

This file doesn't have P036 enabled?

Which command do you give, how it is it being processed? (e.g received via MQTT, from rules, from HTTP-GET, via serial?)

The P036 is enabled in my custom.h.

As I mentioned above the oledframedcmd is processed by
ExecuteCommand_all(EventValueSource::Enum::VALUE_SOURCE_SYSTEM, CommandLines[i].Command);
in P016_data_struct::ExecuteCode() (line 226).
The function P016_data_struct::ExecuteCode() is called within the PLUGIN_TEN_PER_SECOND but it was no problem before the merges.

uwekaditz · 2024-01-19T18:16:01Z

Is your oledframedcmd command directly executed from the IR plugin, or have you moved it to rules, by using event,thisIRevent as an intermediate?

As I mentioned above the oledframedcmd is processed by
ExecuteCommand_all(EventValueSource::Enum::VALUE_SOURCE_SYSTEM, CommandLines[i].Command);
in P016_data_struct::ExecuteCode() (line 226).
The function P016_data_struct::ExecuteCode() is called within the PLUGIN_TEN_PER_SECOND but it was no problem before the merges.

TD-er · 2024-01-19T19:02:32Z

In that function, there is a stack allocated object, which is relatively large:

case PLUGIN_TEN_PER_SECOND:
    {
      decode_results results;

This is 80 bytes.

But anyway, what is the origin of the oledframedcmd command?
Where does it come from?
Do you trigger events in the rules when handling the event?

uwekaditz · 2024-01-19T19:05:33Z

I also have also a modiefied section in platformio_esp82xx_envs.ini to enable custom.h:

[env:custom_IR_ESP8266_4M1M]
extends                   = esp8266_4M1M
platform                  = ${ir.platform}
platform_packages         = ${ir.platform_packages}
build_flags               = ${ir.build_flags} 
                            ${esp8266_4M1M.build_flags} 
                            -DPLUGIN_BUILD_CUSTOM
; commented                           -DPLUGIN_BUILD_IR
; following two line added
                            -D_IR_ENABLE_DEFAULT_=false
                            -DUSE_IR
lib_ignore                = ESP32_ping
                            ESP32WebServer
                            ServoESP32
                            ESP32HTTPUpdateServer
                            adafruit/Adafruit GFX Library@^1.11.1
                            LOLIN_EPD
                            Adafruit ILI9341 ESPEasy
                            adafruit/Adafruit BusIO
                            Adafruit NeoPixel
                            NeoPixelBus_wrapper
                            NeoPixelBus by Makuna
                            Adafruit NeoMatrix via NeoPixelBus
                            Adafruit Motor Shield V2 Library
                            Adafruit_ST77xx
                            Adafruit NeoMatrix
                            I2C AXP192 Power management
                            EspSoftwareSerial
; following line added
                            HeatpumpIR
extra_scripts             = pre:tools/pio/pre_custom_esp82xx_IR.py
                            ${extra_scripts_esp8266.extra_scripts}
                            pre:tools/pio/ir_build_check.py

uwekaditz · 2024-01-19T19:20:32Z

In that function, there is a stack allocated object, which is relatively large:
case PLUGIN_TEN_PER_SECOND:
    {
      decode_results results;
This is 80 bytes.

But anyway, what is the origin of the oledframedcmd command? Where does it come from? Do you trigger events in the rules when handling the event?

The oledframe commands are defined inside the P016 settings (see jpg)

TD-er · 2024-01-19T20:02:07Z

OK, and it is when using this exact pull request?

uwekaditz · 2024-01-19T20:11:55Z

Yes, it is except my custom.h

TD-er · 2024-01-19T20:13:42Z

I will have a look at your code to see if things can be improved or maybe something a-typical is happening when receiving IR commands.

uwekaditz

Typo error in the first commit
@TD-er I'm not sure if I should merge your branch, too.
Therefore I reverted the merge.

TD-er · 2024-01-22T21:51:35Z

Typo error in the first commit @TD-er I'm not sure if I should merge your branch, too. Therefore I reverted the merge.

Ehh now you really lost me...
I spent all evening/night yesterday in reducing the stack scope of P016 and today about 6 hours fixing out-of-bound issues on the OLED display library and now those must be reverted?

Why?????

TD-er · 2024-01-22T23:15:42Z

I found the first commit in your branch feature/ESP32_IDF_5_1 that causes the crashes. It is from 12.11.2023 18:22 marked as

[ESP8266 Setup] Fix bootloop during initial setup Custom ESP8266 build.

The changes of this commit are quit simple:

Adding a global boolean in _Plugin_init.cpp
bool _Plugin_init_setupDone = false;

and adding a local boolean in EspEasy_Console_t::reInit()
bool somethingChanged = false;

So you mean that with this commit it all became unstable?

d4c7252

tonhuisman · 2024-01-23T07:32:53Z

src/_P016_IR.ino

-              log += description;
-              addLogMove(LOG_LEVEL_INFO, log);
-            }
+            addLogMove(LOG_LEVEL_INFO, strformat(F("AC State: %s"), description));


This fails compilation as you have to pass the .c_str() pointer for strformat to handle in the %s parameter.

uwekaditz · 2024-01-23T08:03:08Z

I know, but I couldn’t change so far. In my local build I do not test all #defines, that’s why I didn’t encounter the error.

uwekaditz · 2024-01-23T08:08:56Z

Yes, at least with my local build. I think in the context of drained resources, the additional booleans serve as the proverbial straw that breaks the camel's back.

uwekaditz · 2024-01-23T08:14:27Z

I'm sorry that I made you desperate. The changes are not lost! I was not sure if I should merge your changes to this PR or whether you wanted to do it.

TD-er · 2024-01-23T08:30:47Z

I already split the P036 code to another PR, so I guess we can keep this PR just about the P016.

I'm looking into the code a bit more and I'm still not 100% convinced it is a stack overflow.
But if it is, then we should make a separate "command queue" to be handled separately.
Just like the event queue.

When receiving commands from some external source, which does not need an immediate reply, then those could be executed in the next loop without any issue. So I guess we should add some queue for at least the P016 and maybe some others too.

If you could test this PR (with only the P036 code) to make sure I didn't break anything, then we can merge it and then merge it into this PR so we essentially move this PR to be on top of the other code changes.

uwekaditz · 2024-01-23T09:13:02Z

Sounds good. I will test it but may be not this week. I also had the idea to create dynamically the following structures in P036_data_struct.h to save ressources: Only for page scrolling: * tScrollLine SLine[P36_MAX_LinesPerPage]{}; (line 225) * tScrollingPageLines In[P36_MAX_LinesPerPage]{}; (line240) * tScrollingPageLines Out[P36_MAX_LinesPerPage]{}; (line 241) Only for ticker: * tTicker Ticker; (line 227) All of them have strings inside.

TD-er · 2024-01-23T13:03:40Z

@uwekaditz
I added a separate PR to allow to queue the commands to run from a lower position on the stack.
See: #4947

Use the new property addToQueue in ExecuteCommand_all() due to the lack of resources Using strformat() for the debug messages Heap and memory can be reported (P016_CHECK_HEAP)

uwekaditz · 2024-01-23T21:37:56Z

@TD-er Thank you very much 👋
Using the new property addToQueue in ExecuteCommand_all() works very well.

I tested all my [P016] setting using the oledframedcmd.
The system is also stable if the web interface is open.
Even saving the P016 settings on the devices page did not crash anymore.

tonhuisman · 2024-01-23T21:38:25Z

src/_P016_IR.ino

+# ifdef P016_CHECK_HEAP
+  if (loglevelActiveFor(LOG_LEVEL_INFO)) {
+    addLogMove(LOG_LEVEL_INFO, strformat(
+      F("Before decode: FreeMem: %d  FreeStack:%d / After: FreeMem: %d  FreeStack:%d"),
+      fMem, fFreeStack,
+      FreeMem(), getCurrentFreeStack()));
+  }
+# endif // ifdef P016_CHECK_HEAP
+


Hmm, not indenting the #ifdef's, and not formatting the source using Uncrustify, horribly breaks the indentation guides (lines) in modern editors like VSCode... 🤔

Sorry, I didn' uncrustify my changes. 😭
On the other hand it is much easier for me to read the code if the #ifdef's start at the beginning of the line.

to read the code if the #ifdef's start at the beginning of the line.

Nope, not for me, that totally breaks the normal flow of the code...

I used the Uncrustify function in VScode, but position of the #ifdef's were not changed!

That's not configured in the settings for Uncrustify 🥲 and I haven't found the courage to learn yet another complicated config...

That means it has to be done manually?

That's what I usually do, as I like my sourcecode prettified 😉

I hope you enjoy my code now too 😃

The position of the #ifdef's were not changed!

TD-er · 2024-01-23T21:51:04Z

@TD-er Thank you very much 👋 Using the new property addToQueue in ExecuteCommand_all() works very well.

I tested all my [P016] setting using the oledframedcmd.

The system is also stable if the web interface is open.

Even saving the P016 settings on the devices page did not crash anymore.

That's good to hear :)

The position of the #ifdef's were not changed!

uwekaditz · 2024-01-23T22:38:36Z

@tonhuisman I'm not sure if DECODE_HASH should get its own protocol?

Hm, though I've done quite some work on this plugin and the IR library, I'm absolutely no expert on the matter, so that question should probably be redirected to the maintainers of the IRremoteESP8266 library.

I created an issue about the protocol type for DECODE_HASH, see crankyoldgit/IRremoteESP8266#2057

uwekaditz · 2024-01-26T16:54:45Z

@tonhuisman I'm not sure if DECODE_HASH should get its own protocol?

Hm, though I've done quite some work on this plugin and the IR library, I'm absolutely no expert on the matter, so that question should probably be redirected to the maintainers of the IRremoteESP8266 library.

I created an issue about the protocol type for DECODE_HASH, see crankyoldgit/IRremoteESP8266#2057

The maintainers of the IRremoteESP8266 library do suggest not to use DECODE_HASH at all because it is not sure if the same key on an IR remote will always issue the same code (timing should be a big issue!).
I checked it on a cheap, noname Chinese IR remote used for Christmas illumination.
Over more than one year I get exactly the same code as hash. Even on a second IR of the same type I get the identical code.
I'm going to change this PR to use decode type UNKNOWN as the result of the hash decoding as it is defined in the IRremoteESP8266 library.

Up to now, in the P016 settings decode type UNKNOWN is also used to mark a valid command that should not be used.
This I'm going to change.

BUG: Decode types UNKNOWN and UNUSED were mixed up CHG: Reverted changes for this PR in \lib\IRremoteESP8266\src\IRtext.cpp CHG: Workaround for decode type UNKNOWN is not necessary CHG: Initalisation of the variables with decode type UNUSED (=0)

uwekaditz · 2024-01-26T19:13:17Z

At the end it was so simpel 😆

uwekaditz and others added 3 commits December 10, 2023 13:54

Merge tag 'mega-20231130' into UweKaditz

7efe1ab

[P016] Add protocol RAW to UI if 'Accept DecodeType UNKNOWN' is set

15dd104

Merge branch 'mega' into P016]-PullRequest

e876714

TD-er reviewed Dec 11, 2023

View reviewed changes

uwekaditz and others added 2 commits December 13, 2023 20:10

uint64ToString() changed to ll2String()

ea02bf2

Merge branch 'mega' into P016]-PullRequest

fbc1132

uwekaditz added 2 commits January 7, 2024 11:56

Merge branch 'mega' into P016]-PullRequest

9a58bbb

Merge branch 'mega' into P016]-PullRequest

7a0aa15

TD-er added a commit that referenced this pull request Jan 15, 2024

Fix crash in command arg parsing with nullptr string

66b6bb7

As reported here: #4907 (comment)

TD-er mentioned this pull request Jan 15, 2024

Fix crash in command arg parsing with nullptr string #4940

Closed

uwekaditz and others added 2 commits January 18, 2024 20:44

Merge branch 'mega' into P016]-PullRequest

f0c72a7

Check the vector size when checking for commands, and check that the …

15722e7

…command is really not empty

uwekaditz commented Jan 22, 2024

View reviewed changes

TD-er mentioned this pull request Jan 22, 2024

[Cleanup] Reduce stack usage + fix bound checks OLED Framed #4946

Closed

tonhuisman reviewed Jan 23, 2024

View reviewed changes

TD-er mentioned this pull request Jan 23, 2024

[Cleanup] Reduce stack usage + fix bound checks OLED Framed #4948

Merged

uwekaditz and others added 2 commits January 23, 2024 19:52

Merge branch 'mega' into P016]-PullRequest

704c39d

Use the new property addToQueue in ExecuteCommand_all()

769ae8c

Use the new property addToQueue in ExecuteCommand_all() due to the lack of resources Using strformat() for the debug messages Heap and memory can be reported (P016_CHECK_HEAP)

tonhuisman reviewed Jan 23, 2024

View reviewed changes

Uncrustify

951b1fb

The position of the #ifdef's were not changed!

Uncrustify #ifdef's

2077ee9

The position of the #ifdef's were not changed!

uwekaditz mentioned this pull request Jan 23, 2024

DECODE_HASH should get its own protocol type crankyoldgit/IRremoteESP8266#2057

Closed

Merge branch 'mega' into P016]-PullRequest

0c8f135

uwekaditz changed the title ~~[P016] Add protocol RAW to UI if 'Accept DecodeType UNKNOWN' is set~~ [P016] DecodeType UNKNOWN was not on the web interface (decodetypes UNKNOWN and UNUSED were mixed up!) Jan 26, 2024

uwekaditz added 3 commits January 29, 2024 15:51

Merge branch 'mega' into P016]-PullRequest

a32f0f9

Merge branch 'mega' into P016]-PullRequest

8533d03

Merge branch 'mega' into P016]-PullRequest

fae0205

TD-er merged commit 4463cfa into letscontrolit:mega Feb 11, 2024
168 checks passed

[P016] DecodeType UNKNOWN was not on the web interface (decodetypes UNKNOWN and UNUSED were mixed up!) #4907

[P016] DecodeType UNKNOWN was not on the web interface (decodetypes UNKNOWN and UNUSED were mixed up!) #4907

Conversation

uwekaditz commented Dec 11, 2023

TD-er Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonhuisman commented Dec 29, 2023 • edited Loading

uwekaditz commented Jan 15, 2024

tonhuisman commented Jan 15, 2024

tonhuisman commented Jan 15, 2024

uwekaditz commented Jan 18, 2024

tonhuisman commented Jan 18, 2024

uwekaditz commented Jan 18, 2024

tonhuisman commented Jan 18, 2024

tonhuisman commented Jan 18, 2024

TD-er commented Jan 18, 2024

uwekaditz commented Jan 19, 2024

uwekaditz commented Jan 19, 2024

TD-er commented Jan 19, 2024

uwekaditz commented Jan 19, 2024

uwekaditz commented Jan 19, 2024

TD-er commented Jan 19, 2024

uwekaditz commented Jan 19, 2024

TD-er commented Jan 19, 2024

uwekaditz left a comment • edited Loading

Choose a reason for hiding this comment

TD-er commented Jan 22, 2024

TD-er commented Jan 22, 2024

Choose a reason for hiding this comment

uwekaditz commented Jan 23, 2024 via email • edited Loading

uwekaditz commented Jan 23, 2024 via email • edited Loading

uwekaditz commented Jan 23, 2024 via email • edited Loading

TD-er commented Jan 23, 2024

uwekaditz commented Jan 23, 2024 via email • edited Loading

TD-er commented Jan 23, 2024

uwekaditz commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TD-er commented Jan 23, 2024

uwekaditz commented Jan 23, 2024

uwekaditz commented Jan 26, 2024

uwekaditz commented Jan 26, 2024

TD-er Dec 11, 2023 •

edited

Loading

tonhuisman commented Dec 29, 2023 •

edited

Loading

uwekaditz left a comment •

edited

Loading

uwekaditz commented Jan 23, 2024 via email •

edited

Loading

uwekaditz commented Jan 23, 2024 via email •

edited

Loading

uwekaditz commented Jan 23, 2024 via email •

edited

Loading

uwekaditz commented Jan 23, 2024 via email •

edited

Loading