Strategy for handling the maybe-terminated-fixed-char-arrays

[wimalopaan]

All over the code-base _maybe-terminated-fixed-char-array_s (MTFCA) are used, and these do (and will do) cause bugs. In my understanding they were introduced mainly for the small radios with small amount of flash and - more severe - little RAM. They are a tradeoff between computational-intense/flash-use and RAM (stack/heap) usage, since they save one-byte (sentinel) per string in the expense of always counting for the case the string may or may not contain the sentinel.

The most radical strategy to achieve less bugs would be to eleminate these MTFCA at all! But it is not to my knowledge if this would break the support for the smaller radios.

I'm very new to EdgeTx, but I see (and fixed of them) the main problems here:

• string-fiddeling (mostly concatination) of MTFCA
• copying to these MTFCA
• copying from and constructing std::string of MTFCA

With a fairly amount of carefulness the above problems are not real problems, but ...

To give an example consider this:

https://github.com/wimalopaan/edgetx/blob/8f1a3907a5ad8db0bf5ca0f3008d46ff23f1720b/radio/src/lua/interface.cpp#L710-L713

This is extremely error prone beause of manually calculating the (minimum) needed size of the composition buffer and the relationship between the various string-literals, MTFCAs ans their corresponding maximum sizes.

A better solution would be this:

https://github.com/wimalopaan/edgetx/blob/8f1a3907a5ad8db0bf5ca0f3008d46ff23f1720b/radio/src/lua/interface.cpp#L675-L678

Still this is error prone because mainly of the same reasons as above.

Another (more subtle) example would be this one:

https://github.com/wimalopaan/edgetx/blob/8f1a3907a5ad8db0bf5ca0f3008d46ff23f1720b/radio/src/gui/colorlcd/view_main_decoration.cpp#L330-L332

Here, if the MTFCA is fully occupied (no sentinel) the std::string is filled with garbage at the end and may occupy a huge amount of RAM. The correction of that requires some carfulness because the obviuos c-tor or std::string(const char*, size_t) does not the complete right thing, because it always copies a fixed amount of chars, regardless of an occurance of a sintinal or not (the resulting std::string then contains the senstinel in the middle.

A solution would be (here commented): https://github.com/wimalopaan/edgetx/blob/b9319c5428aab118b9473e98fcde996355333c8c/radio/src/gui/colorlcd/view_main_decoration.cpp#L332-L333

But this again leaves some work to the programmer.

Quite better would be: https://github.com/wimalopaan/edgetx/blob/b9319c5428aab118b9473e98fcde996355333c8c/radio/src/gui/colorlcd/view_main_decoration.cpp#L334 with https://github.com/wimalopaan/edgetx/blob/b9319c5428aab118b9473e98fcde996355333c8c/radio/src/strhelpers.h#L95-L98 where the compile would deduce the MTFCA length. Still this is not optimal because strnlen() and the ctor of std::string iterate over the array.

The use of templates may result in substantial code-bloat because this function-template will be instantiated with every MTFCA-length that it is called with.

A far better approach from the user-side would be (for first problem above): https://github.com/wimalopaan/edgetx/blob/8f1a3907a5ad8db0bf5ca0f3008d46ff23f1720b/radio/src/lua/interface.cpp#L672-L673

But this is only an example, not how it should be

Here the compiler calculates at compile-time all required data-types (here: std::array<> with correct size), deduces the type of argument (string-literal which comprises of an sentinel) or MTFCA, and the concat() variadic generic function does the concatenation.

But this comes with two (severe or not) drawbacks:

• a fairly amount of template-meta-programming
• an unpredictable amount of code-bloat (flash, not RAM)

Because EdgeTx only uses up to C++11, this can be quite cumbersome: https://github.com/wimalopaan/edgetx/blob/8f1a3907a5ad8db0bf5ca0f3008d46ff23f1720b/radio/src/strhelpers.h#L96-L273

The amount of code-bloat depends on the amount of different template-instantiations. But here we have a variadic function, so the number of instantiation is the number of all permutations of parameter-types. But to my knowledge and experience with very heavy use of TMP in microcontrollers, modern compilers are very good in optimizations of templated-code (because the see the full definition and not only the interface, unless lto (link-time optimization is used).

Ok, looks like an long write-up (my hope is you don't say TL;DR).

[gagarinlg]

All over the code-base _maybe-terminated-fixed-char-array_s (MTFCA) are used, and these do (and will do) cause bugs.

I totally agree.

With a fairly amount of carefulness the above problems are not real problems, but ...

This can and will go wrong and can be mised in a review.

https://github.com/wimalopaan/edgetx/blob/8f1a3907a5ad8db0bf5ca0f3008d46ff23f1720b/radio/src/gui/colorlcd/view_main_decoration.cpp#L330-L332 Here, if the MTFCA is fully occupied (no sentinel) the std::string is filled with garbage at the end and may occupy a huge amount of RAM. The correction of that requires some carfulness because the obviuos c-tor or std::string(const char*, size_t) does not the complete right thing, because it always copies a fixed amount of chars, regardless of an occurance of a sintinal or not (the resulting std::string then contains the senstinel in the middle.

Also text.length() will give you the number of copied bytes. You would have to use strlen(text.c_str()) to get the correct length, which is by design as std::string can be used as a data storage.

But this comes with two (severe or not) drawbacks: a fairly amount of template-meta-programming

I have no problem with this

an unpredictable amount of code-bloat (flash, not RAM)

This could be more of a problem. A few weeks ao we introduced RLE for images on the higher resolution greyscale LCD screens, because the flash was full.

Because EdgeTx only uses up to C++11, this can be quite cumbersome:

The compilers are all new enough and everything that works with C++11 should also work in C++17. Maybe you can just try compiling with C++17 enabled?

The amount of code-bloat depends on the amount of different template-instantiations. But here we have a variadic function, so the number of instantiation is the number of all permutations of parameter-types. But to my knowledge and experience with very heavy use of TMP in microcontrollers, modern compilers are very good in optimizations of templated-code (because the see the full definition and not only the interface, unless lto (link-time optimization is used).

Yes, the code is normally very well optimized. Just debugging can get very difficult. When stepping the call stack can get prety large, even if it is just a one-liner in assembly, because all the intermediate types/instatiations are in the debug information, even though they are completely optimized out in the end.

[wimalopaan]

Because EdgeTx only uses up to C++11, this can be quite cumbersome:

The compilers are all new enough and everything that works with C++11 should also work in C++17. Maybe you can just try compiling with C++17 enabled?

If you take a look at the proposed solution you'll see that I provided a C++11 and alternatively a >=C++17 variant. And yes, one can freely set the C++-Standard in der CMakeList.txt to 17 oder 20, that works.

[wimalopaan]

One thing I did not mention in the above text is to change the type of the MTFCA from char[N] to std::array<char,N> as this would be a hard change (all using code breaks). This would require the user to change all usages of these arrays to safer functions.

[jfrickmann]

I fully support a strategy for handling strings and memory, but I do not feel qualified to make recommendations here!

[wimalopaan]

I looked a bit deeper into other places of the code and saw very inconsistent use of MTFCA, std::string and null-terminated char-arrays. And some unneccessary use of other global objects which makes we worry a bit in the context of a MT-system.

Well, I'll try to make a suggestion for a small subsystem as the mixer-scripts because this was my starting point for digging into the code-base.