driver/w5500: driver for the W5500 ethernet chip #20301

stemschmidt · 2024-01-26T17:11:39Z

Contribution description

This is a driver for a WIZ5500 ethernet chip. The driver supports interrupt and polling mode. Polling mode is the default, as plugging the Adafruit Ethernet FeatherWing on the Adafruit nrf52840 Feather requires no additional wiring for the interrupt pin.

Provide a definition for W5500_PARAM_EVT to enable the interrupt mode.

Testing procedure

Build:
make BOARD=feather-nrf52840 -C tests/drivers/w5500/ flash term

Ping ipv6.google.com:
2024-01-26 18:08:37,885 # : This is RIOT! (Version: a74a7-driver_w5500)
2024-01-26 18:08:37,887 # Test application for W5500 ethernet device driver
2024-01-26 18:08:37,889 # All up, running the shell now

ping 2a00:1450:4016:808::200e
2024-01-26 18:08:58,580 # ping 2a00:1450:4016:808::200e
2024-01-26 18:08:58,775 # 12 bytes from 2a00:1450:4016:808::200e: icmp_seq=0 ttl=118 time=192.157 ms
2024-01-26 18:08:59,675 # 12 bytes from 2a00:1450:4016:808::200e: icmp_seq=1 ttl=118 time=92.177 ms
2024-01-26 18:09:00,675 # 12 bytes from 2a00:1450:4016:808::200e: icmp_seq=2 ttl=118 time=92.203 ms
2024-01-26 18:09:00,676 #
2024-01-26 18:09:00,678 # --- 2a00:1450:4016:808::200e PING statistics ---
2024-01-26 18:09:00,680 # 3 packets transmitted, 3 packets received, 0% packet loss
2024-01-26 18:09:00,682 # round-trip min/avg/max = 92.177/125.512/192.157 ms

riot-ci · 2024-01-26T20:35:35Z

Murdock results

✔️ PASSED

9d62ad4 driver/w5500: driver for the W5500 ethernet chip

Success	Failures	Total	Runtime
10082	0	10083	13m:14s

Artifacts

Documentation preview

benpicco

Thank you for the driver!
This looks quite good already, just some comments:

benpicco · 2024-01-26T20:24:18Z

drivers/include/w5500.h

+ */
+enum {
+    W5500_ERR_BUS = -1,
+};


Why do you need this?

This is returned if the device cannot be accessed via I2C bus

I mean why a custom error code instead something from errno.h

Ok, I switch to -ENODEV

benpicco · 2024-01-26T20:24:51Z

drivers/include/w5500.h

+    netdev_t netdev;        /**< extends the netdev structure */
+    w5500_params_t p;       /**< device configuration parameters */
+    uint16_t frame_size_to_be_send;
+    uint16_t frame_size_sent;


Why would they differ?

My understanding of confirm_send is that this function can be called at any time to get the number of transmitted bytes of the current frame:

called before a new transmission is started (frame_size_to_be_send = 0): return the transferred size of the previous frame (frame_size_sent)

called after the new transmission has been started and is still in progress (frame_size_sent = 0): return -EAGAIN

called after the new transmission terminated -> see 1.

I think the intention here was that this would only be called after the transmission and that the beginning of a new transmission would invalidate it (returns -EAGAIN).

So there is no need to store the size of the previous transmission.

But @maribu should know best.

As @benpicco said:

Call send(), it returns right away, the hardware transmits in background

Wait for transmission to complete

Call confirm_send() exactly once to get the number of transmitted bytes or the error code

Note: The is a provision for trivial polling:

send(); while ((result = confirm_send) == -EAGAIN) { /* polling for TX to complete */ } /* transmission result is in result */

Ok, I understand. I thought you might be able to poll the current number of transmitted bytes of the frame while the transmission is still ongoing (the hardware might transmit the frame in chunks, so you could get the number of bytes already transmitted)...
I will change the implementation.

drivers/w5500/w5500.c

benpicco · 2024-01-26T20:53:37Z

drivers/w5500/w5500.c

+
+    uint8_t tmp = rreg(dev, REG_PHYCFGR);
+
+    if (tmp & PHY_LINK_UP) {


What do you need dev->link_up for then? It's never read.

It is read in isr() in order to send NETDEV_EVENT_LINK_UP and NETDEV_EVENT_LINK_DOWN only when the status has change

Ah so a change in link status does not cause an interrupt on it's own?

I could not find a way to trigger an interrupt when the PHY state changes. So I poll it in several places.

drivers/w5500/w5500.c

benpicco · 2024-01-29T21:11:33Z

drivers/w5500/w5500.c

+    /* Polling mode: W5500_PARAM_EVT == GPIO_UNDEF and polling_interval_ms > 0u    */
+    /* Interrupt mode: W5500_PARAM_EVT != GPIO_UNDEF and polling_interval_ms == 0u */
+    return((dev->p.evt == GPIO_UNDEF && dev->p.polling_interval_ms > 0u) ||
+           (dev->p.evt != GPIO_UNDEF && dev->p.polling_interval_ms == 0u));
+}


I'd be a bit more graceful here: You could have a valid polling interval always configured by default, but when a IRQ pin is provided use that instead of polling (and instead of producing an unnecessary error on init).

I can't think of a use case where you would want polling if you have an IRQ pin.

Ok. My intention was to point the user to the invalid configuration, but I have softened the edge now :-) And removed the function.

benpicco · 2024-01-29T21:12:57Z

drivers/w5500/w5500.c

+    tmp = read_register(dev, REG_VERSIONR);
+    if (tmp != CHIP_VERSION) {
+        spi_release(dev->p.spi);
+        LOG_ERROR("[w5500] error: no SPI connection\n");


Suggested change

LOG_ERROR("[w5500] error: no SPI connection\n");

LOG_ERROR("[w5500] error: invalid chip ID %x\n", tmp);

maribu

Thanks for the PR :)

First round of comments

sys/net/gnrc/netif/init_devs/auto_init_w5500.c

drivers/w5500/w5500.c

benpicco

Sorry for the delay, some more nits:

drivers/w5500/include/w5500_params.h

drivers/w5500/w5500.c

benpicco · 2024-02-12T19:41:22Z

drivers/include/w5500.h

+    spi_clk_t clk;                /**< clock speed used on the selected SPI bus */
+    gpio_t cs;                    /**< pin connected to the chip select line */
+    gpio_t evt;                   /**< pin connected to the INT line */
+    uint32_t polling_interval_ms; /**< interval for polling, 0 if interrupt mode */


Have you considered doing a compile-time check, so we don't even need to compile the polling code in if we have a IRQ pin connected?

Suggested change

uint32_t polling_interval_ms; /**< interval for polling, 0 if interrupt mode */

#if W5500_PARAM_EVT == GPIO_UNDEF

uint32_t polling_interval_ms; /**< interval for polling, 0 if interrupt mode */

#endif

The w5500.h header is independent of the configuration, it does not include w5500_params.h. So I do not have the information W5500_PARAM_IRQ == GPIO_UNDEF available in the header

benpicco

Sorry for the delay, I think this driver is fine if it works for you.

Just some small nits, please squash directly.

tests/drivers/w5500/Makefile

drivers/w5500/w5500.c

benpicco · 2024-04-24T21:23:25Z

The changes to the example Makefile still apply 😉
Why not just use the example app from tests/drivers/w5100 and change w5100 -> w5500?

stemschmidt · 2024-04-27T14:44:55Z

Hi @benpicco, could you please help me with the failing release-tests? Where do I find information about the "Invalid access token passed." message?

maribu · 2024-04-27T14:59:35Z

The issue is that the new test app fails to link on some boards. The CI stopped building on the first instance, which was the ATmega8.

This is expected, as some MCUs (like the ATmega8) just have too little RAM and/or ROM to allow running the app. We manually keep a list of boards that are expected to fail linking in the Makefile.ci. You can run make generate-Makefile.ci in the test folder to update/generate that file.

Note: You might need to run docker pull riot/riotbuild first to update the docker image with the versions used in the CI, as different compiler versions optimize the code differently, resulting in some apps that barely fit with one to overflow with the other.

maribu · 2024-04-27T15:02:57Z

You can directly squash the update of the Makefile.ci into the commit that adds the app.

Note: I can also run the script tonight. Building an app 200+ times burns a few CPU cycles (we have a lot of boards supported these days 😎)

maribu · 2024-04-28T07:38:27Z

Please tick the checkbox that allow maintainers to edit this PR, otherwise I'm unable to push the Makefile.ci update.

maribu · 2024-04-28T08:52:29Z

Or alternatively, store this as Makefile.ci in the test app:

BOARD_INSUFFICIENT_MEMORY := \
    arduino-duemilanove \
    arduino-leonardo \
    arduino-nano \
    arduino-uno \
    atmega328p \
    atmega328p-xplained-mini \
    atmega8 \
    nucleo-f031k6 \
    nucleo-l011k4 \
    samd10-xmini \
    stk3200 \
    stm32f030f4-demo \
    #

Please also note: The commit message is syntactically incorrect. There is missing a mandatory blank line between the header and the body of the commit message.

stemschmidt · 2024-04-30T11:44:29Z

Please tick the checkbox that allow maintainers to edit this PR, otherwise I'm unable to push the Makefile.ci update.

Mmmh, I can't see the checkbox...

maribu · 2024-04-30T12:05:03Z

I think you are looking at the right spot in the page. It looks like this for me:

Are you logged in?

In either case, just storing the content from two messages above in a file named Makefile.ci in the test app would get this past the CI as well ;)

Please don't forget to add a blank line after the header (the first line) in the commit message when you --amend the commit.

stemschmidt · 2024-04-30T12:08:15Z

I will update the makefile.in later or tomorrow...

stemschmidt · 2024-04-30T21:11:58Z

Ok, I added the Makefile.in and modified the commit message... Now fingers crossed :-)

stemschmidt · 2024-05-01T09:28:02Z

Mmmh, why is it still being build for the atmega8? And also for other cpus/boards in the insufficient-memory list?

maribu

The list of boards to skip the linking step for should go into Makefile.ci, not Makefile.in. Sorry for the confusion.

maribu · 2024-05-01T10:56:29Z

tests/drivers/w5500/Makefile.ci

+BOARD_INSUFFICIENT_MEMORY := \
+    arduino-duemilanove \
+    arduino-leonardo \
+    arduino-mega2560 \
+    arduino-nano \
+    arduino-uno \
+    atmega328p \
+    atmega328p-xplained-mini \
+    bluepill-stm32f030c8 \
+    i-nucleo-lrwan1 \
+    msb-430 \
+    msb-430h \
+    nucleo-f030r8 \
+    nucleo-f031k6 \
+    nucleo-f042k6 \
+    nucleo-f303k8 \
+    nucleo-f334r8 \
+    nucleo-l011k4 \
+    nucleo-l031k6 \
+    nucleo-l053r8 \
+    samd10-xmini \
+    slstk3400a \
+    stk3200 \
+    stm32f030f4-demo \
+    stm32f0discovery \
+    stm32l0538-disco \
+    waspmote-pro \
+    #


Suggested change

BOARD_INSUFFICIENT_MEMORY := \

arduino-duemilanove \

arduino-leonardo \

arduino-mega2560 \

arduino-nano \

arduino-uno \

atmega328p \

atmega328p-xplained-mini \

bluepill-stm32f030c8 \

i-nucleo-lrwan1 \

msb-430 \

msb-430h \

nucleo-f030r8 \

nucleo-f031k6 \

nucleo-f042k6 \

nucleo-f303k8 \

nucleo-f334r8 \

nucleo-l011k4 \

nucleo-l031k6 \

nucleo-l053r8 \

samd10-xmini \

slstk3400a \

stk3200 \

stm32f030f4-demo \

stm32f0discovery \

stm32l0538-disco \

waspmote-pro \

#

BOARD_INSUFFICIENT_MEMORY := \

arduino-duemilanove \

arduino-leonardo \

arduino-nano \

arduino-uno \

atmega328p \

atmega328p-xplained-mini \

atmega8 \

nucleo-f031k6 \

nucleo-l011k4 \

samd10-xmini \

stk3200 \

stm32f030f4-demo \

#

maribu · 2024-05-01T10:56:43Z

tests/drivers/w5500/Makefile.in

@@ -0,0 +1,14 @@
+BOARD_INSUFFICIENT_MEMORY := \


This file should be removed

ok, I will do it later today...

Ok, I have modified the Makefile.ci and removed the Makefile.in (I have added the missing boards to the existing Makefile.ci)... If I had used my brain I could have come up with the idea that the Makefile.ci is the relevant file :-)

- driver can be used with interrupt or in polling mode (default)

maribu · 2024-05-03T09:34:29Z

It finally is in 🎉 Sometimes it really is a bit of a pain to get a PR past the CI 😦

Thanks a lot for your pull request!

stemschmidt · 2024-05-04T13:04:06Z

Yeah :-)

I am impressed by the RIOT CI chain, I think it is a great (or maybe the best) way to define what is expected from a piece of software before it can be merged into main.

I think it is a bit difficult to keep track of what is missing and how things are handled if you are not working 100% of your time on the Repository, but finally it is in the main!!!

stemschmidt requested review from leandrolanzieri, jia200x, MrKevinWeiss, aabadie, MichelRottleuthner, miri64 and PeterKietzmann as code owners January 26, 2024 17:11

github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: sys Area: System Area: Kconfig Area: Kconfig integration labels Jan 26, 2024

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 26, 2024

benpicco reviewed Jan 26, 2024

View reviewed changes

stemschmidt force-pushed the driver_w5500 branch 2 times, most recently from b423975 to 63d5c41 Compare January 28, 2024 17:05

benpicco reviewed Jan 29, 2024

View reviewed changes

maribu mentioned this pull request Jan 30, 2024

drivers/periph_spi: Add spi_transfer_u16_be() #20312

Merged

1 task

maribu reviewed Jan 30, 2024

View reviewed changes

sys/net/gnrc/netif/init_devs/auto_init_w5500.c Outdated Show resolved Hide resolved

drivers/w5500/w5500.c Outdated Show resolved Hide resolved

drivers/w5500/w5500.c Outdated Show resolved Hide resolved

maribu mentioned this pull request Jan 30, 2024

sys/libc: add endian.h #20310

Merged

benpicco reviewed Feb 12, 2024

View reviewed changes

stemschmidt force-pushed the driver_w5500 branch from 66f961e to a109aec Compare March 17, 2024 11:09

benpicco reviewed Mar 26, 2024

View reviewed changes

stemschmidt force-pushed the driver_w5500 branch 3 times, most recently from 8801a71 to 2eaebc4 Compare April 24, 2024 20:37

stemschmidt force-pushed the driver_w5500 branch from 2eaebc4 to 6924b62 Compare April 25, 2024 16:37

benpicco added this pull request to the merge queue Apr 26, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 26, 2024

stemschmidt force-pushed the driver_w5500 branch 2 times, most recently from a126323 to 87612aa Compare April 30, 2024 21:11

maribu enabled auto-merge May 1, 2024 05:55

maribu added this pull request to the merge queue May 1, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 1, 2024

maribu reviewed May 1, 2024

View reviewed changes

driver/w5500: driver for the W5500 ethernet chip

9d62ad4

- driver can be used with interrupt or in polling mode (default)

stemschmidt force-pushed the driver_w5500 branch from 87612aa to 9d62ad4 Compare May 2, 2024 20:07

maribu enabled auto-merge May 2, 2024 20:53

maribu disabled auto-merge May 3, 2024 06:12

maribu enabled auto-merge May 3, 2024 06:12

maribu added this pull request to the merge queue May 3, 2024

Merged via the queue into RIOT-OS:master with commit e244918 May 3, 2024
26 checks passed

stemschmidt deleted the driver_w5500 branch May 3, 2024 09:12

mguetschow added this to the Release 2024.07 milestone Jul 5, 2024


		uint8_t tmp = rreg(dev, REG_PHYCFGR);

		if (tmp & PHY_LINK_UP) {

	LOG_ERROR("[w5500] error: no SPI connection\n");
	LOG_ERROR("[w5500] error: invalid chip ID %x\n", tmp);

driver/w5500: driver for the W5500 ethernet chip #20301

driver/w5500: driver for the W5500 ethernet chip #20301

Conversation

stemschmidt commented Jan 26, 2024

Contribution description

Testing procedure

riot-ci commented Jan 26, 2024 • edited Loading

Murdock results

Artifacts

benpicco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stemschmidt Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stemschmidt Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stemschmidt Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

stemschmidt Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu left a comment

Choose a reason for hiding this comment

benpicco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco left a comment

Choose a reason for hiding this comment

benpicco commented Apr 24, 2024 • edited Loading

stemschmidt commented Apr 27, 2024

maribu commented Apr 27, 2024

maribu commented Apr 27, 2024

maribu commented Apr 28, 2024

maribu commented Apr 28, 2024

stemschmidt commented Apr 30, 2024

maribu commented Apr 30, 2024

stemschmidt commented Apr 30, 2024

stemschmidt commented Apr 30, 2024

stemschmidt commented May 1, 2024

maribu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stemschmidt May 2, 2024 • edited Loading

Choose a reason for hiding this comment

maribu commented May 3, 2024

stemschmidt commented May 4, 2024

riot-ci commented Jan 26, 2024 •

edited

Loading

stemschmidt Jan 27, 2024 •

edited

Loading

stemschmidt Feb 2, 2024 •

edited

Loading

stemschmidt Jan 27, 2024 •

edited

Loading

benpicco Jan 29, 2024 •

edited

Loading

stemschmidt Feb 2, 2024 •

edited

Loading

benpicco commented Apr 24, 2024 •

edited

Loading

stemschmidt May 2, 2024 •

edited

Loading