arduino-libraries / Ethernet

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

Embrace more network chips (W5200 and W5500 at least) to be more generic #37

Closed q2dg closed 6 years ago

q2dg commented 8 years ago

My main reasoning is here (https://github.com/arduino/Arduino/issues/5170) but it was centered about web documentation, so I think maybe here is the right place to explain the issue. Let's go:

None of current official Arduino products has Ethernet conectivity (sadly). So...why keep maintaining this library? If the plan is to be not tied to any specific official Arduino product, then I think this library should be more generic (as it is this: https://github.com/Wiznet/WIZ_Ethernet_Library, at least). Current situation is no sustainable because W5100 is vanishing little by little and currently this library doesn't offer other alternative. It's the same case as TFT library...if it were generic, maybe someone used it (as it is used LiquidCrystal library, for instance) but being tied to an specific chip, if this chip is not associated to an official product, it's a dead-end.

Another solution would be offer a new shining product: an Ethernet shield with TLS integrated chip and SD card , (and therefore, modify this library accordingly)...but I suspect this is not the plan

njh commented 8 years ago

See also: https://github.com/adafruit/Ethernet2

q2dg commented 8 years ago

Well, in fact, now THE Arduino Ethernet shield is the former .org's Arduino Ethernet 2, based on W5500 (https://www.arduino.cc/en/Main/ArduinoEthernetShield) so...I think official library should be updated accordingly. But it's just an opinion.

EDIT: In fact, https://www.arduino.cc/en/Reference/EthernetV2 returns nothing

PaulStoffregen commented 8 years ago

Please please please do not just use Arduino.org's Ethernet2 library. It's based on a very old copy of Ethernet, from before SPI transactions.

It's not difficult to make an Ethernet library which automatically detects the chip and works with all 3 SPI-interface Wiznet parts. My own fork of Ethernet has long auto-detected W5100 & W5200, and just recently I added support for W5500.

https://github.com/PaulStoffregen/Ethernet

My fork also has quite a lot of optimization work in the socket layer. It runs much faster, but this does cost some extra RAM usage. So perhaps my fork would be best to use on the ARM boards like Zero & Due, and another fork with the original socket layer could be used on AVR?

Just yesterday, only hours after finally getting my W5500 code to work reliably, I (re)discovered an auto-detecting contribution has been pending for quite a long time.

https://github.com/arduino/Arduino/pull/2325

Normally I don't comment on these sorts of "Arduino ought to do XYZ" threads, especially when it's about hardware product choices. But please, for the sake of so many Arduino users who will use this shield together with other libraries, I beg of you, please please please don't publish a sloppy least-possible-effort hack Ethernet2. My fork or that old contribution can make everything "just work" automatically for everyone on all Wiznet-based shields, and preserving the SPI transaction support will spare a lot of users who try to combine Ethernet with SPI-based libs using interrupts from terribly painful frustration.

njh commented 8 years ago

I was completely unaware of your work Paul, and I suspect most other Arduino users will be too. I have not tried it myself but it sounds like a real improvement.

I'm personally unaware of how the official Arduino development model works and how something becomes official/supported. I don't quite understand how there can be an Arduino Ethernet Shield being sold in high street stores around the world, but without proper library support.

Unfortunately at the moment, the @adafruit repo is the most official W5500 library out there. And I suspect that only came into existence because of the lack of official W5500 support from Arduino.

PaulStoffregen commented 8 years ago

I was also completely unaware Arduino.cc was adopting Arduino.org's W5500-based shield. Guess it makes sense now that they're starting to settle, but still news to me.

As a general rule, I publish my work on github, but I don't "evangelize" too much. That's usually just a recipe for frustration.

q2dg commented 8 years ago

I think this discussion deserves a lot more visibility. Maybe in developers mail list, don't you think?

In general, the degree of support of "new kids in town" (as well as "returned kids" like Due or old Wifi Shield -wtf??-) is a real mistery.

I'd like to know some roadmap (in regards to hardware stock and support) because I'm starting to be very dazzed, confused and tired. Tje worst part is I suspect there's no such roadmap.

PaulStoffregen commented 8 years ago

I think this discussion deserves a lot more visibility. Maybe in developers mail list, don't you think?

I'd suggest patience. At the very least wait until next week, since today is a major USA holiday. When/if you do post, perhaps take a gentle tone, avoid the instance you've previously had about Arduino's hardware product decisions, and focus on ease-of-use benefits for users.

PaulStoffregen commented 7 years ago

I've been thinking about this a bit over the last couple days.

Instead of asking the Arduino devs to put their own time and effort into evaluating these many Ethernet library changes, perhaps I should clean up my version meant for Teensy and retest it on various non-Teensy boards, and then publish with a distinct name.

Then you (and perhaps others) could suggest to people on the Arduino forums who are having issues with Ethernet to try the optimized, works-with-all-chips version. Ideally, they could be encouraged to leave feedback somewhere, even if only "me too".

After it's been used with good results by hundreds or even dozens of real people across the many different types of modern Arduino hardware, then we could make a compelling argument to the Arduino devs to just replace the old Ethernet. They could dig into the code and technical details if they like, but offering them a well tested replacement would probably work much better than asking them to merge many different patches and improvements, only to then need to begin testing.

njh commented 7 years ago

That sounds like a very good idea @PaulStoffregen.

Would also be good to get it into the Arduino library manager with its distinct name.

PaulStoffregen commented 7 years ago

Yes, will soon (next couple weeks) submit distinctly named version for the library manager.

Right now I'm (again) working on trying to figure out why all these libraries are so very slow. On WebClient, Ethernet2 achieves about 10 kbytes/sec. My optimizations speed it up to 95 kbytes/sec. But that's still pretty terrible. In theory, about 1 Mbyte/sec ought to be possible with 12 or 14 MHz SPI clock and W5500's protocol overhead.

PaulStoffregen commented 7 years ago

I see you just posted on the developers mail list. Apparently patience is difficult. Would have been better to wait another week or two.

PaulStoffregen commented 7 years ago

I'm still looking into why W5200 and W5500 are so much slower when allocating larger per-socket buffers. I purchased a network tap, so I can better see what's really happening. It's supposed to arrive early next week.

PaulStoffregen commented 6 years ago

FWIW, I've continued to work on my fork of Ethernet. https://github.com/PaulStoffregen/Ethernet

It supports & auto-detects all 3 wiznet chips. I put in optimized I/O for chip select, and I adopted Adafruit's extension for configuring which pin is used. I've added caching of socket state, which dramatically reduces the SPI communication overhead for TCP and even UDP when accessed as less than whole packets.

Someday I believe my fork should replace the official Ethernet lib....

cmaglie commented 6 years ago

Someday I believe my fork should replace the official Ethernet lib...

@PaulStoffregen OK, let's do it. How would you like to proceed?

(we are going to lost our https://github.com/arduino-libraries/Ethernet/commits/experimental-v1.2 branch... but you know... that's life... :-))

PaulStoffregen commented 6 years ago

I'll go through the 1.2 branch and merge anything that's not already implemented. Then we won't lose anything. I'd also like a little time to retest with the many boards I have here. Let's plan on mid-January time frame. Is that ok?

cmaglie commented 6 years ago

Sounds good, thanks Paul.

cmaglie commented 6 years ago

@PaulStoffregen I'm wondering what's the best plan to integrate your changes, I see that your repository started as a copy (not as a fork) so the two repositories have no common ancestors and requires a bit of work to be successfully merged. Removing one repository in favor of the other will delete history/issues/pr, and this doesn't look a good plan either.

On the other end I need to have a W5500 support implemented soon, so I'm for merging the experimental-1.2 branch for now, unless you have a better solution? I don't know if you have time right now to look into this, if you want we can plan to merge your change later?

PaulStoffregen commented 6 years ago

Oh, thanks for the reminder. Must admit, I've not really touched Ethernet for months. Spent most of this time on a custom discovery manager (how 3rd party boards should provide their own discovers is still a question....)

Will merged Ethernet by July 8 work for you, timing wise? Before merging, I'd really like to retest on all my Arduino and Arduino compatible boards. Middle of next week is a big US holiday. July 8 should be plenty of time.

I'm not concerned about preserving the commit history and issues of my repository. Do you need this?

If it's ok with you, I'd like to call this new code version 2.0.0. It's a major change. Hopefully increasing the major version number will communicate that to users.

Can you allow me commit access to this repository? Kinda makes sense for me to be able to do maintenance.

cmaglie commented 6 years ago

Will merged Ethernet by July 8 work for you, timing wise?

Works for us. I've given you write access to this repository. Version 2.0.0 is ok even if this is not strictly a breaking change.

The experimental-1.2 branch has some features like the linkStatus() method that I'd like to see in the new library. If you can port your patches on top of this branch it would be great, otherwise, if it is too much work, we can backport our features later, up to you :-).

PaulStoffregen commented 6 years ago

Should I make pin 5 the default for CS when any of the MKR boards are selected?

How should I detect this case? So far I'm considering something like this:

#if defined(PIN_SPI_MISO) && PIN_SPI_MISO == 10
akash73 commented 6 years ago

+1

Il giorno mar 3 lug 2018 alle 13:50 Paul Stoffregen < notifications@github.com> ha scritto:

Should I make pin 5 the default for CS when any of the MKR boards are selected?

How should I detect this case? So far I'm considering something like this:

if defined(PIN_SPI_MISO) && PIN_SPI_MISO == 10

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

--

-- Best Regards Andrea Richetta

R&D - Product Business Unit Manager

cmaglie commented 6 years ago

Should I make pin 5 the default for CS when any of the MKR boards are selected?

Yes

How should I detect this case? So far I'm considering something like this:

Good question, ATM there isn't a define that identifies the "MKR" format, we should add them to the variants. For the moment I'd go with:

#if defined(ARDUINO_SAMD_MKRZERO) || defined(ARDUINO_SAMD_MKR1000) || defined(ARDUINO_SAMD_MKRFox1200) || defined(ARDUINO_SAMD_MKRGSM1400) || defined(ARDUINO_SAMD_MKRWAN1300)

Once we add the define for the MKR format a define like:

#if defined(USE_ARDUINO_MKR_PIN_LAYOUT)

should do the trick, but I'll keep the longer one anyway to maximize backward compatibility.

BTW, this make me think that we should add an explicit define inside the variant.h for the Ethernet library, like:

#define ETHERNET_LIB_CS_PIN 5

so in the ethernet library we can do:

#if defined(ETHERNET_LIB_CS_PIN)
  ...use ETHERNET_LIB_CS_PIN as default CS...
#else
  ...big ifdef chain with the boards census...
#endif

This will allow all future board to add their default without touching the Ethernet library anymore. What do you think?

PaulStoffregen commented 6 years ago

I really like the idea that variant.h can define ETHERNET_LIB_CS_PIN. Seems like a much cleaner long-term solution. Perhaps PIN_SPI_SS_ETHERNET_LIB might be a little more consistent with the names already used in variant.h on SAMD & AVR?

cmaglie commented 6 years ago

Ok, either way is fine for me, let's go with PIN_SPI_SS_ETHERNET_LIB then.

PaulStoffregen commented 6 years ago

I've brought in linkStatus and the network settings functions. I do have a concern about how functions to set network parameters (eg, setLocalIP, setSubnetMask) are meant to work if used when the library is already running with sockets open, especially if DHCP is in use. Any thoughts on this?

I have a short list of issues to investigate. Looked at one just now when turned out to be user error. My hope is to start testing with all the various hardware (I do have the MKR ETH running now, after some initial trouble with a long cable) and hopefully wrap up a release by Sunday.

cmaglie commented 6 years ago

I've brought in linkStatus and the network settings functions. I do have a concern about how functions to set network parameters (eg, setLocalIP, setSubnetMask) are meant to work if used when the library is already running with sockets open, especially if DHCP is in use. Any thoughts on this?

They were meant to be used when running without DHCP, right now the only way to change IP is using begin. I didn't thought on what happens on opened sockets when the IP address is changed, my expectation is that they will be kept alive with the old IP until closed, but it seems ok if they became "stale" too. Anyway I see that these functions have very little use cases, but seemed weird to have functions to query the current IPs but not to set them.

I have a short list of issues to investigate. Looked at one just now when turned out to be user error. My hope is to start testing with all the various hardware (I do have the MKR ETH running now, after some initial trouble with a long cable) and hopefully wrap up a release by Sunday.

Ok thanks. I'll be out from this Saturday to Jul 10th, no need to rush a release for Sunday.

PaulStoffregen commented 6 years ago

I'm going to slip a few days on this. My PC died yesterday morning, after 6 very productive years. New motherboard, CPU, memory arrives tomorrow. Confirmed my last daily backup is good, so should be back up & running soon. Writing this from a Mac I usually used only for video editing.

PaulStoffregen commented 6 years ago

Still working on Ethernet, as you can see on my repository. This is yet another case of "everything takes longer than anticipated".

I have a few more issues people have reported on forums. Most look like "user error" or bad hardware, but still I'd like to investigate a few more before releasing to all Arduino users.

If you need this sooner, just ping me and I can wrap this up.

PaulStoffregen commented 6 years ago

I've brought in the 6 functions from @per1234 EthernetMod. https://github.com/per1234/EthernetMod

These are really useful functions. @cmaglie - hopefully making these part of the 2.0 API is ok?

Rather than have users include w5100.h (which doesn't do SPI transactions), I put setRetransmissionTimeout() and setRetransmissionCount() into the main Ethernet class.

cmaglie commented 6 years ago

These are really useful functions. @cmaglie - hopefully making these part of the 2.0 API is ok?

Yes

sandeepmistry commented 6 years ago

New API's look good. One small suggestion is to consider renaming setClientTimeout, maybe to setConnectionTimeout?

PaulStoffregen commented 6 years ago

One small suggestion is to consider renaming setClientTimeout, maybe to setConnectionTimeout?

Done. https://github.com/PaulStoffregen/Ethernet/commit/ff8a1431a9db70abe7618a271c70878c5ad0a118

PaulStoffregen commented 6 years ago

Ok, I've imported my fork as one giant commit. https://github.com/arduino-libraries/Ethernet/commit/ac843291454f522cdef6bddbacce2c52028be943

My plan is to retest on every board (at least the ones I have - covers most Arduino boards) over the next few days. Would like to wrap all this up by next week and tag 2.0.0.

If anyone has concerns with any of this new stuff, especially the new APIs, now's the time to say something!

PaulStoffregen commented 6 years ago

@cmaglie - How's the new API look? I added Ethernet.hardwareStatus(), similar to linkStatus() but tells which hardware was detected. How about the enums, like making "Unknown" a global scope constant?

After retesting WebClient, WebServer and UdpNtpClient on all boards, I'm ready to call this 2.0.0.

cmaglie commented 6 years ago

API looks good. After a quick check everything seems to be working OK. I have no other concerns, so let's release it :tada: :tada: :tada:

(I guess you deserve the honor to create the new release tag :-))

PaulStoffregen commented 6 years ago

I'm writing a blog with a summary of all the changes. Will tag it late today or early tomorrow.

PaulStoffregen commented 6 years ago

Also want to retest with all the hardware. Just made a tweak to TCP settings. I'm kinda paranoid like that.

Rotzbua commented 6 years ago

After a quick look I saw some (optional?) optimization potential. E.g.

int EthernetClass::socketStartUDP(uint8_t s, uint8_t* addr, uint16_t port);
int EthernetClass::socketSendUDP(uint8_t s);

to

uint8_t EthernetClass::socketStartUDP(uint8_t s, uint8_t* addr, uint16_t port);
uint8_t EthernetClass::socketSendUDP(uint8_t s);

@PaulStoffregen I can offer a small review this weekend if you want?

@cmaglie Is c++11 still not allowed or can we use type hint for enums? eg. enum color : byte { red, blue }

PaulStoffregen commented 6 years ago

I saw some (optional?) optimization potential.

Done. https://github.com/arduino-libraries/Ethernet/commit/6e9dffa64f6b0eb89607dbb5293dc33be82cc39e Saves 4 bytes code space on AVR boards.

PaulStoffregen commented 6 years ago

It's time to cut off any more non-bugfix changes.

I'm starting a massive test with 15 boards & 9 shields in 26 board+shield combinations with 4 sketches each. If all 104 tests pass, really not looking to make any more edits.

I can offer a small review this weekend if you want?

My hope is to release sometime tomorrow. That'll be timely for a review, but if there's any bugs to discover, now is the time to test.

PaulStoffregen commented 6 years ago

Testing on Arduino Zero is showing there isn't enough timing margin for 12 MHz SPI when used with either of the W5100-based shields. Here's the hardware detection as seen by my oscilloscope.

file

This sequence writes 0x10 to the MR register, then tries to read it back. Even though the scope sees 0x10 (where I circled in yellow), when running at this 12 MHz SPI clock Arduino Zero actually receives 0x08. Arduino Zero does receive 0x10 and everything works fine if I reduce the SPI clock to 8 MHz.

However, the W5200 & W5500 chips do seem to work with 12 MHz SPI. They must have more timing margin.

I'm a bit sad to have to hobble all the SAMD boards with only 8 MHz SPI clock speed, but I just don't see any other way to support the many existing shields on boards like Zero and M0 Pro. :(

PaulStoffregen commented 6 years ago

Here's a zoom in to the erroneously received byte. MISO (the blue trace) is transitioning well before the rising edge of SCK. Sadly, it seems the SAMD chip just doesn't quite have the timing margin to receive this.

file2

PaulStoffregen commented 6 years ago

Whew, finally finished testing all the boards & shields. Here's the results.

Board                   Shield                  WebClient       WebClient       UdpNtpClient    WebServer
                                                (local)         (google)

Arduino Uno R3          Arduino Ethernet R2     82.66           79.27           ok              ok
                        Arduino Ethernet R3     82.66           79.11           ok              ok
                        WIZ820io                331.27          191.85          ok              ok
                        Arduino.org Ethernet2   329.60          195.32          ok              ok
                        Seeed Ethernet W5500    329.00          185.44          ok              ok

Arduino Leonardo        Arduino Ethernet R2     82.28           78.75           ok              ok
                        WIZ820io                330.30          183.69          ok              ok
                        Seeed Ethernet W5500    328.14          179.98          ok              ok

Arduino Uno Wifi Rev2   Arduino Ethernet R2     72.30           69.55           ok              ok
                        Arduino Ethernet R3     72.23           69.50           ok              ok
                        WIZ820io                212.19          161.94          ok              ok
                        Arduino.org Ethernet2   212.88          169.72          ok              ok (linkstatus wrong)
                        Seeed Ethernet W5500    213.36          163.86          ok              ok (linkstatus wrong)

Arduino Mega 2560       Arduino Ethernet R2     77.44           74.31           ok              ok
                        WIZ820io                325.44          172.73          ok              ok
                        Seeed Ethernet W5500    323.36          179.58          ok              ok

Arduino Zero            Arduino Ethernet R2     96.64           91.42           ok              ok
                        Arduino Ethernet R3     96.64           91.33           ok              ok
                        WIZ820io                298.53          177.53          ok              ok
                        Arduino.org Ethernet2   305.28          181.60          ok              ok
                        Seeed Ethernet W5500    305.26          183.13          ok              ok

Arduino Due             Arduino Ethernet R3     109.73          105.98          ok              ok
                        WIZ820io                670.88          206.51          ok              ok
                        Seeed Ethernet W5500    689.69          214.44          ok              ok

Arduino 101 (Intel)     Arduino Ethernet R3     43.60           42.39           ok              ok
                        WIZ820io                349.35          169.37          ok              ok
                        Seeed Ethernet W5500    359.32          168.96          ok              ok

Arduino MKR1000         MRK ETH                 298.93          181.27          ok              ok
                        WIZ820io                291.98          125.20          ok              ok

Teensy 3.6              WIZ850io                1143.58         212.59          ok              ok
                        WIZ820io                1102.71         202.44          ok              ok
                        WIZ812MJ                274.14          180.76          ok              ok

Teensy 3.2              WIZ850io                958.06          205.37          ok              ok
                        WIZ820io                914.78          215.44          ok              ok
                        WIZ812MJ                234.55          170.07          ok              ok

Teensy LC               WIZ850io                479.73          200.51          ok              ok
                        WIZ820io                471.95          199.62          ok              ok
                        WIZ812MJ                137.77          126.40          ok              ok

Teensy 2.0              WIZ812MJ                84.85           81.07           ok              ok

ChipKit Uno32           Arduino Ethernet R2     272.18          159.72          ok              ok
                        WIZ820io                837.56          188.31          ok              ok
                        Seeed Ethernet W5500    858.81          177.19          ok              ok

Adafruit ESP8266        FeatherWing Ethernet    583.31          fail (dns)      fail (dns)      ok

Adafruit ESP32          FeatherWing Ethernet    965.76          211.06          ok              ok

Possible issues:
Adafruit ESP32 fails WebClient (google) and UdpNtpClient tests, due to DNS
Arduino Uno R3 with WIZ820io running WebClient sometimes fails to connect to google
Arduino Due with WIZ820io running WebClient sometimes fails to connect to google
Arduino Uno Wifi Rev2 with WIZ820io running WebClient sometimes fails to connect to google
Arduino Mega 2560 with WIZ820io running WebClient sometimes fails to connect to google
PaulStoffregen commented 6 years ago

Tagged 2.0.0. :-)

https://github.com/arduino-libraries/Ethernet/releases/tag/2.0.0

Will close this and other issues soon....

PaulStoffregen commented 6 years ago

Finished a blog article about the changes. https://www.pjrc.com/arduino-ethernet-library-2-0-0/

PaulStoffregen commented 6 years ago

ok, closing this issue now that all chips are supported