drivers/stm32_eth: add 'NETDEV_EVENT_LINK_UP' event #14688

JannesVolkens · 2020-08-03T13:13:00Z

Contribution description

This PR offers the possibility to check the link status of stm32 devices to issue the NETDEV_EVENT_LINK_UP event. To achieve this the link status is periodically checked with the initialization of the netdev and as soon as a link is detected the event NETDEV_EVENT_LINK_UP is triggered.

Testing procedure

Try running the test under /tests/lwip/ for IPv4 and typing in ifconfig with an ethernet cable connected. The device should have a valid IPv4 address.

This PR was testing using a Nucleo-F207ZG board.

Issues/PRs references

#14150

benpicco · 2020-08-05T07:46:47Z

This periodic polling seems odd.
Is there no way to generate an interrupt on link with the stm32 Ethernet peripheral?

jia200x · 2020-08-05T08:27:43Z

This periodic polling seems odd.
Is there no way to generate an interrupt on link with the stm32 Ethernet peripheral?

It seems there's no way to do it :(.
An alternative would be to use something like the ifconfig up command (same as GNRC LoRaWAN).
If the Link is not up, the interface won't be up.

How does it sound?

benpicco · 2020-08-05T09:06:06Z

Seems like there is only a sense pin that can generate an interrupt if you plug the cable, I guess that's not what you want.

But looks like Zephyr does this too (zephyrproject-rtos/zephyr#19163)

PeterKietzmann

Seems like there is only a sense pin that can generate an interrupt if you plug the cable, I guess that's not what you want.

But looks like Zephyr does this too (zephyrproject-rtos/zephyr#19163)

@benpicco I don't understand what you're saying. In zephyr, they have a polling based checkup routing (in the driver!) as well. I don't like this solution either, but there is just no interrupt pin connected. One thing that I'm wondering is whether we should provide such feature within the driver and not just inside the lwip adaptation.

drivers/stm32_eth/stm32_eth.c

pkg/lwip/contrib/netdev/lwip_netdev.c

maribu · 2020-08-07T08:38:12Z

One thing that I'm wondering is whether we should provide such feature within the river and not just inside the lwip adaptation.

+1 for having this in the driver. Ideally as (pseudo)module, as not every use might be willing to pay the overhead for this feature.

maribu · 2020-08-07T08:48:54Z

Btw: The link status timer could be reset in the driver after processing an RX event; the link must be if frames come in. That way the link polling won't add much additional load if the link is very busy.

benpicco · 2020-08-07T08:53:14Z

@benpicco I don't understand what you're saying. In zephyr, they have a polling based checkup routing (in the driver!) as well.

That's exactly what I'm saying 😉
I wanted to see how the problem was solved elsewhere, only to find that they came to the very same solution.
So just confirming that this is indeed the way to go ¯\(ツ)/¯

PeterKietzmann · 2020-08-07T09:06:13Z

That's exactly what I'm saying

Ah I see (:

@benpicco, @jia200x what is you opinion on implementing the indication (optionally) in the driver?

miri64 · 2020-08-07T09:12:12Z

Like @maribu I would prefer an implementation of this in the driver as optional pseudo-module. At one point or another we will need this in other network stacks.

benpicco · 2020-08-07T09:12:45Z

Can stm32_eth_phy_read(0, PHY_BSMR) be called from interrupt context?
Then a simple periodic timer in the driver would indeed be the easiest solution.

jia200x · 2020-08-07T09:16:17Z

@benpicco, @jia200x what is you opinion on implementing the indication (optionally) in the driver?

+1

Implementation-wise, if possible I would prefer a periph_timer based solution rather than pulling a high level API there. I don't know how is it with this devices, but several IEEE 802.15.4 radios provide a MAC timer that can be used for such tasks.

That way we don't have to pull the xtimer/ztimer every time we use the driver.

jia200x · 2020-08-07T09:31:43Z

Actually #14391 would be ideal IMO

JannesVolkens · 2020-08-12T12:52:56Z

I have now moved the implementation into the driver.

I tried to solve it with a periph_timer, as @jia200x recommended, but ran into some problems, so I'm using xtimer now after all.
For reasons unknown to me, calling timer_stop caused no valid address to be assigned even though dhcp_start was executed successfully. Calling timer_stop was necessary in this case because timer_set had started the timer periodically in my case and not just once. So without calling timer_stop, the timer was still periodically executed in the background.

Unfortunately I am not very familiar with the timers, so I have not found the source of the error yet.

makefiles/pseudomodules.inc.mk

tests/lwip/Makefile

drivers/stm32_eth/stm32_eth.c

benpicco · 2020-08-12T13:23:02Z

For reasons unknown to me, calling timer_stop caused no valid address to be assigned even though dhcp_start was executed successfully.

You have to use a different hardware timer than then one used by xtimer, otherwise you will meddle with that.
Most MCUs have plenty of hardware timers, unfortunately most board configurations in RIOT will only provide one (to be used by xtimer)

drivers/stm32_eth/stm32_eth.c

JannesVolkens · 2020-08-12T13:52:34Z

@miri64 @benpicco I have applied your requested changes.

drivers/stm32_eth/stm32_eth.c

JannesVolkens · 2020-08-27T08:24:22Z

@benpicco I removed the old file and moved everything to cpu/stm32/periph/eth.c.

aabadie · 2020-08-27T08:32:19Z

makefiles/pseudomodules.inc.mk

@@ -74,6 +74,7 @@ PSEUDOMODULES += mpu_noexec_ram
 PSEUDOMODULES += nanocoap_%
 PSEUDOMODULES += netdev_default
 PSEUDOMODULES += netdev_ieee802154_%
+PSEUDOMODULES += netdev_link_up


Should this be removed ?

It is now removed.

benpicco · 2020-08-27T08:32:53Z

cpu/stm32/periph/eth.c

+    netdev_t *dev = (netdev_t *)arg;
+    if (stm32_eth_get(dev, NETOPT_LINK, NULL, 0)) {
+        _link_status = 1;
+        dev->event_callback(dev, NETDEV_EVENT_ISR);


If this causes stm32_eth_isr to be called, it will also generate a NETDEV_EVENT_RX_COMPLETE event.

I’m not quite sure how to handle this right now. In the old eth.c implementation there was this function stm32_eth_get_rx_status_owned, which was used to check whether a frame reception was completed or not.
What is the best way to check for that in the current implementation?

benpicco · 2020-08-27T08:33:31Z

cpu/stm32/periph/eth.c

@@ -437,6 +511,11 @@ void stm32_eth_isr_eth_wkup(void)

 static void stm32_eth_isr(netdev_t *netdev) {
    netdev->event_callback(netdev, NETDEV_EVENT_RX_COMPLETE);


Now this is wrong.

I have added the function stm32_eth_get_rx_status_owned again to be able to distinguish between the link up and a message reception.

PeterKietzmann

The pseudomodule stm32_eth_link_up should include xtimer as a dependency somewhere here.

I've tested this PR in two ways:

1.

printfon NETDEV_EVENT_RX_COMPLETE and NETDEV_EVENT_LINK_UP.
examples/default on one nucleo-f207zg.
Connect ethernet cable to an other nucleo-f207zg.
NETDEV_EVENT_LINK_UP is reported once.

2.

printfon NETDEV_EVENT_RX_COMPLETE and NETDEV_EVENT_LINK_UP.
tests/lwip compiled with LWIP_IPV4=1 on one nucleo-f207zg.
Connect ethernet cable to local network (DHCP server running).
NETDEV_EVENT_LINK_UP is reported once and various packets (that are not filtered by hardware) are received.
Board is assigned an IPv4 address.

PeterKietzmann · 2020-08-27T15:59:41Z

cpu/stm32/periph/eth.c

+    if (stm32_eth_get_rx_status_owned()) {
+        netdev->event_callback(netdev, NETDEV_EVENT_RX_COMPLETE);
+    }
+    if (_link_status && _first_call) {


Please address this comment

benpicco · 2020-08-27T16:41:30Z

cpu/stm32/periph/eth.c

@@ -435,8 +512,19 @@ void stm32_eth_isr_eth_wkup(void)
    cortexm_isr_end();
 }

+int stm32_eth_get_rx_status_owned(void)


should this be static?

This function is not needed (see comment below). Please drop it again.

maribu

See inline comments

maribu · 2020-08-28T19:11:00Z

cpu/stm32/periph/eth.c

@@ -435,8 +512,19 @@ void stm32_eth_isr_eth_wkup(void)
    cortexm_isr_end();
 }

+int stm32_eth_get_rx_status_owned(void)


This function is not needed (see comment below). Please drop it again.

maribu · 2020-08-28T19:15:42Z

cpu/stm32/periph/eth.c

+/* Internal flags for the DMA descriptors */
+#define DESC_OWN           (0x80000000)
+


Please drop this and use the existing macros

maribu · 2020-08-28T19:19:49Z

cpu/stm32/periph/eth.c

+static uint8_t _link_status = 0;
+static uint8_t _first_call = 1;


No need to have two variables for this. It is a simple state machine with theses states:

+------------------+ 1 +---------------+ 2 +-------------+ | Link down (init) | --> | Link up | --> | Notified Up | +------------------+ +---------------+ +-------------+ ^ | | 1 | 3 | v +---------------+ 4 +-------------+ | Notified Down | <-- | Link Down | +---------------+ +-------------+

And these events triggering the transitions:

Link goes up

Upper layer gets informed that link is up

link goes down

Upper layer gets informed that link is down

Tracking the current state fits nicely into a single byte.

No need to distinguish between the initial link down state and the notified down state

maribu · 2020-08-28T19:30:34Z

cpu/stm32/periph/eth.c

+    if (stm32_eth_get_rx_status_owned()) {
+        netdev->event_callback(netdev, NETDEV_EVENT_RX_COMPLETE);
+    }
+    if (_link_status && _first_call) {
+        netdev->event_callback(netdev, NETDEV_EVENT_LINK_UP);
+        _first_call = 0;
+    }


No need to call stm32_eth_get_rx_status_owned() here. If there is no notification for link state changes pending, the ISR can only be caused by an RX event. And if the pseudo module for link status polling is not used, it will always be an RX event.

You could go as simple as:

#ifdef MODULE_STM32_ETH_LINK_UP switch (link_state) { case LINK_STATE_UP: netdev->event_callback(netdev, NETDEV_EVENT_LINK_UP); link_state = LINK_STATE_NOTIFIED_UP; return; case LINK_STATE_DOWN: netdev->event_callback(netdev, NETDEV_EVENT_LINK_DOWN); link_state = LINK_STATE_NOTIFIED_DOWN; return; } #endif /* MODULE_STM32_ETH_LINK_UP */ netdev->event_callback(netdev, NETDEV_EVENT_RX_COMPLETE);

PeterKietzmann

I've tested this PR again as explained here. Looks good! @maribu do you agree with the changes now?

PeterKietzmann · 2020-08-31T12:47:14Z

cpu/stm32/periph/eth.c

@@ -86,6 +98,9 @@ static char rx_buffer[ETH_RX_DESCRIPTOR_COUNT][ETH_RX_BUFFER_SIZE];
 /* Netdev used in RIOT's API to upper layer */
 netdev_t *_netdev;

+/* Used for checking the link status */
+static uint8_t _link_state = LINK_STATE_DOWN;


Compiler complains now about unused variable when module is not not enabled.

The variable is now only defined if the module is used.

maribu · 2020-08-31T18:20:28Z

@maribu do you agree with the changes now?

Yep, looks good to me. @JannesVolkens thanks for adding it, this is going to be very useful. From my point of view only squashing is needed.

(Note: You might find squashing easier if you commit fixes for previous commits using git commit --fixup=<COMMIT_TO_FIX>. Later on you can squash with git rebase -i --autosquash <COMMIT_TO_REBASE_ON>.)

JannesVolkens · 2020-08-31T18:52:05Z

@maribu Squashed.
And thank you for that hint! :)

JannesVolkens requested a review from miri64 as a code owner August 3, 2020 13:13

JannesVolkens removed the request for review from miri64 August 3, 2020 13:13

fjmolinas requested review from benpicco, jia200x and miri64 August 5, 2020 07:34

fjmolinas added Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Aug 5, 2020

benpicco requested a review from maribu August 5, 2020 09:26

PeterKietzmann reviewed Aug 7, 2020

View reviewed changes

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

pkg/lwip/contrib/netdev/lwip_netdev.c Outdated Show resolved Hide resolved

pkg/lwip/contrib/netdev/lwip_netdev.c Outdated Show resolved Hide resolved

JannesVolkens force-pushed the stm32_link_up branch from 4188e26 to 3a69968 Compare August 12, 2020 12:23

JannesVolkens changed the title ~~lwip: add 'NETDEV_EVENT_LINK_UP' event~~ drivers/stm32_eth: add 'NETDEV_EVENT_LINK_UP' event Aug 12, 2020

JannesVolkens force-pushed the stm32_link_up branch from 239e128 to 79afa99 Compare August 12, 2020 12:28

miri64 requested changes Aug 12, 2020

View reviewed changes

makefiles/pseudomodules.inc.mk Outdated Show resolved Hide resolved

tests/lwip/Makefile Outdated Show resolved Hide resolved

miri64 reviewed Aug 12, 2020

View reviewed changes

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

benpicco reviewed Aug 12, 2020

View reviewed changes

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

miri64 reviewed Aug 12, 2020

View reviewed changes

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

JannesVolkens removed the request for review from MrKevinWeiss August 27, 2020 08:22

aabadie reviewed Aug 27, 2020

View reviewed changes

benpicco reviewed Aug 27, 2020

View reviewed changes

PeterKietzmann reviewed Aug 27, 2020

View reviewed changes

benpicco reviewed Aug 27, 2020

View reviewed changes

maribu reviewed Aug 28, 2020

View reviewed changes

JannesVolkens force-pushed the stm32_link_up branch from 2a5db3d to 2d13318 Compare August 31, 2020 08:04

JannesVolkens requested review from DipSwitch, fjmolinas and vincent-d as code owners August 31, 2020 08:04

JannesVolkens removed request for DipSwitch, fjmolinas and vincent-d August 31, 2020 08:05

PeterKietzmann reviewed Aug 31, 2020

View reviewed changes

JannesVolkens force-pushed the stm32_link_up branch from 2d13318 to 627e906 Compare August 31, 2020 13:58

maribu added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Aug 31, 2020

drivers/stm32_eth: add 'NETDEV_EVENT_LINK_UP' event

f3e9349

JannesVolkens force-pushed the stm32_link_up branch from 627e906 to f3e9349 Compare August 31, 2020 18:51

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 31, 2020

PeterKietzmann approved these changes Aug 31, 2020

View reviewed changes

PeterKietzmann merged commit c5b63fc into RIOT-OS:master Aug 31, 2020

benpicco mentioned this pull request Jun 2, 2023

cpu/sam0_eth: interrupt based link detection/auto-negotiation #19703

Merged

		@@ -437,6 +511,11 @@ void stm32_eth_isr_eth_wkup(void)

		static void stm32_eth_isr(netdev_t *netdev) {
		netdev->event_callback(netdev, NETDEV_EVENT_RX_COMPLETE);

		/* Internal flags for the DMA descriptors */
		#define DESC_OWN (0x80000000)

		static uint8_t _link_status = 0;
		static uint8_t _first_call = 1;

drivers/stm32_eth: add 'NETDEV_EVENT_LINK_UP' event #14688

drivers/stm32_eth: add 'NETDEV_EVENT_LINK_UP' event #14688

Uh oh!

Conversation

JannesVolkens commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Aug 5, 2020

Uh oh!

jia200x commented Aug 5, 2020

Uh oh!

benpicco commented Aug 5, 2020

Uh oh!

PeterKietzmann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Aug 7, 2020

Uh oh!

maribu commented Aug 7, 2020

Uh oh!

benpicco commented Aug 7, 2020

Uh oh!

PeterKietzmann commented Aug 7, 2020

Uh oh!

miri64 commented Aug 7, 2020

Uh oh!

benpicco commented Aug 7, 2020

Uh oh!

jia200x commented Aug 7, 2020

Uh oh!

jia200x commented Aug 7, 2020

Uh oh!

JannesVolkens commented Aug 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benpicco commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JannesVolkens commented Aug 12, 2020

Uh oh!

Uh oh!

JannesVolkens commented Aug 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JannesVolkens commented Aug 3, 2020 •

edited

Loading

PeterKietzmann left a comment •

edited

Loading

benpicco commented Aug 12, 2020 •

edited

Loading