FS#10528 - A new implementation of logf, logfdisplay and logfdump

Attached to Project: Rockbox
Opened by amaury pouly (pamaury) - Saturday, 15 August 2009, 13:40 GMT
Last edited by Maurus Cuelenaere (mcuelenaere) - Friday, 21 August 2009, 22:54 GMT
Task Type Patches
Category Operating System/Drivers
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


This patch is a complete reimplementation of logf. Instead of using a line oriented buffer which complicated, it directly stores entries in a big buffer with '\0' as a separator.
This patch includes:
* reimplementation of logf
* reimplementation of remote display of logf
* reimplementation of logfdisp to nicely handle non fixed-width font
* reimplementation of logfdump
Wrapping is also implemented.

It also adds a function to sprintf.h which is nicer to use than snprintf in this context.

Everything was tested except serial_tx logging.
   logf.diff (17.7 KiB)
This task depends upon

Closed by  Maurus Cuelenaere (mcuelenaere)
Friday, 21 August 2009, 22:54 GMT
Reason for closing:  Accepted
Additional comments about closing:  In r22462.
Comment by Thomas Martitz (kugel.) - Saturday, 15 August 2009, 15:40 GMT
I really think wrapped printing of strings should be a lcd driver function (possibly returning the number of lines printed), there are a few places that do/want this (text viewer, splash[although it additionally centers the string], test_* plugins possibly also).

I don't have a remote, and I've never seen logf on a remote either so the following is just wondering:
I also don't know why displayremote() is in logf.c (I think logfdisp should be in apps/, using the multi screen api for displaying the logf on the main and remote display). The old logf also does it so strangely.

the name vfnprintf is bad IMO, it doesn't have anything to do with files (the f part), nor with a buffer size (the n part). vfprintf (a standard function) is also totally different.

But I surely welcome the work you've done on it, old weird code indeed!
Comment by Jonas Häggqvist (rasher) - Saturday, 15 August 2009, 15:47 GMT
Thomas: The point of logf-on-remote was that you'd have the logf display realtime on the remote, while you operated the player on the main screen. The f in printf is for "formatted".
Comment by Thomas Martitz (kugel.) - Saturday, 15 August 2009, 15:52 GMT
Ah, thanks for the explaination, I didn't know that (the remotes thing). As for formatted, the latter f already stands for it, I'm groaning about the first one (as mentioned on irc).
Comment by amaury pouly (pamaury) - Saturday, 15 August 2009, 16:02 GMT
Thank your for your comments.
The name of vfnprintf is perhaps bad, I had though "fn" would refer to "function" because it printf by calling a function. Similary there is the "fd" function which prints to a "file descriptor".
I don't think "f" is for "formatted" because it's already at the end (printf="print" + "formatted"), no ?

kugl: I don't have a remote-capable device so I tested it with the simulator :)
Comment by Jonas Häggqvist (rasher) - Saturday, 15 August 2009, 16:15 GMT
Yeah, I'm a dolt. Ignore that last bit of my comment.
Comment by Maurus Cuelenaere (mcuelenaere) - Saturday, 15 August 2009, 23:00 GMT
Very nice! This fixes some logf breakages I've seen (not showing all data that was meant to be logf'ed).

However you broke touchscreen scrolling in apps/logfdisp.c, this patch fixes that.
Comment by amaury pouly (pamaury) - Sunday, 16 August 2009, 20:38 GMT
Yes you're right, I haven't got a touchscreen so I didn't modify that part of the code and I didn't see this stupid error.