Fix serial stdio - add new get_stdio_serial()

[0xc0170]

This is similar to what we already did in mbed-drivers (PR for the addition there ARMmbed/mbed-drivers#145).

I considered SingletonPtr object for this serial stdio but did not find any benefit, just more sugar around to get the same result, or? Additionally, the first commit cleans HAL targets, the memcpy of stdio objects.

Replaces #2455 , an application should use this new method for stdio configuration.

Questions:

• what to do with patterns like Serial pc(USBTX,USBRX) ? We could potentially deprecate it (somehow check that serial was already initialized, and a user is doing it for the same pins (might not work for each use case as sometimes stdio gets initalized later once used..), thus printf a warning that use a new method since v5.1.2 mbed-os version). ideas?

@bridadan @bogdanm @sg- @geky @c1728p9

[0xc0170]

This should fix #2454

[0xc0170]

Fixing failures (the last rebased somehow squashed first 2 commits), will fix

[geky]

Unfortunately, C++ is very terrible at garbage-collecting globally-allocated non-pod objects. It may not be much of a concern here, since we expect almost all devices to have stdio, but if a device never calls a serial function, this static Serial declaration will cause the entire serial codebase to get dragged into the final image.

This is what SingletonPtr was trying to solve, but it unfortunately doesn't support non-default constructors.

If we want to avoid the class leak, we can use a static buffer with placement new. This also avoid multiple initialization flags introduced by the static constructor. It's ugly, but significantly reduces poorly-defined behaviour from my experience:

static bool stdio_inited = false;
static unsigned stdio_uart[(sizeof(Serial)+sizeof(unsigned)-1)/sizeof(unsigned)];

if (!stdio_inited) {
    new (stdio_uart) Serial(STDIO_UART_TX, STDIO_UART_RX);
#if MBED_CONF_CORE_STDIO_BAUD_RATE
    reinterpret_cast<Serial*>(stdio_uart)->baud(MBED_CONF_CORE_STDIO_BAUD_RATE);
#endif
    stdio_inited = true;
}

return reinterpret_cast<Serial*>(stdio_uart);

[0xc0170]

Unfortunately, C++ is very terrible at garbage-collecting globally-allocated non-pod objects. It may not be much of a concern here, since we expect almost all devices to have stdio, but if a device never calls a serial function, this static Serial declaration will cause the entire serial codebase to get dragged into the final image.

I am not certain this is a case. It's not in the local scope. I just tested both solutions, this solution and with placement new, the only difference for IAR was a static guard added for this one (as you noted, one static guards are present). I'll update it to the placement new, thanks for the tip, as it saves some memory (see my commit for more info)

[bridadan]

@0xc0170 I don't believe that this can replace #2454 completely because greentea and utest are using RawSerial instead of Serial. I don't believe greentea can switch to using Serial quite yet because there are currently tests that are printing in interrupt context (tickers, timers). These tests are sensitive to timing, so I believe this is the reason they are currently doing this (this were legacy tests from mbed 2 and mbed OS 3). I think it'd be possible to delay prints to main() with a little effort and refactoring. However, that can be done in another PR. When the tests have been refactored, greentea and utest could definitely switch to just using printf/stdio serial.

[geky]

We can gracefully deprecate Serial pc(USBTX,USBRX) by introducing a new type for the USB pins, although this would require more changes in the target implementations:

// in PinNames.h
typedef enum {
    USBTX = PB_10,
    USBRX = PB_11,
} DeprecatedPinName;

// in Serial.h
MBED_DEPRECATED("`Serial pc(USBTX,USBRX)` is deprecated, please use stdio directly")
Serial(DeprecatedPinName tx, DeprecatedPinName rx)
    : Serial((PinName)tx, (PinName)rx) {
}

Serial(PinName tx, PinName rx) {
    bablabla;
}

[0xc0170]

@0xc0170 I don't believe that this can replace #2454 completely

It needs to - we have to figure it out how to fix it once for all. We need to clean up the diff between Serial and RawSerial. I chose Serial here (I considered to return RawSerial&) as I assumed Serial will become default (can be implemented as RawSerial), that makes sense? I am not fully certain about unification of serial objects, this requires further discussion.

I would propose for greentea/utest to use RawSerial for now, but we dont garantee that they get initialazed properly , as it is now. The apps that use Serial pc(), should switch, and RawSerial will follow.

We can gracefully deprecate Serial pc(USBTX,USBRX) by introducing a new type for the USB pins

That could be, but as you noted takes lot of refactoring. Leets see if we can come up with something better.

[geky]

We need to clean up the diff between Serial and RawSerial.

+1 for this!

[sg-]

This is what SingletonPtr was trying to solve, but it unfortunately doesn't support non-default constructors.

Would prefer we use SingletonPtr and update SerialBase (or the class name a cleanup resolves to) with an initialization list and default constructor. Maybe think about how deferring pin assignments (if needed) and allowing to change them.

[0xc0170]

I fixed the bad rebase, changed to placement new as it saves some memory. I'll run some tests, was running KL25Z locally with IAR and uvision today.

Would prefer we use SingletonPtr and update SerialBase (or the class name a cleanup resolves to) with an initialization list and default constructor. Maybe think about how deferring pin assignments (if needed) and allowing to change them.

I don't see any benefit for that pattern for this one. Serial and SerialBase would not make sense without pin arguments (leads to errors), plus SingletonPtr is not true singleton (I'll create an issue about it), and does not allow arguments to be passed and used for construction for the type (which is OK because it has its own use).

One question still remains - RawSerial and Serial for stdio. Should we solve it within this PR. if yes, then how? I can't think of anything that is not breaking, thus would like to do it as a separate step.

[bogdanm]

Should we solve it within this PR. if yes, then how?

Probably not, since that's potentially an API-breaking change, so it deserves its own PR.

I can't think of anything that is not breaking, thus would like to do it as a separate step.

RawSerial was initially created to be able to call serial.printf from an interrupt context. It's not there 100% since it might still use malloc(), but lots of times it'd still work. That said, I don't know how relevant it is anymore.

[0xc0170]

Note: as we are merging some new targets, there are conflicts, once this approved (at least two +1), I'll rebase, until then, conflicts are allowed :-)

@bogdanm Thanks, thus you agree that this is fine as it is (having a function to return Serial reference, as that is a default stdio object, not RawSerial) ?

Ideas?

[bogdanm]

Yup, returning a Serial& looks like the right choice here.

[bridadan]

@0xc0170 For the reasons I specified above, we can't use Serial for stdio with greentea because there are currently tests printing in interrupt context. This is why need to use RawSerial for the moment. That's why I was saying this PR can't 100% fix the issue detailed in #2455. Does that make sense?

We should probably be refactoring the tests that print in interrupts, but that will take non-trivial changes. In the meantime I wanted to solve #2454. When this is merged and the tests are refactored, then I can submit another PR to take advantage of this new API and actually share the stdio Serial instance.

[0xc0170]

@bridadan I am aware of those, yes this does not cover all use cases. What we can do here, to add a comment/note to RawSerial, if that class is used for stdio, there's no guarantee for proper configuration of stdio, and state there that this new function should be used?

[geky]

Small question, should this be a reference? This could confuse the ownership of the Serial object, and I'm not sure what the benefit is over Serial*.

With Serial& it's easy to accidentally do this:

void maximize_baud() {
    Serial s = get_stdio_serial();
    s.baud(96000000);
    // s's destructor is called, possibly deinitializing serial
}

[pan-]

I believe it should be a reference, a pointer might be NULL, a reference is never null.
Your question leverage another one, why the Serial class is copy constructible ?

[0xc0170]

With Serial& it's easy to accidentally do this:

That's a user error, we can protect it as @pan- highlighted it's about copy ctor. I'll have a look at it

[bogdanm]

I don't know if it helps, but both Serial and RawSerial inherit from SerialBase, so you could in theory return a SerialBase& reference. Probably not useful though.

[geky]

It sounds like stdio needs to be interrupt safe, at least temporarily. Is it possible to avoid malloc / synchronization in printf when in interrupt context?

[pan-]

@geky Yes it can, just assign a static buffer to the stream with setvbuf.

[0xc0170]

@geky Yes it can, just assign a static buffer to the stream with setvbuf.

How big buffer to allocate by default? As we have now the config, it could be configurable. What do you guys think about this?

[sg-]

@0xc0170 if we're not proceeding with this for now lets close and reopen when a plan is in place for stdio UART. There are consequences for devices with one UART.