(WIP) Feature: Implement fstring topics

[hmueller01]

Resolves #35

We can not use publish_P(), as here the payload it already from PROGMEM.
But we can implement a beginPublish_P() with a topic from PROGMEM.

[hmueller01]

Ok, checks look good. Next I will implement a template for writeString() / writeString_P() ...

[TD-er]

on what device are you testing?
For ESP32-xx the flash string is actually just a const char* and much more forgiving if you don't use _P functions.

[hmueller01]

on what device are you testing? For ESP32-xx the flash string is actually just a const char* and much more forgiving if you don't use _P functions.

Same thing will crash on ESP8266 ...
Reason is the esp8266 32bit aligned flash.
The bad thing is, that the compiler will not complain, as PGM_P == const char * and crash might happen. I struggled so often with that already ... So a _P function is needed as overloading will not work.
https://arduino-esp8266.readthedocs.io/en/latest/PROGMEM.html

[TD-er]

There is one really big disadvantage of templates...
For each type that can call this function, the compiler will generate a new blob of code.
Now it is contained as a private function, which will keep it somewhat manageble.

For publicly called functions you really should take care or else you might end up with a compiled blob for types like const char[10], const char[11], const char[...], ... well you get the point :)

[hmueller01]

Hmm, thanks for the hint. I wasn't aware that the compiler creates new code for every type. The goal was to have just one implementation. But wasting flash does not sound good. Maybe I should not use templates and instead just use a parameter:
size_t PubSubClient::writeStringImpl(bool progmem_string, const char* string, size_t pos) {
Do you think this is better?

[TD-er]

Well the whole idea of templates is indeed to have the same code from a programmer's perspective. But it should be considered as 'inline' code and thus appears potentially multiple times in the binary when a different (template) type is calling it.

N.B. the same does apply to code written in .h files.

If you want to switch to using const char* then you must make sure whatever is calling this function is handling potential PGM reads.
So either wrap it into a String or feed this writeStringImpl per char.
I don't think adding a bool to indicate type is a good thing as you then do the extra implementation in that function and you potentially introduce bugs as you expect the programmer to always have this pair of bool and string to match.

[hmueller01]

Haven't seen your answer yet ...
The point is, that I need different implementations in case of PROGMEM and normal RAM. In case of ESP8266 / ESP32 I could always use the PROGMEM functions (strlen_P() and so on). But not for AVR. There RAM and ROM needs to be handled differently. So to make this lib compatible to all kinds of MCUs I need a double implementation or a switch (which is also there in case of the template). And as this is an internal function I think I can deal with the risk. As the template implementation does have no benefit, I will change to the conventional parameter.

[hmueller01]

I just tested

    mqtt_client.publish_P(F("test/progmem"), PSTR("This is a test message from PROGMEM"), MQTT_QOS0, false);
    mqtt_client.publish(F("test/progmem"), "This is a test message", MQTT_QOS2, false);

and it works as expected on the ESP8266.

[hmueller01]

I just ask Copilot for more details about always using strlen_P(). On ESP32/8266 this might be ok, but not on AVR.

No, on AVR microcontrollers you should not always use strlen_P(). You need to use the correct function depending on where the string is stored:

For RAM strings: Use strlen()
For Flash/ROM strings (PROGMEM): Use strlen_P()
This is because AVR uses a Harvard architecture where program memory (Flash/ROM) and data memory (RAM) are separate and accessed differently.

Using strlen_P() on a RAM string on AVR is dangerous because:

It will try to read the address as if it were in program memory
This can cause undefined behavior or crashes
The string length returned might be incorrect or random
This behavior is different from ESP8266/ESP32 where both functions work for both memory types due to their unified memory architecture.

It just came into my mind as @mcspr changed WString.{h,cpp} in PR esp8266/Arduino#9272
But this lib should work on all platforms to I think we need to implement both variants.

[mcspr]

It just came into my mind as @mcspr changed WString.{h,cpp} in PR esp8266/Arduino#9272
But this lib should work on all platforms to I think we need to implement both variants.

Note that PR always assumes const char * may be in flash, I just did not want another branch in String methods. (...and also b/c of PGM_P == const char * assumption, since I am pretty sure if I change compiler type for everyone that would be pretty annoying :)

Just using _P is also fast enough in most cases. Perf drop from individual reads is definitely there, but pretty marginal; I'd measure it before jumping to optimizations.

[TD-er]

Just using _P is also fast enough in most cases. Perf drop from individual reads is definitely there, but pretty marginal; I'd measure it before jumping to optimizations.

Yep and you might even get away with relying on the String class constructor handling the flash strings properly.
Sure it will add to the run time memory usage, but only on flash strings, which is testable for the programmer using this.

[TD-er]

There is a number of strnlen (and _P) checks, which are all against either max. Packet size or buffer size.
However in a packet/buffer, there is also the topic and some header, so those checks are still allowing for some overflow.
The max. possible packet size is probably not really a problem as we won't get that large payloads, or at least not from a char pointer.

[hmueller01]

As far as I understand the max. length is only there to limit strlen somewhere if the payload is corrupted. Or who wants to send ~268MB payload on a micro controller?