Pages: [1]   Go Down
Author Topic: RAMEND bug in HardwareSerial.cpp?  (Read 5580 times)
0 Members and 1 Guest are viewing this topic.
0
Offline Offline
Edison Member
*
Karma: 44
Posts: 1471
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

The following is from Arduino 0022 at line 40 of HardwareSerial.cpp
Code:
#if (RAMEND < 1000)
  #define RX_BUFFER_SIZE 32
#else
  #define RX_BUFFER_SIZE 128
#endif

This will always use a buffer size of 128 since RAMEND is always over 1000.

On an ATmega168P RAMEND is 0x4FF or 1279 decimal.  Line 820 of iom168p.h
Code:
#define RAMEND       0x4FF     /* Last On-Chip SRAM Location */
On an ATmega8 RAMEND is 0x45F or 1119 decimal.  Line 568 of iom8.h
Code:
#define RAMEND             0x45F
Logged

Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12285
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset


Why do you think this is a bug?
Logged

0
Offline Offline
Edison Member
*
Karma: 44
Posts: 1471
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Why do you think it's not a bug?

0021 had only the single line
Code:
#define RX_BUFFER_SIZE 128

0022 added the conditional above which always results in RX_BUFFER_SIZE being 128.  Why add a conditional that is never used?

Many people with a 168 Arduino edited this file to reduce the buffer size.  The new code would be nice if it reduced the buffer size for a 168 Arduino.
Logged

Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12285
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Why add a conditional that is never used?
You assume the Arduino IDE is only used with Arduino boards.  Might that conditional be relevant for ATtiny processors?

Quote
The new code would be nice if it reduced the buffer size for a 168 Arduino.

To what size?
Logged

Offline Offline
God Member
*****
Karma: 32
Posts: 506
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Do you think the original programmer meant to use RAMSIZE rather than RAMEND?

It does seem to be a bug to compare a memory address with a number. RAMSIZE would be the actual memory size, RAMEND just happens to be the last address and could be anywhere depending on the internal memory map.
Logged


0
Offline Offline
Edison Member
*
Karma: 44
Posts: 1471
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Looks like the mod was done by Mark Sproul.

There is a comment about the atmega8535 so it could be for that processor.

Bug or not it would be nice to use a smaller buffer on processors with 1K of RAM.
Logged

Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12285
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
Do you think the original programmer meant to use RAMSIZE rather than RAMEND?
My suspicion is that RAMEND was used because of an eccentricity in the the header files.  There's an E2END (EEPROM_END) but no E2SIZE.  If the developer was familiar with the EEPROM constant, I can see how one would assume there is a RAMEND but no RAMSIZE.

Quote
It does seem to be a bug to compare a memory address with a number. RAMSIZE would be the actual memory size,
I agree.  RAMSIZE is the correct constant to use.

Quote
RAMEND just happens to be the last address and could be anywhere depending on the internal memory map.
I believe on the AVR processors, RAMEND is always RAMSIZE - 1.

Quote
Bug or not it would be nice to use a smaller buffer on processors with 1K of RAM.
Size?
Logged

Offline Offline
God Member
*****
Karma: 32
Posts: 506
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Unfortunately it seems that RAMSIZE isn't always defined for each processor. What is and isn't defined is a bit hit and miss smiley-sad

I can't see an easy way to improve on the code that's already there. Using RAMEND is a reasonable compromise if RAMSIZE isn't available.
Logged


Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12285
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Code:
[glow]#if ! defined( RAMSIZE )
#define RAMSIZE ( RAMEND + 1 )
#endif[/glow]

#if (RAMSIZE <= 1000)
  #define RX_BUFFER_SIZE 32
#else
  #define RX_BUFFER_SIZE 128
#endif
smiley
Logged

0
Offline Offline
Edison Member
*
Karma: 44
Posts: 1471
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I started this topic after helping a user of one of my libraries with a common problem.

The user put a Serial.println() debug statement in a sketch that will not use the UART after it is finished.  The user has a 168 Arduino.

An extra 128 bytes of receive buffer is allocated even though no serial data is read.  The use of this extra RAM caused his sketch to crash.

Too bad no buffer is allocated unless you used a serial input function like Serial.read or Serial.available.

I am working on a GPS logger where I would like more than 128 bytes for the serial buffer.  I have enabled every possible NMEA sentence from the GPS and it blasts out about 600 bytes at the start of every second.

A simple way to override the default receive buffer would help.

I started fighting serial I/O in 1971 when I designed a large terminal concentrator at one of the national labs.  I guess my struggle with serial I/O will never end.
Logged

Offline Offline
God Member
*****
Karma: 32
Posts: 506
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Why not write an alternative hardware serial library, one with adjustable/optional buffers? (Is there some technical reason why that would not work? ie is Arduino tied to its own Serial class somehow?)
Logged


SF Bay Area (USA)
Online Online
Tesla Member
***
Karma: 106
Posts: 6364
Strongly opinionated, but not official!
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

In recent Arduino cores, a lot of changes were added to support additional processors, mostly by making feature inclusion dependent upon the definitions defined by the processor include files, instead of on the processor name.  So while the old HardwareSerial.cpp code would contain
Code:
#if defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
ring_buffer rx_buffer1 = { { 0 }, 0, 0 };
ring_buffer rx_buffer2 = { { 0 }, 0, 0 };
ring_buffer rx_buffer3 = { { 0 }, 0, 0 };
#endif
While the new code does  
Code:
#if defined(UBRRH) || defined(UBRR0H)
  ring_buffer rx_buffer  =  { { 0 }, 0, 0 };
#endif
#if defined(UBRR1H)
  ring_buffer rx_buffer1  =  { { 0 }, 0, 0 };
#endif
#if defined(UBRR2H)
  ring_buffer rx_buffer2  =  { { 0 }, 0, 0 };
#endif
#if defined(UBRR3H)
  ring_buffer rx_buffer3  =  { { 0 }, 0, 0 };
#endif
So support for a particular serial port is only included if the constants for that particular serial port are included in the definition associated with the cpu named passed from the compile line (assuming normal avr-gcc usage.)

The intent of the RAMSIZE conditional you found is almost certainly to support the arduino serial code on CPU that have even less memory than any of the "real" Arduinos, while keeping the behavior on the real Arduinos the "same as it always was."

Quote
A simple way to override the default receive buffer would help.
Ah.  That would be an issue that is only peripherally related; your user's code would have crashed with any of the older versions of the core as well.
Since the allocated buffer will be used by the ISR regardless of whether the user ever calls Serial.read(), it's a bit hard to omit.  The compile/link isn't smart enough to exclude the rx ISR based on the fact that the user never calls the user-level read functions.

Quote
I started fighting serial I/O in 1971 ...  I guess my struggle with serial I/O will never end.
:-) Ah.  Another expert in "legacy technology."  Even older than me!  (supdup and xmodem for tops10, circa 1979)  Hah!  No, I don't expect it to ever end.  At least USB made it easier for users.  Mostly.  Sort of.  Fewer cable problems, anyway...
Logged

Global Moderator
Dallas
Offline Offline
Shannon Member
*****
Karma: 176
Posts: 12285
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset


Why are serial recieve and serial send bound together?  The only technical reason I can find is initialization.  From a resource perspective, it would certainly make sense to separate them.  Should they be split?  Would it be too confusing?

@fat16lib: For debugging would "Tiny Debug Serial" help?  It's a write-only software serial.
Logged

0
Offline Offline
Edison Member
*
Karma: 44
Posts: 1471
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Forty years ago we knew strong coupling between receive and transmit in communications software caused problems.

There is no fundamental reason the receive ISR and buffers need to be loaded when you only need transmit.  I have written C++ classes to do it but it would change the Serial api.

I suspect"Tiny Debug Serial" would works fine.

The problem really needs to be fixed in the standard Arduino distribution and that's not likely to happen.






Logged

Pages: [1]   Go Up
Jump to: