End oappend

[willmmiles]

Replace the usage of oppend with Arduino's Print abstraction. This permits efficient use of dynamically allocated memory for returning settings page data, without requiring a single large contiguous allocation up front, or risking smashing the stack.

This change also permits the output generation functions to easily use more sophisticated serialization functions such as printf, reducing the number of required function calls. With this optimization, the overall result is approximate parity in code size to the original implementation. This has unfortunately come at a cost in code readability.

NOTE - this is a BREAKING CHANGE to the Usermod API!

Also note: to get the full benefits from ths PR, the AsyncWebServer's AsyncStreamResponse needs a better allocation implementation than it currently has. This improvement has not yet been merged upstream.

[blazoncek]

I like the outcome. I even went ahead and checked AWS and the implementation of DynamicBuffer. Nice!

I do not find the code less readable but it sure is odd to see dest.print() and sappend() in the same context. Would it make sense to make a derived class and wrap sappend() functionality into it to keep dest.print() format?

[blazoncek]

Why not use PSTR(...) and avoid casting?

[willmmiles]

In an in-between version of the code, I had it set up so the print of that element was PROGMEM aware. Given that this version just passes it in to printf as a '%s', you're correct that F() adds no extra value here -- instead we'll just do the alignment fault dance.

[willmmiles]

I do not find the code less readable but it sure is odd to see dest.print() and sappend() in the same context. Would it make sense to make a derived class and wrap sappend() functionality into it to keep dest.print() format?

Keeping something like sappend as a function makes sense as it's used so many times. It needs to be able accept any Print type though, since it's used in multiple contexts with different Print subclasses. We could change the name to clarify the usage, though - maybe print_setting or somesuch? Actually it might be clearer to go one step further eliminate the switch statement in the sappends and pass the type through the function name instead, eg. print_value or print_checkbox or print_message etc.

[willmmiles]

I suppose the alternative would be to add some shim types, eg.

#include <Printable.h>

class SetCheckbox : public Printable {
   const char* _key; bool _val;
   public:
      inline SetCheckbox(const char* key, bool val) : _key(key), _val(val) {};
      size_t printTo(Print& dest) override { return dest.printf_P(PSTR("d.Sf.%s.checked=%d;",_key,(int)_val); }
}

template<typename T>
class SetValueImpl : public Printable {
   const char* _key; const T& _val;
   public:
     inline SetValueImpl(const char* key, const T& val) : _key(key), _val(val) {};
     size_t printTo(Print& dest) override { return dest.printf_P(PSTR("d.Sf.%s.value=",_key) + dest.print(val) + dest.print(';'); }
}

// Factory for convenient usage syntax
template<typename T>
inline auto SetValue(const char* key, const T& value) { return SetValueImpl<T> {key, value} );


// Usage
{
   dest.print(SetValue(PSTR("CM"), cmDNS));
   dest.print(SetValue(PSTR("AB"), apBehavior));
   dest.print(SetValue(PSTR("AS"), apSSID));
   dest.print(SetCheckbox(PSTR("AH"), apHide));
}

That said, this almost certainly would come at a code size and performance cost as the compiler can't see through the resolution of the Printable class to skip the vtable lookups, since the Arduino core folks didn't put the implementation of Print::print(const Printable&) in the header like they probably should've.

[blazoncek]

I am nowhere near the C++ knowledge to understand what you said but common logic says "if you use template doesn't that use base classes"?

[willmmiles]

The template usage in that example was mostly because I dislike having to specify the types at the point of usage, eg, sappend vs sappends. My preference is to use templates or overloading to have the compiler select the function that does the right thing for each type. (Though as I review the sketch above, it's actually wrong -- string types need quotes around the value output; this could be fixed by some explicit template instantiations for string types...)

My specific complaint about the formulation using Printable types is that the Arduino core doesn't provide the compiler with enough information to resolve the call sequence completely. When making virtual calls, if the compiler knows the concrete, final type of the class it's using, it can skip the vtable lookup and just call the correct function directly. See https://godbolt.org/z/e5hG5vY6j -- in the middle non-optimized output pane, you can see that it call Derived::some_function without making a vtable lookup; and in the right-hand optimized pane, it was able to inline the call away entirely and just pass the literal value on to cout directly!

However, the Arduino core has put the logic that actually calls the virtual function in another translation unit (Print.cpp). So the compiler can't do any of that - if we used something like the sketch above, the compiler has no choice but to actually instantiate the object on the stack, and then pass a pointer in to Print.print(Printable&), which then just turns around makes a vtable lookup to call Printable.printTo() :( If they'd put the one-line implementation of Print.print(Printable&) in Print.h, all that could've been optimized away, making it zero cost at runtime to use this alternative syntax. Alas!

All that said: it's also possible that link-time optimization offers the toolchain enough power to fix some of this, but I don't have a lot of experience with it, so my confidence level is low.

[blazoncek]

You act like "my wish is your command". 😄
This is so much more readable than it was.

[willmmiles]

You act like "my wish is your command". 😄

You're the reviewer - it's my responsibility to shape the PR up to your standards!
FWIW it also seems that bc11398 reduces the code size a little more (about 1k on ESP8266, 500 bytes for ESP32).

[blazoncek]

to shape the PR up to your standards!

Ouch!

[willmmiles]

to shape the PR up to your standards!

Ouch!

Erp, sorry, didn't mean to offend! I think having high standards for code quality is a very good thing for a widely-used project, especially with embedded software where a mistake can make it really difficult to install a patch later. I guess maybe I'm just not so good at distinguishing between "wouldn't it be nice to have" and "please include in PR". Especially with a refactor like this, might as well get the most bang for the testing buck, I think..

[blazoncek]

Not offended. Just reminded me that I need to be careful how I phrase my sentences. I am bad at that.
I'll pull this into my local bus-config branch and test it a bit (not that I do not trust your coding but to get familiar with the change) and then merge.

[blazoncek]

Pulled into test branch, let the fun begin.

First sign is good!

Flash: [==========]  98.2% (used 1543870 bytes from 1572864 bytes)

[blazoncek]

Fount 1 issue:

dest.print(F("bLimits="
...
TOSTRING(WLED_MAX_ANALOG_CHANNELS) ");");

causes issues as WLED_MAX_ANALOG_CHANNELS is now defined as

#define WLED_MAX_ANALOG_CHANNELS (LEDC_CHANNEL_MAX*LEDC_SPEED_MODE_MAX)

to use HAL.

EDIT: A quick fix.

dest.printf_P(PSTR("bLimits(%d,%d,%d,%d,%d,%d,%d,%d);"),
   WLED_MAX_BUSSES,
   WLED_MIN_VIRTUAL_BUSSES,
   MAX_LEDS_PER_BUS,
   MAX_LED_MEMORY,
   MAX_LEDS,
   WLED_MAX_COLOR_ORDER_MAPPINGS,
   WLED_MAX_DIGITAL_CHANNELS,
   WLED_MAX_ANALOG_CHANNELS
);

[willmmiles]

Yeah, I went back and forth on the use of TOSTRING. When it works, it saves both code size and runtime performance, but it won't work for all legal macros. I wish there was a compile-time printf() - maybe someday constexpr std::format will give it to me.

[blazoncek]

@softhack007 would you care to take a look? another pair of eyes is nothing but beneficial.
BTW it may look daunting but it is not.

[softhack007]

@softhack007 would you care to take a look? another pair of eyes is nothing but beneficial. BTW it may look daunting but it is not.

@blazoncek yeah i've just "landed" after having a week full of meetings with customers. Once my head has cooled down a bit, i'll take a look.

[softhack007]

First thought: it's probably the right approach, but it means that all usermods need to be touched - and some of them (audioreactive) do a lot of oappend().

Would it be possible to still leave a legacy "oappend()" that's wrapping the new functionality?

• a new function oappend_dest() to store the current "Print &dest" instance pointer into a static variable
• oappend will just read the Print instance stored by oappend_dest(), and then perform dest.print()
• while at it, we could create a variant of oappend() that takes String as parameter, instead of char*

[willmmiles]

Would it be possible to still leave a legacy "oappend()" that's wrapping the new functionality?

If you're happy that changing the usermod API is a good long-term plan, I can build a compatibility shim specifically in the context of usermods to ease the transition process. I'll sketch something tonight if I can.

[willmmiles]

OK, I've put in a shim so that existing Usermod::appendConfigData() functions will be called with a working oappend. The shim is an inline type-forwarding passthrough, so it'll work on strings, ints, literals, or whatever else can be print()ed. This should offer backwards compatibility with the current Usermod API, while exposing the native appendConfigData(Print&) for future use without the extra call.

The followup question: this PR includes a search-and-replace on existing usermods to update them to the new API. I've tried a couple and they seem to work, but I haven't tested everything. Should I revert that? Or just for certain usermods where there's particular value in staying close to their own upstream?

[softhack007]

Idea: while "dest" is technicially correct, it might be better to find a name that describes the purpose of dest.

How about settings.print(....) or uiScript.print(...) ?

[blazoncek]

Dest is just a reference to Print object. There can only be one, but can be named whatever you like.
That's in the function definition above.

[blazoncek]

On a second thought, why not just oappend.print(...);?

[willmmiles]

Hm. My preference would be settingsScript - I think might as well take the opportunity to use a clearly purposed name. Of course as @blazoncek points out, whatever we pick would be merely a convention - usermod authors are free to name it whatever they like.

[softhack007]

OK, I've put in a shim so that existing Usermod::appendConfigData() functions will be called with a working oappend.

👍

The followup question: this PR includes a search-and-replace on existing usermods to update them to the new API. I've tried a couple and they seem to work, but I haven't tested everything. Should I revert that? Or just for certain usermods where there's particular value in staying close to their own upstream?

Good question .... maybe we revert all usermods to the old oappend now, except for a few ones that are maintained by us: fourLineDisplay, Rotary, Temperature, bobLight, multi-relay, audioreactive, example_v2. @blazoncek please feel free to add to this list.

[blazoncek]

Well. 😄
Now that you asked ... I would prefer to abandon oappend altogether.
There will be some work to utilise oappend.printf_P(...) everywhere but just replacing oappend(...) with oappend.print(...) will still be readable, understandable and compatible.

[willmmiles]

I'm closing this one in favor of #4152, which has the same patch sequence but with no usermod changes. We can table those as a separate PR.