Get rid of stdout and stderr inside library code

[dl8dtl]

In the course of working on #1714 I noticed that avrdude_message2() uses stdout and stderr through wrapper macros (and then takes extra pain to distinguish them as well as generic FILE* streams, but that's a different matter). Looking closer, there are many other functions that output things directly to stdout or stderr, namely the print_parms and display methods of struct programmer_t.

Given that the purpose of segregating the library was to decouple the CLI frontend from the library backend, I think there is not much sense that library functions decide to output anything (except of catastrophic failures, e.g. unability to allocate memory) t o IO streams. Suppose someone wants to build up a GUI on top of the library, then they'd certainly prefer to get all those messages into some kind of GUI element, rather than having them on stdout or stderr.

I'd like to start a sweep through the code, removing those references to these FILE* streams except for plain CLI functionality itself (like option handling, or terminal mode).

Opinions?

On a second matter, I also think that the current way avrdude_message2() performs the '\v' handling is a bit too generic, given that this feature is mostly needed inside terminal mode. I'd rather tend to remove '\v' handling from that function, and provide this functionality inside term.c only.

[mcuee]

I'd like to start a sweep through the code, removing those references to these FILE* streams except for plain CLI functionality itself (like option handling, or terminal mode).

Good idea.

On a second matter, I also think that the current way avrdude_message2() performs the '\v' handling is a bit too generic, given that this feature is mostly needed inside terminal mode. I'd rather tend to remove '\v' handling from that function, and provide this functionality inside term.c only.

I tend to think this is a good idea as well if it does not affect the CLI mode. I am not so sure if there are other usages in the CLI mode or not. @stefanrueger and @MCUdude may chime in here.

[stefanrueger]

TLDR, stdout and stderr are already outside libavrdude library code for almost all of the function calls!

The long of it:

The base function avrdude_message2() of AVRDUDE's messaging is provided in main.c, which is not part of libavrdude. The #define macros reside in avrdude.h, not in libavrdude.h. Libavrdude functions call the msg_...() functions in a fine-grained way (basically to inform about severity of errors verbosity of logging), and it is up to the application to provide what's useful for it. AVRDUDE has chosen to use printing to stderr for that, but other applications could do what they want, eg, decide to ignore high-level debugging messages and only cater for OS errors, library errors and warnings etc with pop-up windows or any other way.

IIRC term.c used to be outside libavrdude (like main.c) but as its functionality increased probably was moved into libavrdude. However, the display function term_out() that writes to stdout for AVRDUDE's terminal is also a macro in avrdude.h, so can be remodded for any application. So term_out() too is outside libavrdude.

This leaves the print_parms and display methods of struct programmer_t (which probably were invented with AVRDUDE in mind and not with abstract applications served by libavrdude). I agree, it would be useful to deal with the ensuing printf messages in a more abstract way. That has started with using fmsg_out(), mostly for print_param1() functions, these used a generic FILE *fp pointer in AVRDUDE (at the top of my head, I don't know why generic FILE * fp but I am sure there is a reason).

BTW, as it's currently implemented, libavrdude functions uses a messaging system for debugging messages, warnings, and errors: imsg_...(), pmsg_...(), and msg_...(). These were chosen to make logging and error messaging AVRDUDE less painful, easier to read and give a shorthand for the poor developer, who otherwise needed to write msg_error("%s: ...\n", progname) in 1000s of cases:

Name	pmsg_...()	imsg_...()	msg_...()	fmsg_...(fp, )
# of calls	2147	151	815	36

That pain of providing 29 messaging functions for an application pales in comparison to reading/writing the current 3100+ function calls for the logging/warning messaging system, in particular, as they all can be implemented via one generic function as is demonstrated by avr_message2().

OK, now to the second matter of this issue:

TLDR: \v is actually mostly used by the messaging functions, not the terminal functions.

\v at the start of a message indicates that a new line should be effected unless we are already at the beginning of a line.

Well, the AVRDUDE project received a lot of observations along the way of this doesn't line up, that newline is too much, this doesn't look good etc. That was particularly often the case for the progress reporting interrupted by logging or error messages. It is near impossible to figure out outside the messaging system whether or not the most recent output character is a \n (given that whether or not logging/error messages are printed depends on the whim of a user), hence the code best sits within the messaging system ... And the use of that technique transcends the terminal very much so: It just so happens that the progress report system code happens to sit in term.c but it's use is outside, predominantly between msg_...() and progress reporting functions both in the terminal and the CLI functionality of AVRDUDE.

I suggest moving the progress reporting functions from term.c to main.c to move them outside libavrdude and, hence, have progress reporting as something else that any application needs to think about how it should be done (eg, dancing hamsters rather than ASCII graphics of ################## progress bars).

[mcuee]

IIRC term.c used to be outside libavrdude (like main.c) but as its functionality increased probably was moved into libavrdude. However, the display function term_out() that writes to stdout for AVRDUDE's terminal is also a macro in avrdude.h, so can be remodded for any application. So term_out() too is outside libavrdude.

Not so sure if it is good to trim down term.c into two parts, one part is inside libavdude, the other part is outside.

I suggest moving the progress reporting functions from term.c to main.c to move them outside libavrdude and, hence, have progress reporting as something else that any application needs to think about how it should be done (eg, dancing hamsters rather than ASCII graphics of ################## progress bars).

Good idea!

[dl8dtl]

Progress reporting is already outside the library itself. When I first started the library segregation (and now I'm surprised this already happend in 2007 :-o), I turned progress reporting into a callback function. I just finished implementing Python callbacks for both, messaging as well as progress reporting.

I completely agree that all the work that has been spent into centralizing messaging into avrdude_message2() is a huge step forward into the right direction. I just think that it makes sense to proceed that way, and also turn the print_parms and display methods away from their own stderr handling, towards avrdude_message2().

The ability to distinguish between stdout and stderr is important for the CLI, because it allows to capture information from stdout through shell redirects, while still keeping the communication outside of the captured text through stderr. However, that is very CLI-specific, so I think it would best be the CLI implementation of avrdude_message2() that actually uses the FILE* objects. Anything above that does not need to know anything about the backend the information is being passed to. Distinction between stdout and stderr could be performed by one more msgmode flag, at the wrapper macro level.

I'm not quite sure whether the print_parms and display methods actually have to distinguish between both options, but even that could easily be handled by the fmsg_out() wrapper.

I see why '\v' handling is important for the CLI though most of its use is in term.c. Except of one occurence in serialadapter.c, the remainder is probably actually only used inside CLI code but not in the library.