arduino-libraries / Ethernet

Ethernet Library for Arduino
http://arduino.cc/
259 stars 264 forks source link

Avoid conflicts with similar declaration of MAX_SOCK_NUM #68

Open agdl opened 6 years ago

PaulStoffregen commented 6 years ago

Why is this needed? What other code is conflicting?

Ethernet has exposed this define since the earliest days of Arduino, since at least 2008 (long before Arduino was using github).

agdl commented 6 years ago

@PaulStoffregen using for example MKRWiFi1010 + MKR ETH Shield. I already Changed the WiFi NINA here https://github.com/arduino-libraries/WiFiNINA/commit/00c9caacb2f6e4df94c26a8c393ad60824b5a4f1 so this isn't really necessary, but will be nice to align the defines names. That's it!

Rotzbua commented 6 years ago

A perfect example why namespaces were introduced into programming :wink:

PaulStoffregen commented 6 years ago

If this is changed, I believe the proper approach would be the bring it inside EthernetClass, rather than keeping it as a preprocessor macro. I had considered doing this for version 2.0.0, but I intentionally left things the way they were...

The main question I have is whether we should keep MAX_SOCK_NUM defined for backwards compatibility? That is the reason I left it in 2.0.0. Removing MAX_SOCK_NUM from newer libs is much easier. Ethernet has exposed this define to Arduino sketches and libraries for at least 10 years. Many people have used it.

PaulStoffregen commented 6 years ago

For example, here are several Arduino sketches where users have relied on MAX_SOCK_NUM.

https://github.com/seth1993/Spectro/blob/f744c7b05c09474544b4081920633bbafd870c80/ArduinoTempSensor/libraries/bitlash/examples/ethernetcommander/ethernetcommander.ino#L144

https://github.com/kmpelectronics/Arduino/blob/ea8d03cec359f7b185883314fb34827543bf366e/KMPDinoEthernet/src/KMPDinoEthernet/examples/projects/ProDinoIntRelay/ProDinoIntRelay.ino#L136

https://github.com/neophob/SkyInvaders/blob/master/Arduino/SkyInvaders/WOL.ino#L42

https://github.com/mistergreen/WAAC/blob/9efd403f7a2be7a58f8450c78f2915c47cda58b8/Arduino_Due/WAAC_ethernet_1.8.1/WAAC_ethernet_1.8.1.ino#L913

https://github.com/seth1993/Spectro/blob/f744c7b05c09474544b4081920633bbafd870c80/ArduinoTempSensor/libraries/bitlash/examples/BitlashWebServer/BitlashWebServer.ino#L283

https://github.com/omidomidi/Smart-Home/blob/64f4d619bf23eb6b37e4a8c95e3d1af8d8d7e0f7/Humidity_Temperature_Sensor/examples/CC3200_WeatherWebServer/CC3200_WeatherWebServer.ino#L321

Several of these, specially Bill Roy's code, have been very widely copied into many people's projects. If we remove or rename MAX_SOCK_NUM, we will almost certainly break all of these programs!

Rotzbua commented 6 years ago

I think with version 2.0.0 MAX_SOCK_NUM is not fully backwards compatible. Because MAX_SOCK_NUM is defined at compile time. If it enough ram is available, it is set to 8. But if a w5100 is found at runtime, it is wrong because a w5100 only support 4 sockets.

https://github.com/arduino-libraries/Ethernet/blob/6e9dffa64f6b0eb89607dbb5293dc33be82cc39e/src/Ethernet.h#L36-L40

BTW The pr is incomplete. The definition of MAX_SOCK_NUM is not renamed.

CLAassistant commented 3 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.