Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

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

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

Details

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) 
 firmware/logf.c            |  212 +++++++++++++++++++++-----------
 firmware/export/logf.h     |    9 -
 apps/logfdisp.c            |  293 ++++++++++++++++++++++++++-------------------
 firmware/common/sprintf.c  |    5 
 firmware/include/sprintf.h |    2 
 5 files changed, 318 insertions(+), 203 deletions(-)

This task depends upon

Closed by  Maurus Cuelenaere (mcuelenaere)
Saturday, 22 August 2009, 00:54 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  In r22462.
Comment by Thomas Martitz (kugel.) - Saturday, 15 August 2009, 17:40 GMT+2
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, 17:47 GMT+2
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, 17:52 GMT+2
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, 18:02 GMT+2
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, 18:15 GMT+2
Yeah, I'm a dolt. Ignore that last bit of my comment.
Comment by Maurus Cuelenaere (mcuelenaere) - Sunday, 16 August 2009, 01:00 GMT+2
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.
   rockbox_logf2.diff (17.7 KiB) 
 firmware/logf.c            |  212 +++++++++++++++++++++-----------
 firmware/export/logf.h     |    9 -
 apps/logfdisp.c            |  293 ++++++++++++++++++++++++++-------------------
 firmware/common/sprintf.c  |    5 
 firmware/include/sprintf.h |    2 
 5 files changed, 318 insertions(+), 203 deletions(-)

Comment by amaury pouly (pamaury) - Sunday, 16 August 2009, 22:38 GMT+2
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.

Loading...