Project

General

Profile

Actions

Bug #3504

closed

wformat and long printing

Added by tsaitgaist over 5 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
firmware
Target version:
-
Start date:
08/28/2018
Due date:
% Done:

100%

Spec Reference:

Description

https://git.osmocom.org/simtrace2/commit/?id=c394109964a18d05d46de46728f617465f3493c4 enabled Wformat compiler errors.
This for example warns about the following:

./atmel_softpack_libraries/libchip_sam3s/include/trace.h:187:40: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'RoReg' {aka 'const volatile long unsigned int'} [-Wformat=]
 #define TRACE_INFO(...)       { printf("-I- " __VA_ARGS__); }
                                        ^~~~~~
apps/trace/main.c:168:2: note: in expansion of macro 'TRACE_INFO'
  TRACE_INFO("Chip ID: 0x%08x (Ext 0x%08x)\n\r", CHIPID->CHIPID_CIDR, CHIPID->CHIPID_EXID);
  ^~~~~~~~~~
apps/trace/main.c:168:28: note: format string is defined here
  TRACE_INFO("Chip ID: 0x%08x (Ext 0x%08x)\n\r", CHIPID->CHIPID_CIDR, CHIPID->CHIPID_EXID);
                         ~~~^
                         %08lx

RoReg is defined as follows:

53:typedef volatile const uint32_t RoReg; /**< Read only 32-bit register (volatile const unsigned int) */

SAM3S is a 32-bit platform, where int and long both correspond to 32-bit integers.
gcc says uint32_t correspond to unsigned long because is is defined as being at least 16-bit long, while long is defined as being at least 32-bit long.
Because RoReg is now considered as long, Wformat recommends to use "%l" (in out case %lx".

the atmel printf does not provide %l support:
libcommon/source/stdio.c vsnprintf

    // Normal character
        if (*pFormat != '%') {

            *pStr++ = *pFormat++;
            size++;
        }
        // Escaped '%'
        else if (*(pFormat+1) == '%') {

            *pStr++ = '%';
            pFormat += 2;
            size++;
        }
        // Token delimiter
        else {

            fill = ' ';
            width = 0;
            pFormat++;

            // Parse filler
            if (*pFormat == '0') {

                fill = '0';
                pFormat++;
            }

            // Parse width
            while ((*pFormat >= '0') && (*pFormat <= '9')) {

                width = (width*10) + *pFormat-'0';
                pFormat++;
            }

            // Check if there is enough space
            if (size + width > length) {

                width = length - size;
            }

            // Parse type
            switch (*pFormat) {
            case 'd': 
            case 'i': num = PutSignedInt(pStr, fill, width, va_arg(ap, signed int)); break;
            case 'u': num = PutUnsignedInt(pStr, fill, width, va_arg(ap, unsigned int)); break;
            case 'x': num = PutHexa(pStr, fill, width, 0, va_arg(ap, unsigned int)); break;
            case 'X': num = PutHexa(pStr, fill, width, 1, va_arg(ap, unsigned int)); break;
            case 's': num = PutString(pStr, va_arg(ap, char *)); break;
            case 'c': num = PutChar(pStr, va_arg(ap, unsigned int)); break;
            default:
                return EOF;

using %l causes a bug were the previous buffer is reread:

=============================================================================
SIMtrace2 firmware 0.4.159-e076-dirty (C) 2010-2016 by Harald Welte          
=============================================================================
-I- Chip ID: 0x==================================================================
SIMtrace2 firmware 0.4.159-e076-dirty (C) 2010-2016 by Harald Welte              
=============================================================================
-I- Serial Nr. 51203120-38463850-33303231-36313035 

laforge should we re-disable Wformat or add (void) %l support to stdio?

Actions #1

Updated by tsaitgaist over 5 years ago

a bypass would be to cast the RoReg variables as (unsigned int) when printing, but this is not pretty.

alternatively we could tells gcc that uint32_t is safe for be considered as unsigned int instead of using long, but I did not find how to do that.

Actions #2

Updated by laforge over 5 years ago

On Tue, Aug 28, 2018 at 06:26:45AM +0000, tsaitgaist [REDMINE] wrote:

laforge should we re-disable Wformat or add (void) %l support to stdio?

I think the latter is the way to go, thanks!

Actions #3

Updated by laforge over 5 years ago

I'm not quite sure how you conclude it's not possible.

The current PLLA configuration seems to put MCK at 18.432 MHz * 13 / 5 = 47.9232 MHz
(see common/source/board_lowlevel.c with common/incllude/board_common.h)

Dividing this by 52 will render exactly: 921600 !

The result would be different if we eventually want to clock the SAM3 at a higher
core clock rate, closer to the 64MHz it supports. However, we're not doing that
so far as there hasn't been a need so far.

Actions #4

Updated by laforge over 5 years ago

  • if we stay at 48 MHz, we can render perfect 921600
  • if we ever want to go for higher speeds:

The fastest CPU speed I could find supporting 921600 from 18.432 MHz
XTAL is: 18.432 * 16 / 5 = 58.9824 with a baud-rate divider of 64
renders exactly 921600

For a 12 MHz XTAL system (QMOD), we can do 12.000 * 29 / 6 = 58.000 MHz
with a baud-rate divider of 63 renders 920635 (a 1.05% Error 921600) which
should still be acceptable.

Actions #5

Updated by tsaitgaist over 5 years ago

laforge wrote:

  • if we stay at 48 MHz, we can render perfect 921600

For a 12 MHz XTAL system (QMOD), we can do 12.000 * 29 / 6 = 58.000 MHz
with a baud-rate divider of 63 renders 920635 (a 1.05% Error 921600) which
should still be acceptable.

right, the 48 MHz only apply to the sysmoQMOD. With simtrace itself 921600 works, and I used it to debug DFU.
I will try changing the clock for the sysmoQMOD and see if it affects something else.
thanks for the tip.

Actions #6

Updated by tsaitgaist over 5 years ago

here are the results.

for simtrace (and owhw):
- 18.432 MHz * 13 / 5 = 47.9232 MHz is what is currently, but won't work for outputing 921600. the dividing factor is 52, but we can't set this value. the set value is CR, and the resulting dividing value is 16 * CR.
- instead I used 18.432 * 16 / 5 = 58.982400, dividing value is 64, CR is 4
- trace/sniff works

for qmod:
- I used 12.000 * 29 / 6 = 58.000 MHz, ideal dividing value is 63 (which can't be set), actual dividing value is 64, resulting error is 1.694%. it is decoded fine by the CP2102 and CP2104
- the least error rate (with MCK between 48 and 64 MHz) is 12.000 * 34 / 7 = 58. 285714285 MHz (not even), with and error rate of 1.195%. but 58.000 Mhz is the second least error rate, and works fine
- cardem/remsim also works fine

Actions #7

Updated by tsaitgaist over 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100
Actions

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)