Suggestion for Lower Memory Usage

[Pecacheu]

This isn't so much an issue but I have a question. This library really seems to be a massive ROM/RAM hog and I think part of the reason is all those non-PROGMEM global const vars, why use them instead of macros? Such things as the long lists of const uint8_t kDefaultBlahBlah = x found in all the header files.

RAM is a precious resource on ESP CPUs, and while this lib is well within the limits (so this is hardly of immidiate concern), I think it could be improved if the global vars were kept to a minimum, using compiler macro #defines or PROGMEM where possible.

[NiKiZe]

You might want to look into all the work that already has gone into efforts relating to the size.

Please check what is actually included in your build, maybe you should poke your compiler to do the right thing.

[Pecacheu]

Are you saying that those consts don't get compiled into the global namespace to take up space even if they aren't used? Because on my ESP, with default Arduino settings, GDB seems to disagree.

Besides, even for the ones that are used, they could be taking up zero global space if they were a compiler constant instead, it'd only hog memory while calling functions that say, have default parameters set, which would be initialized to that value at runtime (assuming no further optimization occurs), then disposed afterwards.

I mean, I appreciate it if work has already gone in to optimizing this library, and it works well, but I'm just making a suggestion. I haven't observed every inch of the code, so I don't know if there's another reason it has to be done this way. If that's the case then fine, but generally speaking, this is well known as a bad practice in C, especially on embedded systems.

There's a reason that most libraries don't use globals in this way, including the ESP's own SDK libs which use defines and enums. C compilers just can't be trusted to optimize globals well, partly due to the way linking works.

I mean for my main project, I have 6 or 7 other libraries, including OTA code and an HTTP page in PROGMEM for establishing WiFi connection, yet my code was only taking up 28% space and 20% RAM (for globals). This library (IR send + receive) literally doubled that all the way to 40% and 42%! (Granted that includes some extra Serial printing and reading code to go along with, but that shouldn't fill too much space, as I already had a fair bit of Serial code, all strings in PROGMEM of course)

[NiKiZe]

Some documentation
https://github.com/crankyoldgit/IRremoteESP8266#upgrading-from-pre-v25

[crankyoldgit]

This isn't so much an issue but I have a question. This library really seems to be a massive ROM/RAM hog and I think part of the reason is all those non-PROGMEM global const vars, why use them instead of macros? Such things as the long lists of const uint8_t kDefaultBlahBlah = x found in all the header files.

Depending how you use the library, yes it is a huge RAM/ROM hog. There is a shirt-tonne of code in it. As you indicate in a latter comment, you're using the decoder (IRrecv class) which is the most expensive, ram, flash PROGMEM, & CPU-wise.

To reduce the memory/flash/cpu footprint, take a look at these options/comments:

IRremoteESP8266/src/IRremoteESP8266.h

Lines 62 to 87 in 9573cb2

	// Do we enable all the protocols by default (true), or disable them (false)?
	// This allows users of the library to disable or enable all protocols at
	// compile-time with `-D_IR_ENABLE_DEFAULT_=true` or
	// `-D_IR_ENABLE_DEFAULT_=false` compiler flags respectively.
	// Everything is included by default.
	// e.g. If you only want to enable use of he NEC protocol to save program space,
	// you would use something like:
	// `-D_IR_ENABLE_DEFAULT_=false -DDECODE_NEC=true -DSEND_NEC=true`
	//
	// or alter your 'platform.ini' file accordingly:
	// ```
	// build_flags = -D_IR_ENABLE_DEFAULT_=false
	// -DDECODE_NEC=true
	// -DSEND_NEC=true
	// ```
	// If you want to enable support for every protocol *except* _decoding_ the
	// Kelvinator protocol, you would use:
	// `-DDECODE_KELVINATOR=false`
	#ifndef _IR_ENABLE_DEFAULT_
	#define _IR_ENABLE_DEFAULT_ true // Unless set externally, the default is on.
	#endif // _IR_ENABLE_DEFAULT_
	// Supported IR protocols
	// Each protocol you include costs memory and, during decode, costs time
	// Disable (set to false) all the protocols you do not need/want!
	// The Air Conditioner protocols are the most expensive memory-wise.

Using the IRac class is the most expensive of all, as it pretty much includes/uses all the text symbols/objects & code.
Next is using an A/C protocol via it's specific class object. e.g. IRKelvinatorAC, especially if you use A/C class's .toString() method

If you use the IRrecv::decode() method without making any changes to what protocols are supported, then you're including a huge amount of code. i.e. EVERY protocol decoder. Also study that method/class, as there are memory tuning options in there too.

For sending, it's similar. IRac is by far the most expensive, then the individual A/C class, then the lower level IRsend::send() method, which includes ALL of the supported protocols code, then last but not least, the low-level IRsend::sendBlah() which is the least expensive flash/memory wise.

Are you saying that those consts don't get compiled into the global namespace to take up space even if they aren't used? Because on my ESP, with default Arduino settings, GDB seems to disagree.

Now, to your "const vs DEFINE" point. The compiler should be treating these as literals and putting them in the symbol table. They should (in theory) not be taking up stack/variable space. They are(should be) behaving the same as a #DEFINE except they are typed, thus safer. If you're using it with a debugger (e.g. gdb), you're likely including the symbol table with the resultant debug binary, thus including a heap of bloat with your binary. You can't compare a debug binary's memory using with a stripped binary's size/memory usage etc. Also note, namespace is not the same as the global variable space. You may be conflating two different things.

If they were true global variables, yes, I'd agree with you 100%, but const(ants) are not variables.
If g++/the ESP is storing them in memory, or in the global (in-ram) variable memory, rather than flash space, then yes. There is probably an argument for using PROGMEM on all the constants. If you can prove/show me with hard data that is the case for a stripped binary then I'll gladly do that to save space. The intention is for these literals to not take up variable space & to increase code readability.
Note: If a constant/literal isn't used it shouldn't be included in the final binary. i.e. It should be excluded during the object code linking phase of building the executable at the latest, if not prior to that.

[crankyoldgit]

I mean, I appreciate it if work has already gone in to optimizing this library, and it works well, but I'm just making a suggestion. I haven't observed every inch of the code, so I don't know if there's another reason it has to be done this way. If that's the case then fine, but generally speaking, this is well known as a bad practice in C, especially on embedded systems.

FYI, I came across this today on the Arduino Reference site:
Constants defined with the const keyword obey the rules of variable scoping that govern other variables. This, and the pitfalls of using #define, makes the const keyword a superior method for defining constants and is preferred over using #define.
&
In general, the const keyword is preferred for defining constants and should be used instead of #define.

[crankyoldgit]

FWIW, I just did some tests with const variables with variable attribute of PROGMEM over a large portion of the library.
e.g.

const uint64_t kEcoclimDefaultState PROGMEM = 0x11063000FFFF02;

or

const static uint64_t kEcoclimDefaultState PROGMEM = 0x11063000FFFF02;

or

const PROGMEM uint64_t kEcoclimDefaultState = 0x11063000FFFF02;

There was zero change in the size of the RAM use.
i.e.

IRMQTTServer compiled for the D1_mini:
Without:

RAM:   [=====     ]  52.9% (used 43372 bytes from 81920 bytes)
Flash: [=====     ]  52.5% (used 548792 bytes from 1044464 bytes)

With:

RAM:   [=====     ]  52.9% (used 43372 bytes from 81920 bytes)
Flash: [=====     ]  52.5% (used 548792 bytes from 1044464 bytes)

IRrecvDumpV2 compiled for en-AU:
Without:

RAM:   [====      ]  36.9% (used 30216 bytes from 81920 bytes)
Flash: [===       ]  34.9% (used 364940 bytes from 1044464 bytes)

With:

RAM:   [====      ]  36.9% (used 30216 bytes from 81920 bytes)
Flash: [===       ]  34.9% (used 364940 bytes from 1044464 bytes)

So to me, the data/evidence says either the compiler is doing the correct thing already (as expected), or PROGMEM makes no difference to "global" constants. 🤷 I'm out of ideas.

[Pecacheu]

@crankyoldgit That's weird, considering that GDB found that those const variables inhabited the RAM address space and were all taking up room there. But keep in mind, the compiler's RAM usage numbers are just an estimate, they're different than the true numbers that you get at runtime.

It could be that PROGMEM actually does improve the situation at runtime (#define did for me when I did a CTRL+F replace). I would assume that a PROGMEM const would be just as good as define with none of the pitfalls.

Though frankly there's a lot of messy code in this library and bugs I've had with fragmentation after running for a long time, not to mention the codes in the AC class for the LG AC not being quite right for some of the functions, despite being for the exact same remote model I have, so I had to fix them. Oh yeah and a fahrenheit mode was missing too and it was a pain to implement because the library wasn't really set up with the best practices of code modularity. And that's depsite that craptastic Google code style guide (yeah read though that thing and it was an absolute disaster... All I'll say is, no wonder Stadia was a mess on launch lol)

The other thing is you can't dynamically load in/out different classes of device/AC, or even use multiple at once on the same IR instance, which seems like a fairly big oversight and was something I need to do. At this rate I'm considering just stealing the decoders I need (with credit of course) and putting them in a more basic IR library.

[crankyoldgit]

@crankyoldgit That's weird, considering that GDB found that those const variables inhabited the RAM address space and were all taking up room there. But keep in mind, the compiler's RAM usage numbers are just an estimate, they're different than the true numbers that you get at runtime.

Like I said earlier, are you including the symbol table (i.e. a non-stripped binary) when you're running GDB? That might be your issue.
I spent a fare few hours yesterday going over ways to reduce memory usage, espectially SRAM etc on the arduino platform/framework. We do most/all of what they suggest. A lot of the guides seem to indicate the compiler's RAM estimates are accurate, given that is typically the block/section of the binary that is loaded directly into memory for "global" / SRAM memory at the start of execution. Thus, it makes sense that value is accurate.

It could be that PROGMEM actually does improve the situation at runtime (#define did for me when I did a CTRL+F replace). I would assume that a PROGMEM const would be just as good as define with none of the pitfalls.

It could be the PROGMEM variable attribute just doesn't work on Integers etc. Just on strings/arrays. i.e. It's not forcing those values to be stored in flash.
The best way to see what is going on is to disassemble the final(stripped) binary and see how it is referencing the values.

Though frankly there's a lot of messy code in this library and bugs I've had with fragmentation after running for a long time, not to mention the codes in the AC class for the LG AC not being quite right for some of the functions, despite being for the exact same remote model I have, so I had to fix them.

Well, this is open source, feel free to contribute back the fixes. I look forward to your PRs.

I am curious about the fragmentation issues you are referring to. There is very little dynamic memory usage in the library. The only fragmentation issues we've ever had reported are with the toString() functions. Which I thought I had dealt with mostly.
If you can isolate or even point to the area where it is happening, I'll try to address it.

Oh yeah and a fahrenheit mode was missing too and it was a pain to implement because the library wasn't really set up with the best practices of code modularity.

There are a number of protocols that have support Fahrenheit, you can see from those how I tend to handle Fahrenheit. If no one provides raw data for Fahrenheit it is kind of hard to reverse engineer it. Turns out people using these devices tend to come from places that don't use Fahrenheit. I wonder why that is. ;-)

And that's depsite that craptastic Google code style guide (yeah read though that thing and it was an absolute disaster... All I'll say is, no wonder Stadia was a mess on launch lol)

Well, it is one of the better public style guides out there. None are perfect. Better to have a style guide than no style guide IMHO. Perhaps you've better. I'm not sure you can blame Stadia on the style guide. ;-)

The other thing is you can't dynamically load in/out different classes of device/AC, or even use multiple at once on the same IR instance, which seems like a fairly big oversight and was something I need to do.

We could add the ability to dynamically enable/disable protocols, but that turns out to be next to useless. As the code has to be stored in the flash, and flash space when all the "modules" are compiled is the main issue people have. i.e. limited flash space.
If you have a way of "loading in/out" code without storing it in flash, I'm all ears. I've been unable to come up with one that is acceptable. Loading in remote interpreted code across a network isn't an acceptable solution IMHO. ;-)

I'm not sure what you're doing, but you can have multiple classes/protocols/etc on the same "IR instance". The library doesn't stop you in any way with respect to supporting/controlling multiple devices from a single ESP chip or via a single IR led.
i.e. I control at least three devices using different protocols from the same "instance".

At this rate I'm considering just stealing the decoders I need (with credit of course) and putting them in a more basic IR library.

Feel free. It's open source, after all. Please respect the LGPL license. There is plenty of scope to create a simpler/smaller send-only library. I wish you luck.

[Pecacheu]

Like I said earlier, are you including the symbol table (i.e. a non-stripped binary) when you're running GDB? That might be your issue.
I spent a fare few hours yesterday going over ways to reduce memory usage, espectially SRAM etc on the arduino platform/framework. We do most/all of what they suggest. A lot of the guides seem to indicate the compiler's RAM estimates are accurate, given that is typically the block/section of the binary that is loaded directly into memory for "global" / SRAM memory at the start of execution. Thus, it makes sense that value is accurate.

I guess it's possible that the act of including GDB affects the program. Though what I meant by the RAM estimate not being accurate is, I've noticed that in many programs the readout of the RAM usage at runtime, even right at the start of the program, can be much higher than the estimate. I don't think the compiler can really predict various dynamic allocations that go on during initializations of classes and libraries, etc.

Well, it is one of the better public style guides out there. None are perfect. Better to have a style guide than no style guide IMHO. Perhaps you've better. I'm not sure you can blame Stadia on the style guide. ;-)

Heh, I suppose that's true. But hey I just like making fun of Statia lol

I am curious about the fragmentation issues you are referring to. There is very little dynamic memory usage in the library. The only fragmentation issues we've ever had reported are with the toString() functions.

That's what I think is happening cause I'm using some of the debug functions of the library related to the IRRecv, and I've noticed that if I go crazy pressing buttons on my remote for 20 minutes straight I can start getting issues much faster. So my theory is that this miiiight be more the fault of Arduino's String class than your library's fault. I noticed that the library does try to do some pre-allocation of the strings to prevent fragmentation. I'm sure that helps but it seems like that... isn't perfect.

I'm not sure what you're doing, but you can have multiple classes/protocols/etc on the same "IR instance". The library doesn't stop you in any way with respect to supporting/controlling multiple devices from a single ESP chip or via a single IR led.

It does though, the IRSend instance in each protocol class is a private variable you can't access. And maybe I'm doing something wrong but having more than one IRSend for the same IR LED seems to conflict with each other. Whereas when I modify the library to make the IRSend instance public, and use the same one for multiple protocols, it does work.

If you have a way of "loading in/out" code without storing it in flash, I'm all ears. I've been unable to come up with one that is acceptable. Loading in remote interpreted code across a network isn't an acceptable solution IMHO. ;-)

That's true, like literally loading a binary of each module over the network would be all kinds of insane, but I am trying to think of a way to do something like that... Like being able to dynamically add support for a new protocol without a full OTA update would be nice, through some sort of like... JSON file type thing. But then the parsing for that would take up space and all but idk... Maybe a silly/dumb feature lol

[crankyoldgit]

I guess it's possible that the act of including GDB affects the program. Though what I meant by the RAM estimate not being accurate is, I've noticed that in many programs the readout of the RAM usage at runtime, even right at the start of the program, can be much higher than the estimate. I don't think the compiler can really predict various dynamic allocations that go on during initializations of classes and libraries, etc.

The "RAM Estimate" is the floor value. It's all the globals and statics in the code. The RAM usage during runtime will be higher and of course can't be predicted accurately via static analysis. Dynamic memory, stack usage etc is not included in that estimate. Globals etc, can be accurately predicted, well, the minimum at least.

I am curious about the fragmentation issues you are referring to. There is very little dynamic memory usage in the library. The only fragmentation issues we've ever had reported are with the toString() functions.

That's what I think is happening cause I'm using some of the debug functions of the library related to the IRRecv, and I've noticed that if I go crazy pressing buttons on my remote for 20 minutes straight I can start getting issues much faster. So my theory is that this miiiight be more the fault of Arduino's String class than your library's fault. I noticed that the library does try to do some pre-allocation of the strings to prevent fragmentation. I'm sure that helps but it seems like that... isn't perfect.

I've no idea what you're doing with the code so I can't comment, and if you've enabled DEBUG well, all bets are off. It's not exactly designed to run optimally with that enabled. ;-)

I've just added commit 14e0e42 that does some more string reservation etc to try to reduce fragmentation if you are using some of the routines used by the examples etc. But, as I've got no idea what you're doing, that's a long shot.
It could be the Arduino String library, it could be the Serial library, it could be your code, it could be the libraries. Without concrete examples it's hard to tell. e.g. If you are constantly sending a stream of IR to IRrecvDumpV2, of course it can't keep up as it's limited to the serial baud rate. You can easily saturate that.

It does though, the IRSend instance in each protocol class is a private variable you can't access. And maybe I'm doing something wrong but having more than one IRSend for the same IR LED seems to conflict with each other. Whereas when I modify the library to make the IRSend instance public, and use the same one for multiple protocols, it does work.

If you can provide a simplified case where this happens, please log it as an issue. Off the top of my head, I can't see why that wouldn't work as expected. (Unless you were trying to send two messages simultaneously, producing garbage)
After all, all it is doing is bit-banging the GPIO. Nothing fancy at all.

[crankyoldgit]

That's true, like literally loading a binary of each module over the network would be all kinds of insane, but I am trying to think of a way to do something like that... Like being able to dynamically add support for a new protocol without a full OTA update would be nice, through some sort of like... JSON file type thing. But then the parsing for that would take up space and all but idk... Maybe a silly/dumb feature lol

I can see a "Parse a spec and implement it" method for the simple protocols. i.e. send X many bits in a certain way. But for the complexities of the A/C protocols, I can't see it being implemented without a Turing complete language of sorts. Bags not implementing that on an ESP chip. ;-)