Feedback on multi-platform nature of Warning errors found on Due

[Paul Carpenter]

Not seen anything in github or elsewhere, (but could have missed it). If I have missed something as bug or documentation link let me know.

Some signed and unsigned issues from writing a DUE application and testing my own libraries for multi-platform use.

Was using Arduino 1.6.5 and Due package from Boards Manager Due 1.6.4 on Windows XP platform, several warning errors in CORE libraries so upgraded to latest stable Arduino 1.6.7 and Boards Manager Due 1.6.6, which removed some standard library warnings, but not all.

Base Test is compile with ALL warnings using a blank project (empty setup and loop)

    Arduino Uno     NO errors
    Arduino Due     2 signed/unsigned comparison errors
   
When compiling my library example I also get some avr/pgmspace.h substitution warnings, which I need to track down for Due not Uno (i.e. on ARM not AVR), which may well be needing tight brackets or similar to allow substitutions to work correctly or other errors of my doing.

First warning reported on core library file

C:\[...]\Arduino15\packages\arduino\hardware\sam\1.6.6\cores\arduino\Stream.cpp:167:10: warning: comparison is always false due to limited range of data type [-Wtype-limits]

   if(c < 0)

          ^

Extract from Stream.cpp (lines 158 to 168)

float Stream::parseFloat(char skipChar){

  bool isNegative = false;
  bool isFraction = false;
  long value = 0;
  char c;
  float fraction = 1.0;

  c = peekNextDigit();
    // ignore non numeric leading characters
  if(c < 0)
    return 0; // zero returned if timeout

This code segment is the same for AVR and ARM libraries, compiles fine in AVR for Uno or Mega, but fails for Due. So the timeout will not be honoured and potential corrupted data. This may be rare.

My SHORT term fix is to change definition to 'signed char c' to make this code section work properly. This works for me but need to know how this may affect others if at all.

However this has other related issues

1. Arduino Reference states that char is signed across platforms so example code or applications that should  work across platforms, cannot, but probably rare occurrence as signed nature of char might not be being used often.  
2. What other platforms may be affected as Stream.cpp is probably a straight copy across platforms.
3. Not found yet where in compiler tree and arduino files char has been made unsigned for Due as all standard compiler headers for types, stdint etc.. have it as unsigned, that I have SO FAR checked.
4. What tests to ensure any change will not break other things if the compiler is made to match usage  as per AVR architecture.

Could do with feedback before I sort out github pull and issue tracking for input on other platforms and above

Second warning reported on core library file

C:\[...]\Arduino15\packages\arduino\hardware\sam\1.6.6\cores\arduino\UARTClass.cpp:153:34: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

     while (_tx_buffer->_iTail == l)

                                 ^

First issue with this -

That is '== l' a single letter variable named lower case L, BAD case of variable name as that is easily confused
with a 1 (number one) in some fonts. Warning message on its own confused me at first.

Likewise I suggest others NOT use single letter variables especially certain characters, as MANY fonts on screen or print can
be confusing in code such as -

•     S - upper case S confusable with 5
•     l or L - lower or upper case L confusable with 1 in various fonts
•     O - upper case O
•     B or D - upper case B and D confusable with 8 and 0 respectively
•     I - upper case I can be confused with 1

Second issue and comment in other file issue

The code that causes this error is UARTClass.cpp (lines 151 to 154) function

 size_t UARTClass::write( const uint8_t uc_data )

    // If busy we buffer
    unsigned int l = (_tx_buffer->_iHead + 1) % SERIAL_BUFFER_SIZE;
    while (_tx_buffer->_iTail == l)
      ; // Spin locks if we're about to overwrite the buffer. This continues once the data is sent

    _tx_buffer->_aucBuffer[_tx_buffer->_iHead] = uc_data;
    _tx_buffer->_iHead = l;

From RingBuffer.h

// Define constants and variables for buffering incoming serial data.  We're
// using a ring buffer (I think), in which head is the index of the location
// to which to write the next incoming character and tail is the index of the
// location from which to read.
#define SERIAL_BUFFER_SIZE 128

class RingBuffer
{
  public:
    volatile uint8_t _aucBuffer[SERIAL_BUFFER_SIZE] ;
    volatile int _iHead ;
    volatile int _iTail ;

Two things to note here

1/  I suggest removal of " (I think)" from second line of comment from RingBuffer.h

2/  volatile int _iTail is a SIGNED int so comparison will always be of wrong type to block scope variable unsigned int l

Whilst on AVR HardwareSerial.cpp in its write function doing same thing uses

  tx_buffer_index_t i = (_tx_buffer_head + 1) % SERIAL_TX_BUFFER_SIZE;

So that the types match as both variables are of same type.

Yes it probably will never be a problem with realtively small buffer sizes, but reducing warning errors to zero is worth while.

Suggested fix as far as I am aware the index to an array (which this buffer is) can for this compiler be a max size of a signed int anyway, calculations on right hand side start with signed int as well, so suggested code changes are to correct warning and improve readability, by changing signed type and name of variable (and its other places of use).

    int nextWrite = (_tx_buffer->_iHead + 1) % SERIAL_BUFFER_SIZE;
    while (_tx_buffer->_iTail == nextWrite)
      ; // Spin locks if we're about to overwrite the buffer. This continues once the data is sent

    _tx_buffer->_aucBuffer[_tx_buffer->_iHead] = uc_data;
    _tx_buffer->_iHead = nextWrite;

This may affect other platforms, but suggest others check

As reminder my suggested fixes would affect THREE files -

1. Stream.cpp
2. RingBuffer.h  (comment change only)
3. UARTClass.cpp

[Andrew Kroll]

Variable c should be an signed char, then your code will work on all platforms without complaining.  
Never assume signedness, ever!

This is a bug waiting to bite someone.

Correct. Same as above, another bug waiting to bite.