Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#114 PIC16: vfprintf

closed-accepted
Borut Ražem
None
5
2008-09-15
2008-08-09
Mauro Giachero
No

Hello again.
This time my work towards having no regression failures on PIC16 brought me to vfprintf, the root of the *printf implementation for PIC16.
Regression failures were due to the much partial implementation of that function, so I started working towards a more complete version.

Now, there are way too many *printf* implementations in the sdcc source tree for my limited brain, even looking only at those logically reachable from PIC16:
device/lib/printf_fast.c
device/lib/printf_tiny.c
These are mcs51-specific (shouldn't these go in device/lib/mcs51?).
device/lib/printfl.c
Contains little assembly, and is much limited.
device/lib/printf_large.c
Looks portable and full-featured. Unfortunatly integrating it into
PIC16 looked nontrivial, and most probably would generate very
large code.
device/lib/pic16/libc/stdio/vfprintf.c
Current PIC16 implementation for these functions, with basic
features.
device/lib/pic16/libc/stdio/printf_tiny.c
Looks like a cut-down version of the above.
device/lib/pic16/libc/stdio/printf_small.c
Even more cut-down.
So it looks like there's no full-featured *printf available for PIC16, with the only option of porting printf_large or write a new one.

The attached is an improved version of vfprintf.c. I didn't even try to make a diff since there are simply too many changes.

The first thing one notices is are the 5 switches defining which features should be enabled. These switches actually clutter the code horribly, their main purposes were to evaluate the features impact one by one and to allow a proper comparison with the old code. Maybe merging some of them so that only ~2 switches remain is a good idea. Maybe not.

Without any switch enabled, the code is 7 words smaller than the old version, and there is actually a bug fix (printing 16-bit non-decimal values with high bit set). With all switches on, the code passes the snprintf.c regression test without float support (which is only tested on ds390 and mcs51). The code is 1.6x larger than the old version.

I think PIC16 deserves a reasonable *printf implementation, so merging this code can be a good thing.

My opinion anyway is that long-term there should be:
- a common infrastructure (all ports should have their *printf*
implementations based on the same function [that is, functions
with the same prototype]);
- a basic portable implementation, maybe with switches {dis,en}abling
less-used features of that function;
- port-specific implementations for optimization sake.
My impression instead is that all ports have their own partial implementations and approach the problem in different ways, causing duplicate efforts and much confusion in the head of the poor souls trying to put their hands there ;-).
Just my 2 cents.

That's it. I hope you'll like the code.
With best regards
Mauro

Discussion

1 2 > >> (Page 1 of 2)
  • Mauro Giachero
    Mauro Giachero
    2008-08-09

    New vfprintf.c implementation for PIC16

     
    Attachments
  • Maarten Brock
    Maarten Brock
    2008-08-10

    Logged In: YES
    user_id=888171
    Originator: NO

    Mauro,

    I fully agree there should be only one common implementation. I do not believe in hand optimized assembly in the libraries which is why I removed it from printf_large a long time ago. My goal was to make it fully portable and as complete and bugfree as possible. I don't like preprocessor feature-switches in library code because they require recompilation. Only for floating point support is it appropriate as it requires so much resources. IMO optimization should come from the compiler itself.

    Have you tried to compile printf_large for pic16? How does it compare to your version?

    Greets,
    Maarten

     
  • Mauro Giachero
    Mauro Giachero
    2008-08-10

    Logged In: YES
    user_id=2160854
    Originator: YES

    Hello Maarten,

    > Have you tried to compile printf_large for pic16?
    Now yes!
    To make a quick test I:
    - deleted
    fprintf.c
    printf.c
    printf_small.c
    printf_tiny.c
    sprintf.c
    vfprintf.c
    vprintf.c
    vsprintf.c
    from device/lib/pic16/libc/stdio/
    - copied
    printf_large.c
    sprintf.c
    vprintf.c
    from device/lib/ to device/lib/pic16/libc/stdio/
    - copied
    stdbool.h
    from device/include/ to device/include/pic16
    - applied the attached patch (pic16-stdio-printf_large.patch)
    and it successfully built and completed the snprintf regression test
    (and bug1057979.c, once thrown the PIC16-specific amendments).
    Then I enabled float support and the build broke, apparently due to
    a compiler fault.

    > How does it compare to your version?
    Your library has a larger code footprint than the code I submitted,
    but uses less RAM.
    For comparison:
    ; Statistics PRINTF_LARGE:
    ; code size: 2770 (0x0ad2) bytes ( 2.11%)
    ; 1385 (0x0569) words
    ; udata size: 20 (0x0014) bytes ( 1.56%)
    ; access size: 24 (0x0018) bytes
    ---
    ; Statistics VFPRINTF:
    ; code size: 2062 (0x080e) bytes ( 1.57%)
    ; 1031 (0x0407) words
    ; udata size: 16 (0x0010) bytes ( 1.25%)
    ; access size: 31 (0x001f) bytes
    I don't know whether the comparison is fair, I didn't look much at the
    printf_large code so maybe even without float support it has more
    features than the code I submitted. I also haven't played with
    SDCC_STACK_AUTO.

    > [...] I don't like preprocessor feature-switches in library code
    > because they require recompilation. Only for floating point support
    > is it appropriate as it requires so much resources.
    I almost agree. I think that there's space for three implementations:
    - one full-featured;
    - one without floating point support;
    - one very basic with no modifiers etc, only supporting chars, strings
    and signed+unsigned longs in hex (lowercase), oct and dec, and
    always returning 0.
    Limitations should be well documented, and it should be possible to
    switch from one to the other with a command-line switch or a #define
    (something like having in stdio.h:
    #ifndef _PRINTF
    # define _PRINTF 1
    #endif
    #if _PRINTF == 0
    # define printf _printf_bare
    #elif _PRINTF == 1
    # define printf _printf_nofloat
    #else
    # define printf _printf_full
    #end
    with the other *printf as well, and starting sdcc with -D_PRINTF=x).
    All versions should be compiled and available by default for all
    targets.

    Regards
    Mauro

    File Added: pic16-stdio-printf_large.patch

     
  • Mauro Giachero
    Mauro Giachero
    2008-08-10

    Modify pic16's stdio.h to use printf_large

     
  • Mauro Giachero
    Mauro Giachero
    2008-08-12

    Logged In: YES
    user_id=2160854
    Originator: YES

    Apparently the discussion here stalled.

    I think we both agree that there should be a portable *printf version
    that all ports (new and old) can use. I don't think that having
    port-specific highly optimized versions is bad per-se, but that's
    another story.

    But making a unified good version is something that can require quite
    a large amount of time. Even more if we want to have 2 or more
    versions available due to the possibly large Makefile modifications.
    The situation gets even worse if we want to extend the concept to all
    other library functions.

    Putting this vfprintf version in sdcc, maybe after removing all the
    feature selection #defines, is IMHO a good short-term solution to
    provide good *printf to PIC16. This implementation is also portable,
    so can be used as one of the bases for the unified version.
    My suggestion is to apply it. The unification opera can then start
    and proceed gradually.

    Regards
    Mauro

    PS: I'll be on vacation/away till about the first week of September,
    and I most probably won't reply to mails in that timeframe.
    Sorry about that. I'll read mail and reply as soon as I'm back.

     
  • Borut Ražem
    Borut Ražem
    2008-09-14

    Hi Mauro,

    I took a look to your patch:
    vfprintf.c seems OK to me. I added an additional switch to enable %b specifiers: BINARY_SPECIFIER, which is not defined by default.

    I have some problems with stdio.h:
    - why did you uncomment the following lines? Where is the definition of function _print_format?
    //typedef void (*pfn_outputchar)(char c, void* p) _REENTRANT;
    //extern int _print_format (pfn_outputchar pfn, void* pvoid, const char *format, va_list ap);

    - why you removed the following declaration?
    extern unsigned int vfprintf(FILE *, char *, va_list);

    - you removed the following declarations:
    void printf_small(char *, ...);
    void printf_tiny(char *, ...); // __reentrant;
    probably because you meant to substitute them with vfprintf using different switches?
    I propose to leave them for the time being.

    Borut

     
  • Mauro Giachero
    Mauro Giachero
    2008-09-15

    Ok, I should have put a better comment to the patch (but you didn't read the thread, did you? ;-)).

    The proposed vfprintf.c is a drop-in replacement for the original one, no patch is needed.
    The patch comes from my discussion with Maarten about using the existing printf_large with PIC16. See my 2008-08-10 message.
    So, my original proposal was to replace vfprintf.c with no additional patches.
    Adding the BINARY_SPECIFIER switch is OK at least to see whether there is someone complaining, since %b has a different meaning for all other ports (maybe this should become %B?).

    However, I'be been also thinking about the "printf unification" among all the ports, and more generally about using generic library implementations instead of port-specific ones where possible. I'm not sure this thread is the right place to discuss it (I _need_ to discuss about this before starting to code), but my current idea is to work on it from November (I have to finish my Ph.D thesis before that). So this is expected to be an "interim" solution to a larger problem.

    If you apply this new implementation, please remove the PIC16-specific amendments from bug1057979.c (sorry for not having provided a patch).

    Regards
    Mauro

     
  • Borut Ražem
    Borut Ražem
    2008-09-15

    Hi Mauro,

    I red the thread, I understand you general idea, I totally agree that printf (and also many other library sources) should be unified among all targets and I also agree that the solution you proposed is a "workaround" until somebody (you) implements the unified solution. So I'm favorable to commit your version of vfprintf.

    But you still haven't answered my question about stdio.h: should it remain unchanged or you meant to apply the pic16-stdio-printf_large.patch? But the patch is probably only for printf_large. So you are right: I read the thread too superficially ;-)

    Borut

     
  • Mauro Giachero
    Mauro Giachero
    2008-09-15

    Hello Borut,

    stdio.h should remain untouched, the patch was only related to the printf_large experiment.

    Mauro

     
1 2 > >> (Page 1 of 2)