search

arduino-libraries / Ethernet

Ethernet Library for Arduino
http://arduino.cc/
252 stars 255 forks source link

MAX_SOCK_NUM is faulty determined in combination with Mega board #70

Open BertHav opened 5 years ago

BertHav commented 5 years ago

It seems that the number of sockets is determined in ethernet.h based on RAMSTART and RAMEND. But these facts are both defined for the mega chip instead of the W5100. They are both defined in arduino15/packages/arduino/tools/avr-gcc/4.9.2-atmel3.5.4-arduino2/avr/include/avr/iom2560.h Due to the fact that the Arduino Mega has more than 2048 bytes RAM the W5100 is faulty defined with 8 sockets. Arduino IDE 1.8.7 Ethernet library 2.0.0

Rotzbua commented 5 years ago

I agree that this misbehavior should be documented. I already mentioned it at https://github.com/arduino-libraries/Ethernet/pull/68#issuecomment-419288878 .

The reason is the chip type detection while run time and this do not work with variables while compile time ...

SapientHetero commented 5 years ago

Similarly, RAMSTART and RAMEND are not defined at all for the Adafruit Metro M4, which causes the library to allocate only 4 sockets for the W5500 instead of 8.

PaulStoffregen commented 5 years ago

Are you sure about that? The code is:

#if defined(RAMEND) && defined(RAMSTART) && ((RAMEND - RAMSTART) <= 2048)
#define MAX_SOCK_NUM 4
#else
#define MAX_SOCK_NUM 8
#endif

On SAMD or any board where either of these aren't defined, the first 2 checks should be false and it should end up as 8. All 3 have to be true for only 4.

SapientHetero commented 5 years ago

I am. Segger debugger showed sockindex with 4 array elements in EthernetClient. When I commented out the compiler directives and just set MAX_SOCK_NUM to 8, it showed 8 elements.

I found this while chasing a "Connection refused" issue with my web server. After fixing this and adding "Connection: keep-alive" to response headers I thought I'd fixed it but it still happens occasionally.

Any suggestions for where I should look next? Where in the library are connection requests refused?

On Tue, Oct 2, 2018, 2:29 PM Paul Stoffregen notifications@github.com wrote:

Are you sure about that? The code is:

if defined(RAMEND) && defined(RAMSTART) && ((RAMEND - RAMSTART) <= 2048)

define MAX_SOCK_NUM 4

else

define MAX_SOCK_NUM 8

endif

On SAMD or any board where either of these aren't defined, the first 2 checks should be false and it should end up as 8. All 3 have to be true for only 4.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/arduino-libraries/Ethernet/issues/70#issuecomment-426381910, or mute the thread https://github.com/notifications/unsubscribe-auth/AlmIIwwL2GSngTREwhaPpoEmGXVktLQFks5ug7B-gaJpZM4XCyxS .

PaulStoffregen commented 5 years ago

Regarding this:

Due to the fact that the Arduino Mega has more than 2048 bytes RAM the W5100 is faulty defined with 8 sockets.

The difficult design problem is Ethernet really does now support 8 sockets on Arduino Mega, if you connect a shield with the W5200 or W5500 chips.

SapientHetero commented 5 years ago

I'm running an Adafruit Metro M4. I posted here hoping to get this resolved for others who may lack the tools to find it.

I moved to the M4 after Mega RAM became the limiting factor for my application, so I appreciate the need for a solution. Do you think increasing buffer size for my M4 would improve performance? I've got RAM to burn.

On Tue, Oct 2, 2018, 2:42 PM Paul Stoffregen notifications@github.com wrote:

Regarding this:

Due to the fact that the Arduino Mega has more than 2048 bytes RAM the W5100 is faulty defined with 8 sockets.

The difficult design problem is Ethernet really does now support 8 sockets on Arduino Mega, if you connect a shield with the W5200 or W5500 chips.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/arduino-libraries/Ethernet/issues/70#issuecomment-426386381, or mute the thread https://github.com/notifications/unsubscribe-auth/AlmII3V8XSL6N0Z6IOduXho_GOsJeu_Hks5ug7OcgaJpZM4XCyxS .

PaulStoffregen commented 5 years ago

On this issue, please keep the conversation to MAX_SOCK_NUM.

For help with designing your application, post on the forums. For an Adafruit product, post on their forum first. This github issue tracker is for reporting defects in the Ethernet library. If you've found another defect, and you're sure it's an Ethernet library bug, start a new issue.

Edit: if you really have found a server socket connection bug, please invest some time to create a complete test case before you open another issue. I will not investigate if the issue has only code fragments or a vague description lacking specific details of how to reproduce the problem.

BertHav commented 5 years ago

Due to the fact that the Arduino Mega has more than 2048 bytes RAM the W5100 is faulty defined with 8 sockets.

The difficult design problem is Ethernet really does now support 8 sockets on Arduino Mega, if you connect a shield with the W5200 or W5500 chips.

Yes indeed, but the number of sockets does not depend on the processor type. So to my opinion this is a nice try, but a bogus definition. I also agree MAX_SOCK_NUM is widely spread, so removing is not an option. My proposal would be to let it static defined, as it was. The developer who applies an 8 socket chip understands how she/he can define 8 sockets. The socket definition in the current implementation method is weird and produces unpredictable results/instability.

PaulStoffregen commented 5 years ago

My proposal would be to let it static defined, as it was.

Could you be more specific?

Are you suggesting to forever make only half the sockets of the current generation chips available on all boards, without going into edit the library source?

SapientHetero commented 5 years ago

I agree. The existing code determines the number of hardware sockets available based on chip type. Let that be the default behavior. Keep

define MAX_SOCK_NUM as a user-settable manual limit but eliminate the

compiler directive logic surrounding it.

It makes sense to limit RAM consumption based on processor type, but I like the idea of an easy-to-use override. After a processor-based RAM use limit calculation, add a commented-out #define that lets the user override it if their application needs a different value.

On Wed, Oct 3, 2018, 4:03 PM BertHav notifications@github.com wrote:

Due to the fact that the Arduino Mega has more than 2048 bytes RAM the W5100 is faulty defined with 8 sockets.

The difficult design problem is Ethernet really does now support 8 sockets on Arduino Mega, if you connect a shield with the W5200 or W5500 chips.

Yes indeed, but the number of sockets does not depend on the processor type. So to my opinion this is a nice try, but a bogus definition. I also agree MAX_SOCK_NUM is widely spread, so removing is not an option. My proposal would be to let it static defined, as it was. The developer who applies an 8 socket chip understands how she/he can define 8 sockets. The socket definition in the current implementation method is weird and produces unpredictable results/instability.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/arduino-libraries/Ethernet/issues/70#issuecomment-426780926, or mute the thread https://github.com/notifications/unsubscribe-auth/AlmII5dN0QLVQKTrGc8m36cPB3nRbUj2ks5uhRgugaJpZM4XCyxS .

PaulStoffregen commented 5 years ago

I still have no idea what you're specifically suggesting.

BertHav commented 5 years ago

Are you suggesting to forever make only half the sockets of the current generation chips available on all boards, without going into edit the library source?

Yes, for instance for the next 3 years. It is very easy to (re)define to 8 sockets. Defaulting to 4 breaks nothing, defaulting to 8 produces errors and bogus information in a 4 socket chip.

SapientHetero commented 5 years ago

I disagree with the suggestion of hard coding a max of 4 sockets. If someone buys a W5500-based board specifically to get the advertised 8 sockets, why shouldn't the library automatically support them if it can?

The existing library already detects the chip type and correctly sets max_socks for the chip detected. The only thing preventing the library from getting it right for W5100, W5200 & W5500 is the precompiler logic that limits sockets based on RAMSTART & RAMEND.

Remove the RAMSTART logic & let the existing logic determine max_socks unless overridden manually by a user defining a lower value by setting MAX_SOCK_NUM.

On Thu, Oct 4, 2018, 1:14 PM BertHav notifications@github.com wrote:

Are you suggesting to forever make only half the sockets of the current generation chips available on all boards, without going into edit the library source?

Yes, for instance for the next 3 years. It is very easy to (re)define to 8 sockets. Defaulting to 4 breaks nothing, defaulting to 8 produces errors and bogus information in a 4 socket chip.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/arduino-libraries/Ethernet/issues/70#issuecomment-427098759, or mute the thread https://github.com/notifications/unsubscribe-auth/AlmII2dM7iBFiQYwukeMzpv-MVK1swVgks5uhkHdgaJpZM4XCyxS .

SapientHetero commented 5 years ago

Are you sure about that? The code is:

#if defined(RAMEND) && defined(RAMSTART) && ((RAMEND - RAMSTART) <= 2048)
#define MAX_SOCK_NUM 4
#else
#define MAX_SOCK_NUM 8
#endif

On SAMD or any board where either of these aren't defined, the first 2 checks should be false and it should end up as 8. All 3 have to be true for only 4.

Just realized why this logic doesn't work.

"Conditionals written like this: #if defined BUFSIZE && BUFSIZE >= 1024 can generally be simplified to just #if BUFSIZE >= 1024, since if BUFSIZE is not defined, it will be interpreted as having the value zero. "

https://gcc.gnu.org/onlinedocs/cpp/Defined.html

PaulStoffregen commented 5 years ago

Remove the RAMSTART logic

If we do this, users with Arduino Uno & Nano (the most popular boards) will have too much RAM used. Those chips have only 2K RAM.

SapientHetero commented 5 years ago

Perhaps detecting the board type directly instead of inferring it is a better answer.

if defined(TEENSYDUINO)

//  --------------- Teensy -----------------
#if defined(__AVR_ATmega32U4__)
    #define BOARD "Teensy 2.0"
#elif defined(__AVR_AT90USB1286__)
    #define BOARD "Teensy++ 2.0"
#elif defined(__MK20DX128__)
    #define BOARD "Teensy 3.0"
#elif defined(__MK20DX256__)
    #define BOARD "Teensy 3.2" // and Teensy 3.1 (obsolete)
#elif defined(__MKL26Z64__)
    #define BOARD "Teensy LC"
#elif defined(__MK64FX512__)
    #define BOARD "Teensy 3.5"
#elif defined(__MK66FX1M0__)
    #define BOARD "Teensy 3.6"
#else
   #error "Unknown board"
#endif

else

// --------------- Arduino ------------------
#if   defined(ARDUINO_AVR_ADK)
    #define BOARD "Mega Adk"
#elif defined(ARDUINO_AVR_BT)    // Bluetooth
    #define BOARD "Bt"
#elif defined(ARDUINO_AVR_DUEMILANOVE)
    #define BOARD "Duemilanove"
#elif defined(ARDUINO_AVR_ESPLORA)
    #define BOARD "Esplora"
#elif defined(ARDUINO_AVR_ETHERNET)
    #define BOARD "Ethernet"
#elif defined(ARDUINO_AVR_FIO)
    #define BOARD "Fio"
#elif defined(ARDUINO_AVR_GEMMA)
    #define BOARD "Gemma"
#elif defined(ARDUINO_AVR_LEONARDO)
    #define BOARD "Leonardo"
#elif defined(ARDUINO_AVR_LILYPAD)
    #define BOARD "Lilypad"
#elif defined(ARDUINO_AVR_LILYPAD_USB)
    #define BOARD "Lilypad Usb"
#elif defined(ARDUINO_AVR_MEGA)
    #define BOARD "Mega"
#elif defined(ARDUINO_AVR_MEGA2560)
    #define BOARD "Mega 2560"
#elif defined(ARDUINO_AVR_MICRO)
    #define BOARD "Micro"
#elif defined(ARDUINO_AVR_MINI)
    #define BOARD "Mini"
#elif defined(ARDUINO_AVR_NANO)
    #define BOARD "Nano"
#elif defined(ARDUINO_AVR_NG)
    #define BOARD "NG"
#elif defined(ARDUINO_AVR_PRO)
    #define BOARD "Pro"
#elif defined(ARDUINO_AVR_ROBOT_CONTROL)
    #define BOARD "Robot Ctrl"
#elif defined(ARDUINO_AVR_ROBOT_MOTOR)
    #define BOARD "Robot Motor"
#elif defined(ARDUINO_AVR_UNO)
    #define BOARD "Uno"
#elif defined(ARDUINO_AVR_YUN)
    #define BOARD "Yun"

// These boards must be installed separately:
#elif defined(ARDUINO_SAM_DUE)
    #define BOARD "Due"
#elif defined(ARDUINO_SAMD_ZERO)
    #define BOARD "Zero"
#elif defined(ARDUINO_ARC32_TOOLS)
    #define BOARD "101"
#else
   #error "Unknown board"
#endif

endif

On Oct 8, 2018 11:11 AM, "Paul Stoffregen" notifications@github.com wrote:

Remove the RAMSTART logic

If we do this, users with Arduino Uno & Nano (the most popular boards) will have too much RAM used. Those chips have only 2K RAM.