Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Operating System/Drivers
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by pamaury - 2009-08-15
Last edited by mcuelenaere - 2009-08-21

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

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)
Closed by  mcuelenaere
2009-08-21 22:54
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

In r22462.

kugel. commented on 2009-08-15 15:40

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!

rasher commented on 2009-08-15 15:47

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".

kugel. commented on 2009-08-15 15:52

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).

pamaury commented on 2009-08-15 16:02

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 :)

rasher commented on 2009-08-15 16:15

Yeah, I'm a dolt. Ignore that last bit of my comment.

mcuelenaere commented on 2009-08-15 23:00

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)
pamaury commented on 2009-08-16 20:38

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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing