object duplication

[jkroonza]

Describe the bug, issue or concern

When following the LoggingGalore example, it turns out that somewhere along the line the TLog "child" objects are duplicated.

I'm not clear as to why, but after adding a Serial.println("write() called on SyslogStream(%p)\n", this) it became quite clear that there ends up being multiple SyslogStream instances (the pointer values for %p differs).

The motivation was because some aspects I only wanted to log to syslog, not to Serial too, and I wanted to be able to switch on/off independent sub-loggers by invoking their begin() and stop() methods independently as configuration settings are updated, and/or network becomes available etc ...

Relevant code example:

#define SYSLOG_SERVER "your.servername.here"
#define SYSLOG_PORT 514

#define WIFI_SSID "your_ssid_here"
#define WIFI_PSK "and your PSK"

#include <WiFi.h>
#include <TLog.h>
#include <SyslogStream.h>

SyslogStream syslogger;

void setup()
{
    Serial.begin(115200);

    syslogger.setDestination(SYSLOG_SERVER);
    syslogger.setPort(SYSLOG_PORT);
    syslogger.setRaw(true);
    syslogger.setIdentifier(HOSTNAME);
    
    const std::shared_ptr<LOGBase> syslogStreamPtr = std::make_shared<SyslogStream>(syslogger);
    Log.addPrintStream(syslogStreamPtr);

    WiFi.mode(WIFI_STA);
    WiFi.disconnect();
    WiFi.setHostname(HOSTNAME);
    delay(100);
    WiFi.begin(WIFI_SSID, WIFI_PSK);
 }
 
static unsigned long last = 0
loop()
{
    if (WiFi.status() == WL_CONNECTED) {
        syslogger.begin(); // doesn't work - but if syslogger.println("this will");
        Log.begin(); // works
    } else {
        syslogger.stop(); // doesn't work.
        Log.stop(); // works
    }
    Log.loop();
    
    if (millis() - last >= 5000) {
        last = millis();
        Log.println("This line will repeat every 5 seconds");
    }
}

[dirkx]

Ok - never really considered this use case. So Log() is essentially a trampoline; so all it does is keep a list of the 'real loggers' and then for every method (e.g. print, stop, begin, etc) do a 'for( i over all loggers) i.method()' sort of thing. So what should work is is to bypass it - and directly call its methods: SyslogStream syslogger; void setup() { Serial.begin(115200); syslogger.setDestination(SYSLOG_SERVER); ... WiFi.mode(WIFI_STA); ... } loop() { if (WiFi.status() == WL_CONNECTED) { syslogger.begin(); } else { syslogger.stop(); } syslogger.loop(); // <----- the important one ! if (millis() - last >= 5000) { last = millis(); syslogger.println("This line will repeat every 5 seconds"); } } Or if you _ALSO_ add it to the main Log const std::shared_ptr<LOGBase> syslogStreamPtr = std::make_shared<SyslogStream>(syslogger); Log.addPrintStream(syslogStreamPtr); then below should work: loop() { if (WiFi.status() == WL_CONNECTED) { Log.begin(); } else { Log.stop(); } Log.loop(); // <----- the important one ! if (millis() - last >= 5000) { last = millis(); syslogger.println("This line will repeat every 5 seconds only on syslog"); Log.println("and this on boith"); } } Does that work for you ? If so - and I'll make an example of this type of use.

[jkroonza]

The problem is (I suspect) that this code:

    const std::shared_ptr<LOGBase> syslogStreamPtr = std::make_shared<SyslogStream>(syslogger);
    Log.addPrintStream(syslogStreamPtr);

Creates a copy of the syslogger object to which the pointer ends up pointing, so it ends up not referencing the original object. Which makes sense since the idea (as I've got it) behind shared_ptr is that it's a reference-counted pointer such that when the ref-count reaches zero it issues a delete on the pointed-to object, which you can't do with the original object since it's a statically allocated object here.

So without testing, not I don't think your change suggested use-change would work, because the cloned object would have it's begin() called via the trampoline, but the original object would not. One would have to do:

Log.begin();
syslogger.begin();

And:

Log.stop();
syslogger.stop():

Respectively to have that work.

I'm wondering whether the best idea wouldn't be to just outright create the shared_ptr at the global scope, something like:

static std::shared_ptr syslogStreamPtr;

And then in setup() do something like:

syslogStreamPtr = new SystemLogger();

before proceeding to the rest of the steps, obviously now using syslogStreamPtr->methods() as appropriate ... and with the understanding that I've not yet tested any of this.

Another idea would be to store raw pointers into Log ... but for obvious reasons that eliminates the benefits of having the shared_ptr to begin with.

[dirkx]

Or perhaps something like: std::shared_ptr< LOGBase> syslogStreamPtr (syslogger); to avoid the implied 'new' in make shared ?

[jkroonza]

Or perhaps something like: std::shared_ptr< LOGBase> syslogStreamPtr (syslogger); to avoid the implied 'new' in make shared ?

Will try this one. Not familiar enough with shared_ptr to answer that one off the top of my head.

[jkroonza]

Doesn't work, because syslogger isn't a pointer.

Using &syslogger works, but it also implies that we will crash during shutdown (my, and I believe most use-cases for arduino doesn't ever intend to shut down) due to syslogger being static, so instead (more technically correct but both will probably work since no shutdown ever):

SyslogStream *syslogger = NULL;

void setup()
{
    syslogger = new SyslogStream();
    syslogger-> .... various setup calls.
    
    std::shared_ptr< LOGBase> syslogStreamPtr (syslogger);
    TLog.addPrintstream(syslogStreamPtr);
    
    ....
}

void loop()
{
    if (WiFi.statys() == WL_CONNECTED)
        syslogger->begin();
    } else {
        syslogger->stop();
    }
    
    // everything else now works as *expected* ... with the caveat that if TLog goes out of scope, syslogger-> becomes invalid
    // and should no longer be used.
}

To combat the last one should probably keep a std::shared_ptr syslogger(NULL); in the global scope too to which one can assign the constructed obect, and that can then be added directly. I'd recommend adjusting examples ... but honestly in the bigger scheme of things it's possibly not as big an issue. One could also use the relevant functor in shared_ptr to just do nothing upon ref-count reaching zero ... ie, effectively turn it into an expensive pointer being stored by TLog. But that would require changes to the API ... but that can probably be abstracted away by overloading addPrintstream to accept a raw LOGBase*, and wrap that with a shared_ptr which has a destructor functor (_deleter IIRC the template parameter correctly now) which doesn't actually delete the pointer ... but I'm not convinced that's the right approach either.

Not sure what I would do with the examples ... that's your call.