Unified STM32 Network Interface #804

HTRamsey · 2023-03-21T03:07:35Z

New Unified STM32 driver for F4, F7, H5, H7.

Description

Implementation of new STM32 drivers. Does not support f2.

Test Steps

Checklist:

I have tested my changes. No regression in existing tests.
I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

#766

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

HTRamsey · 2023-03-23T20:52:07Z

@htibosch @AniruddhaKanhere Looks like this one NetworkInterface should work for all of Fxx & Hxx. The biggest difference between the HAL drivers are the usage of a few register macros that are named differently, but we can easily abstract that out. I've tested on a F767ZI and can test a H755ZI-Q, but don't have a way to test H5 at the moment.

htibosch · 2023-03-24T04:13:28Z

@holden-zenith wrote:

I've tested on a F767ZI and can test a H755ZI-Q, but don't have a way to test H5 at the moment.

If I'm not mistaken, I have an H747, F407, and F746 for testing.

So e.g. when testing F407, I use the latest HAL drivers for that part?

HTRamsey · 2023-03-24T04:25:37Z

@htibosch yes, last time I checked the latest F4 & F7 Eth drivers were 100% the same, so it should work the same. Before you test, I just noticed the call to HAL_ETH_ReleaseTxPacket needs to be moved to EMAC_IF_TX_EVENT because it calls the non interrupt safe version of vReleaseNetworkBufferAndDescriptor. I was using vNetworkBufferReleaseFromISR in the HAL_ETH_TxFreeCallback, but I guess it needs to be handled in the task to be compatible with both buffer allocation methods.

htibosch · 2023-03-24T05:43:32Z

but I guess it needs to be handled in the task to be compatible with both buffer allocation methods.

As you may have read, I also prefer to handle network buffers from a normal task, rather than from an interrupt.

I just came across this test:

    bool isInterrupt()
    {
        return (SCB->ICSR & SCB_ICSR_VECTACTIVE_Msk) != 0 ;
    }

which could be used to choose the correct network buffer function.

last time I checked the latest F4 & F7 Eth drivers were 100% the same

Doesn't F7 have multiple TX buffers each with their own hardware priority? I never used that feature, but I did have to disable it explicitly.

Would you mind to send me an email so I can ask you little things now and then? hein [at] htibosch [dot] net

Thanks,

htibosch · 2023-03-24T05:59:37Z

source/portable/NetworkInterface/STM32/NetworkInterface.c

+
+void HAL_ETH_RxAllocateCallback( uint8_t **buff )
+{
+ NetworkBufferDescriptor_t * pxBufferDescriptor = pxGetNetworkBufferWithDescriptor( ETH_RX_BUF_SIZE, pdMS_TO_TICKS( 0 ) );


Are you sure that HAL_ETH_RxAllocateCallback() will only be called from a task-context?

And also, do you always use BufferAllocation_2.c ? I think we'll have to use that to make sure that uncached memory will be used by DMA.

shubnil · 2023-03-31T09:16:43Z

HI @holden-zenith thanks for creating the change.
I wanted to get a little bit of context. Please help in explaining the thought behind coming up with this new driver.
I see that the older H and F series portable layers are moved inside legacy and the new driver has an integrated NetworkInterface.c file. Is the idea to have a single file to handle all the multiple stm32 SKUs.

HTRamsey · 2023-03-31T14:59:55Z

@shubnil Historically the STM32 HAL Eth drivers have been awful, which is part of the reason why edited HAL drivers were provided with the NetworkInterface.c. The newer HAL drivers are 'better' and all STM32's from F4, F7, H5, H7 essentially use the same driver now. So with this, edited HAL drivers are no longer necessary, it will be much easier to update to accommodate for future HAL updates, only one NetworkInterface is needed, and I am getting about 20mbps higher on iperf. I left the legacy drivers for now since anything prior to F4 won't be getting the HAL update, so technically I suppose only the Fxx driver would be necessary to leave.

htibosch · 2023-04-01T09:43:51Z

@holden-zenith , do you have a new version of NetworkInterface.c to test?

HTRamsey · 2023-04-02T03:10:41Z

@htibosch I'll probably push it in the next few days. I've been able to max out the bandwidth at 95mbps but it's not consistent and depends on compilation optimization. So I need to spend some time looking through the disassembly before I push it.

bjsowa · 2023-07-05T14:38:43Z

@holden-zenith What's the status of this? I have a Nucleo-H563ZI board and would like to use FreeRTOS+TCP but the current port implementation does not support H5 series.

HTRamsey · 2023-07-05T19:32:41Z

@bjsowa Actually it would be great if you could test it. I have used it successfully on an F7 but there is potential concern with how it compares to the older driver in terms of performance, at least when tested with an F4. I do not have an H5 at the moment to try it.

bjsowa · 2023-07-06T00:52:38Z

@holden-zenith I made the driver work for my nucleo board (at least DHCP and ping replies seem to work).
I posted the modified NetworkInterface.c with some comments here:
HTRamsey#1

I also ported it to work for version 4.0.0 of FreeRTOS-Plus-TCP (as I want IPv6 support) by mimicking the changes made in #602 for the current driver.

bjsowa · 2023-07-06T12:33:07Z

On bjsowa#1 I integrated the new driver with the CMake build system. You can use this for reference.

HTRamsey · 2023-07-06T13:45:18Z

@bjsowa oh nice that's great. I am working on testing an H7 too so all of F4, F7, H5, & H7 will have been tested. Seems to be at least usable on all of them for the time being. In theory it should perform at least as well as the last driver but iperf apparently has had mixed results so far.

htibosch · 2023-07-06T15:04:21Z

The earlier drivers for STM32Fxx and STM32Hxx used a copy of ST's HAL driver. The driver was optimised for efficiency/speed, and support for zero-copy was added.

@holden-zenith created a unified driver that is based on today's HAL drivers. The driver files must be obtained from ST.

I tried the new driver on STM32F4 and STM32F7, and found that iperf3 shows lower speeds, both TX and RX. I wonder what the influence is of the extra mutex?

I've got no testing equipment where I am now. In August I can do a real-time analyses of both drivers.

amazonKamath · 2023-07-18T12:23:51Z

@holden-zenith I made the driver work for my nucleo board (at least DHCP and ping replies seem to work). I posted the modified NetworkInterface.c with some comments here: holden-zenith#1

I also ported it to work for version 4.0.0 of FreeRTOS-Plus-TCP (as I want IPv6 support) by mimicking the changes made in #602 for the current driver.

@bjsowa Thanks for trying out the release candidate versions of 4.0.0. Did you face any difficulties or issues with IPv6? Also curious to know if you use the multiple endpoint feature?
PS: We will be soon doing a formal release (targeting mid August). The stack is currently undergoing security review and penetration testing.

HTRamsey · 2023-08-12T04:24:13Z

@htibosch Here are the ping results of the latest version. It seems to have much better latency, at least for me.
Old Driver:

New Driver:

htibosch · 2023-08-14T09:06:35Z

@holden-zenith wrote:

Here are the ping results of the latest version. It seems to have much better latency, at least for me.

That looks very promising. Dit you already commit and push the changes?
Do you want me to test it on a particular platform?

HTRamsey · 2024-07-05T04:34:21Z

@tony-josi-aws one alternative I've considered is to create a generic cmsis driver so people can just plug in their device files. But anyways this PR should be usable in its current state

HTRamsey · 2024-07-10T22:32:27Z

@htibosch @tony-josi-aws is it possible to ignore spellings and formatting for the included HAL drivers?

chinglee-iot · 2024-07-12T08:19:00Z

@htibosch @tony-josi-aws is it possible to ignore spellings and formatting for the included HAL drivers?

"source/portable/NetworkInterface/STM32" folder is excluded from spelling and formatting check.

source/portable/NetworkInterface/STM32/NetworkInterface.c

source/portable/NetworkInterface/STM32/CMakeLists.txt

Co-authored-by: Błażej Sowa <bsowa123@gmail.com>

bjsowa · 2024-07-27T07:39:49Z

@HTRamsey Could you look at HTRamsey#13 ?

Increment MAC entry index

HTRamsey · 2024-08-02T14:13:55Z

@ActoryOu Is there any plan to check this out? If not, it at least serves as a good example I guess

ActoryOu

Hi @HTRamsey,
Sorry for late response.
We had a internal discussion on this PR, then continue to review this.
Here are my comments. Please help take a look.

Thank you.

.github/workflows/ci.yml

source/portable/NetworkInterface/STM32/NetworkInterface.c

bjsowa · 2024-08-28T13:05:43Z

@HTRamsey What's the reasoning for removing HAL drivers? This makes the FREERTOS_PLUS_TCP_STM32_IF_DRIVER feature in CMakeLists.txt obsolete.

HTRamsey · 2024-08-28T15:34:21Z

I guess that depends on if we can add the hal drivers to be ignored by formatting and spell checks

HTRamsey · 2024-09-19T08:32:36Z

@ActoryOu @tony-josi-aws What should I do from here? I'd like to get this PR closed if not accepted

ActoryOu · 2024-09-19T09:37:05Z

@ActoryOu @tony-josi-aws What should I do from here? I'd like to get this PR closed if not accepted

Hi @HTRamsey,
Sorry for late. I'd like to approve this PR.
But why do you remove all *hal_eth.c files in the latest commit?

Thank you.

tony-josi-aws · 2024-09-19T10:04:36Z

Hi @HTRamsey,
Thanks for this contribution, and we are good with this change.
We plan to merge this PR by early October and are still running some tests.

Few observations that were found:

Build error with ipconfigETHERNET_DRIVER_FILTERS_PACKETS enabled [line]:
1. error: conflicting type qualifiers for 'ulRxDesc'
Comparitvely low iperf performance with TCP compared to the legacy STM32 network interface (still investigating if its configuration/tuning issue).

HTRamsey · 2024-09-19T10:23:48Z

@ActoryOu
I can re-add, but then we have to be able to ignore those files with spelling & formatting.

@tony-josi-aws
will fix that error, and I will check the low performance issue, I've had significant performance differences based on config values. I can max out around 97mbps depending on the config. There have been a couple of HAL ethernet driver updates that have not been released yet (but can be found on their github repos) that may help too.

ActoryOu · 2024-09-19T10:35:58Z

@ActoryOu I can re-add, but then we have to be able to ignore those files with spelling & formatting.

Got it. Then please re-add them and I'll approve this PR. Thank you!

Mixaill · 2024-09-19T21:16:48Z

@tony-josi-aws

The performance of iperf depends heavily on the configuration of the PC and MCU. As mentioned above, I was able to achieve a speed of ~93.3 Mbps.

#804 (comment)

New STM32 driver

6981b02

HTRamsey requested a review from a team as a code owner March 21, 2023 03:07

HTRamsey marked this pull request as draft March 21, 2023 03:17

New STM32 driver

3b92953

Improve error handling & ipconfig macro checking

d1d3098

HTRamsey force-pushed the dev-stm32 branch from 8d205c9 to d1d3098 Compare March 24, 2023 04:55

htibosch reviewed Mar 24, 2023

View reviewed changes

Holden added 2 commits March 24, 2023 13:06

Fix isr usage, add F4, and add missing xGetPhyLinkStatus

bffd9af

Style fixes and config implementations

2beff06

moninom1 mentioned this pull request May 29, 2023

Improve Default Macros #782

Merged

2 tasks

Holden added 2 commits July 23, 2023 21:42

Merge remote-tracking branch 'origin/main' into dev-stm32

520831c

update to 4.0.0 compatibility

dffe267

Add STM32 target to Network Interface selection

eaa7f49

Handle Driver Differences

b278985

HTRamsey force-pushed the dev-stm32 branch from f8f363e to b278985 Compare July 10, 2024 22:28

Merge branch 'main' into dev-stm32

390f69a

chinglee-iot and others added 2 commits July 12, 2024 16:02

Exclude portable STM32 for spelling and formating check

4e0ab39

Merge branch 'main' into dev-stm32

c614c67

bjsowa suggested changes Jul 12, 2024

View reviewed changes

source/portable/NetworkInterface/STM32/NetworkInterface.c Outdated Show resolved Hide resolved

source/portable/NetworkInterface/STM32/CMakeLists.txt Outdated Show resolved Hide resolved

HTRamsey and others added 2 commits July 12, 2024 14:45

Apply suggestions from code review

c18bfe7

Co-authored-by: Błażej Sowa <bsowa123@gmail.com>

Increment MAC entry index

0abb36b

HTRamsey and others added 2 commits July 27, 2024 03:31

Merge pull request #13 from bjsowa/dev-stm32-fix-mac-index

9576301

Increment MAC entry index

Fix packet length assert

445dc68

Merge branch 'main' into dev-stm32

b8c0fd8

ActoryOu reviewed Aug 19, 2024

View reviewed changes

HTRamsey force-pushed the dev-stm32 branch from c6e2849 to f4c36bb Compare September 19, 2024 11:02

Review Fixes

125b153

HTRamsey force-pushed the dev-stm32 branch from f4c36bb to 125b153 Compare September 19, 2024 11:05

Merge branch 'main' into dev-stm32

f737291

ActoryOu approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified STM32 Network Interface #804

Unified STM32 Network Interface #804

HTRamsey commented Mar 21, 2023 •

edited

Loading

HTRamsey commented Mar 23, 2023

htibosch commented Mar 24, 2023

HTRamsey commented Mar 24, 2023

htibosch commented Mar 24, 2023

htibosch Mar 24, 2023

shubnil commented Mar 31, 2023

HTRamsey commented Mar 31, 2023 •

edited

Loading

htibosch commented Apr 1, 2023

HTRamsey commented Apr 2, 2023

bjsowa commented Jul 5, 2023

HTRamsey commented Jul 5, 2023

bjsowa commented Jul 6, 2023

bjsowa commented Jul 6, 2023

HTRamsey commented Jul 6, 2023

htibosch commented Jul 6, 2023

amazonKamath commented Jul 18, 2023

HTRamsey commented Aug 12, 2023

htibosch commented Aug 14, 2023

HTRamsey commented Jul 5, 2024

HTRamsey commented Jul 10, 2024

chinglee-iot commented Jul 12, 2024

bjsowa commented Jul 27, 2024

HTRamsey commented Aug 2, 2024

ActoryOu left a comment

bjsowa commented Aug 28, 2024

HTRamsey commented Aug 28, 2024

HTRamsey commented Sep 19, 2024

ActoryOu commented Sep 19, 2024

tony-josi-aws commented Sep 19, 2024 •

edited

Loading

HTRamsey commented Sep 19, 2024 •

edited

Loading

ActoryOu commented Sep 19, 2024 •

edited

Loading

Mixaill commented Sep 19, 2024

Unified STM32 Network Interface #804

Are you sure you want to change the base?

Unified STM32 Network Interface #804

Conversation

HTRamsey commented Mar 21, 2023 • edited Loading

Description

Test Steps

Checklist:

Related Issue

HTRamsey commented Mar 23, 2023

htibosch commented Mar 24, 2023

HTRamsey commented Mar 24, 2023

htibosch commented Mar 24, 2023

htibosch Mar 24, 2023

Choose a reason for hiding this comment

shubnil commented Mar 31, 2023

HTRamsey commented Mar 31, 2023 • edited Loading

htibosch commented Apr 1, 2023

HTRamsey commented Apr 2, 2023

bjsowa commented Jul 5, 2023

HTRamsey commented Jul 5, 2023

bjsowa commented Jul 6, 2023

bjsowa commented Jul 6, 2023

HTRamsey commented Jul 6, 2023

htibosch commented Jul 6, 2023

amazonKamath commented Jul 18, 2023

HTRamsey commented Aug 12, 2023

htibosch commented Aug 14, 2023

HTRamsey commented Jul 5, 2024

HTRamsey commented Jul 10, 2024

chinglee-iot commented Jul 12, 2024

bjsowa commented Jul 27, 2024

HTRamsey commented Aug 2, 2024

ActoryOu left a comment

Choose a reason for hiding this comment

bjsowa commented Aug 28, 2024

HTRamsey commented Aug 28, 2024

HTRamsey commented Sep 19, 2024

ActoryOu commented Sep 19, 2024

tony-josi-aws commented Sep 19, 2024 • edited Loading

HTRamsey commented Sep 19, 2024 • edited Loading

ActoryOu commented Sep 19, 2024 • edited Loading

Mixaill commented Sep 19, 2024

HTRamsey commented Mar 21, 2023 •

edited

Loading

HTRamsey commented Mar 31, 2023 •

edited

Loading

tony-josi-aws commented Sep 19, 2024 •

edited

Loading

HTRamsey commented Sep 19, 2024 •

edited

Loading

ActoryOu commented Sep 19, 2024 •

edited

Loading