Feature circular file buffer

[mbartling]

Description

Add Circular file buffer retargeting to simulate a pipe to /var/log. Note, this is enabled/disabled and configureable at compile time. Currently, in release mode writes to stdout get dumped to a Sink which acts like writing to /dev/null. However, the c_strings are not getting removed at compile time which is evident using the strings tool.

Having a circular file buffer between stdout and stdin decouples the source and sink of the file objects. For example, a debug build of a project can use a reader method to dump to serial, but a release build can write important logs to persistent storage or to cloud.

Here is a toy example:

Serial pc(USBTX, USBRX);
...
printf("Hello World!\r\n");
// Toy strings are less than 64 bytes long
char* buffer = (char*) calloc(64, sizeof(char));
while(true) {
          char* len = fgets(buffer, 64, stdin);
          if(len != NULL){
              pc.puts(buffer);
              break;
          }

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Breaking change

[kjbracey]

Various thoughts:

Not a fan of putting this into mbed_retarget.cpp. That already has the hooks to redirect to an arbitrary stream via the override functions - we don't have to actually put every single stream device in that file. The ones that are there are providing the base functionality that existed prior to the override hooks.

Having write piped to read is a bit weird - yes, you want a way of getting the buffered data back out at some point, but hooking it up to stdin directly feels unnatural to me. I would rather you did that read explicitly - so hook it up to stdout via the console override only for output.

You're always writable, because you drop data, so POLLOUT should always be set. Your sigio stuff isn't functional.

For tracing, you're better off using mbed_trace rather than raw printf - that has all the necessary macro magic to properly exclude various levels depending on build. It also allows you to distinguish between "real" output and "trace" output, if that matters for your application. In that case, I might be inclined to have mbed_trace using the "CircularBufferFile", and stdin/stdout left as the serial port.

[mbartling]

Hey @kjbracey-arm, a little back story for this PR. I've been looking into various ways to capture logs and efficiently send them to the cloud, and one of these methods is to just look at standard c_strings. Obviously, c strings are not the best way to do this, but I have found that a majority of c strings in mbed projects show up in release builds (rather than getting optimized out). Even looking at team ARMmbed projects on GitHub, we cannot expect all our users to use mbed_trace over printfs.

A Quick parse of the mbed projects for printf("..."); gives the following statistics:

Num Projects 308
total num strings 24811
total string length 665928
avg num strings per project 80.5551948 // + median indicates massive skew in distributions
avg total string length per project 2162.1039
median num strings per project 15
median total string length 442.5
Expected Avg string length 26.8400306

As for your comments:

"That already has the hooks to redirect to an arbitrary stream via the override functions - we don't have to actually put every single stream device in that file." ---- Cool, I'm definitely down for doing this! Just didn't realize that was an option. Where are these functions? and how can I use them?

"Having write piped to read is a bit weird - yes, you want a way of getting the buffered data back out at some point, but hooking it up to stdin directly feels unnatural to me. I would rather you did that read explicitly - so hook it up to stdout via the console override only for output." ---- I completely agree, I just wrote it this way because we don't have direct, or at least obvious, support for `pipe. And default pipe from stdout to stdin seemed to make sense intuitively.

I can fix the POLLOUT issue, pretty much just copy and pasted it from the UART serial console and forgot to update it.

[cmonr]

@mbed-os-core @geky @kjbracey-arm @mbartling
What do we need to get this moing, one way or another?

[SeppoTakalo]

How about re-targeting stdout to TCP stream?

It should be trivial to implement FileHandle API on top of Socket.
Then you could printf() to TCP stream.

I once tried this:
SeppoTakalo@7f12aa5

[mbartling]

@SeppoTakalo I think remapping to TCP streams, etc. is a separate topic entirely and potentially expensive.

@kjbracey-arm I agree that piping stdout to stdin in the same application context is weird, but remember we only have one handle for stdout and stdin across the entire application as threads are not full processes and do not have separate FDs for their std streams. Really what we need is the ability to duplicate the stdout file descriptor and treat it as a "readable" reference in a pipe, which maintains different file offsets than the stdout FD. pretty much just dup2() and pipe()

[cmonr]

I'd prefer to not let this get stale, especially with the feature release coming up.

Reviewers, (@SeppoTakalo @mbed-os-core @geky @kjbracey-arm ) , yay? nay?

[kjbracey]

Even looking at team ARMmbed projects on GitHub, we cannot expect all our users to use mbed_trace over printfs.

Trace is trace - it should be stuff that can conceptually be fully optimised out and still leave a working useful system. Stuff that is actually important to the console or another attached device - eg command input and output for IceTea can be plain printf (or cmd_printf for nice mutexing versus any trace). If it's there as printf, it should stuff that we don't want to and won't optimise out, and so if there is any in the app, there should be some mechanism to get it somehow (either live, or via a capture like this).

If it's is just logging, and there's any prospect of no capture mechanism being in place, then INFO-or-higher level mbed_trace is to be preferred.

But anyway, yes, I agree this is useful.

only have one handle for stdout and stdin across the entire application as threads are not full processes and do not have separate FDs for their std streams.

We're a single process, yes, and we expect our single stdin and stdout to represent the console. Assigning stdin to be the "read the output" stream gives a weird console loopback effect. You can instead open your capture device for read as a separate input FILE, without interfering with stdin.

Conceptually that would then permit a variant where you have a capture device that captures the output while still passing it to an underlying device, and leaving input working. serial input -> stdin, stdout -> capture -> serial output.

[hooks for retargeting stdin/stdout] Where are these functions? and how can I use them?

mbed::mbed_override_console in mbed_retarget.h. I would suggest a JSON option to activate an override of that to assign the circular file buffer to stdout+stderr via that (and maybe stdin as another option - seems weird to me, but some apps might want to do it that way).

So take the object out of mbed_retarget.h, make it a standalone file so anyone can instantiate it (not just for stdout), and then the option to plumb it in for them.

[kjbracey]

data_written is always size. You didn't partially consume the data - you consumed it all and maybe threw some away.

If you're implementing the sigio callback, this should be calling it if writing makes the device readable. If you're not going to implement non-blocking function, you could just avoid implementing sigio, I guess.

[kjbracey]

Returning EAGAIN is not legal - you must be implementing a blocking API here if going to use stdout.

[adbridge]

@mbartling it looks like you have a merge commit on this pr. Please remove it and use rebase instead.

[mbartling]

Hi @adbridge It should actually be a rebase onto master. I just have a bad habit of writing "Merge" instead of rebase. I can change the commit message if you'd like.

[cmonr]

@mbartling Go ahead and change/update the commit message(s) (feel free to squash as well).

Single word commits probably be an automatic 'needs: work'...

[mbartling]

I went ahead and removed the SIGIO/non blocking skeletons. We should mark these as future work

[cmonr]

Wow, that was quick.
Already needs a rebase.

[cmonr]

@mbartling Rebase reminder.

[mbartling]

@cmonr Went ahead and rebased + squashed my commits. Literally just added a }, to platform/mbed_lib.json

[cmonr]

Interesting. Looks like this PR didn't have the astyle check before now.
Check out the job to see what styling issues it found with the PR.

@kjbracey-arm @ARMmbed/mbed-os-core @geky Thoughts on the PR, since it's been open for quite a while now.

[mbartling]

@cmonr Looks like the CI errored out. Do you mind relaunching?

[cmonr]

Waiting on #9131 for a fix to Travis CI.

[mbartling]

@cmonr do you need anything on my end?

[cmonr]

@mbartling Nope!

@kjbracey-arm @ARMmbed/mbed-os-core @geky Please take a final look. This should be more or less good to go.

[bulislaw]

Documentation and tests missing? There's also wait_ms(1); marked to be fixed and some dead code. What does the "sort-file-buffer" config do? I don't see it being used .

[cmonr]

@mbartling ^^^

[mbartling]

Will add tests and documentation as requested.

The soft file buffer config was a legacy thing, I just forgot to remove it, will do that now.

As for the dead wait, I modeled this method after the UARTSerial FileHandle driver. It's just a spinlock for blocking.

[0xc0170]

@mbartling Shall this be closed, reopened with an update?

[cmonr]

Closing PR for now.

@mbartling feel free to reopen once an update is available.