Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce stream support #82

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Introduce stream support #82

wants to merge 3 commits into from

Conversation

jupe
Copy link
Contributor

@jupe jupe commented Apr 7, 2018

Status

IN PROGRESS

Migrations

NO

Description

This allows to pipe traces to any standard FILE* stream.
NOTE: This should not break existing API's or behaviours.

This allows to do this:

FILE *fh = fopen("./debug.log", "a");
mbed_trace_set_pipe(fh);
tr_debug("debugging to file");

Todo

  • Tests
  • Doc
  • Examples

@jupe jupe requested review from tommikas and teetak01 April 7, 2018 12:04
{
m_trace.stream = stream ? stream : stdout;
}
void mbed_trace_fputs_function_set(int (*fputs_f)(const char *, FILE*))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we doesn’t need this API at all

//print out whole data
m_trace.printf(m_trace.line);
m_trace.fputs(m_trace.line, m_trace.stream);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous puts() was used to print line. fputs behaviour a bit differently, but maybe we should use fwrite instead because we know line lenght already and it could be faster, opinions? 🤔

@jupe jupe requested a review from kjbracey April 9, 2018 09:51
@jupe
Copy link
Contributor Author

jupe commented Apr 9, 2018

This PR changes default "printer" function from puts to fputs, which behaviours slightly differently (eg buffers/flushing point of view). When using custom printer there is wrapper function which provides fputs -kind of API and calls that custom printer which are backward compatible. Haven't analyse more deeply does this affects to performance and how much if. Opinions about this ?

@kjbracey
Copy link
Contributor

kjbracey commented Apr 9, 2018

Feels a little bit unnecessary.

I've always been quite happy to provide a print function that does fputs(line, X); fputc('\n', X);. Doesn't seem like this is really easier.

I guess a general config option to include a newline in the output call would streamline it a bit.

@kjbracey
Copy link
Contributor

kjbracey commented Apr 9, 2018

BTW, puts shouldn't be different from fputs(stdout), except for the fact it includes a newline.

In POSIX puts being a single call means it's effectively equivalent to

flockfile(stdout);
fputs(line, stdout);
fputc('\n', stdout);
funlockfile(stdout);

On non-POSIX systems, the atomicity of a single library call isn't guaranteed, and we're relying on our mutex anyway.

But having an option to make the formatting core stick on a \n along with all the other headers/trailers makes perfect sense to streamline this generally - whether or not the friendly "stream" thing is added.

@jupe
Copy link
Contributor Author

jupe commented Apr 9, 2018

Feels a little bit unnecessary.
I've always been quite happy to provide a print function that does fputs(X, line); fputc('\n', X);. Doesn't seem like this is really easier.

True, basically same thing can be achieved already with a bit more code. Idea was to "simplify" output a bit so that output is always standard FILE stream so that output can be something else than stdout easily.. Do you see some (other negative) side-effects with this PR ?

I guess a general config option to include a newline in the output call would streamline it a bit.

I think about it also. Would be easy task.

In POSIX puts being a single call means it's effectively equivalent to
flockfile(stdout);

Maybe our mutex API calls could have stream argument so it could be flockfile by default in POSIX..

@kjbracey
Copy link
Contributor

kjbracey commented Apr 9, 2018

I've always thought part of the concept was that there was no assumption anywhere that the output actually be a stream - it could be an insertion into a circular buffer, or transmission as a network packet.

Hence the entry being delivered as a simple single "unit".

I'd kind of like that purity preserved in the core, but I guess a helper that does set_stream_output(FILE *) makes sense. But it can just be equivalent to

remember_stream
set_output(internal_helper_fputs_function)
turn on line endings

@kjbracey
Copy link
Contributor

kjbracey commented Apr 9, 2018

Locking is a slightly fiddly concept - not sure flockfile fits directly. If we were to attempt an extra buffering layer for buffering from IRQ (as per the universal trace), you'd need a new locking concept anyway, which I've been pondering.

You'd need one lock to protect the formatting and temp buffers in the tr_xxx calls, and a separate lock to protect interaction between buffer drain to screen and other screen users like CLI. That second lock could actually be not visible to the formatter, and built into the output function. But at the minute the CLI assumes it's all one lock, and uses the formatter lock to avoid output clash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants