Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32Hxx porting fixes and suggestions #960

Merged
merged 11 commits into from
Jul 21, 2023
Merged

Conversation

miguelfreitas
Copy link
Contributor

STM32Hxx porting fixes and suggestions

Description

The changes are separated in different changesets so you may choose which one you may like to pick (if any). The first one is the most straightforward since it fixes code using functions that don't exist (they might have been renamed and the code was not updated).

The other changesets are suggestions on how to avoid conflicts with auto-generated files from STM32CubeIDE's project and original HAL ETH driver. Some suggestions may seem hacky but they ease maintenance allowing the ST's code generator to be used for reconfiguring the project without losing changes or reintroducing the conflicts.

Test Steps

The original code did not compiled.

Checklist:

  • I have tested my changes. No regression in existing tests. NOTE: I have just brought the interface up and tested with ping (it works).
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

None.

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

@miguelfreitas
Copy link
Contributor Author

Not sure if you agree with the hack of undef'ing HAL_ETH_MODULE_ENABLED to avoid conflict with original driver. In case you do, there's a new changeset uncrustified.

@tony-josi-aws
Copy link
Member

Hey @miguelfreitas, thanks for taking the time to contribute to FreeRTOS+TCP.

{
HAL_ETH_IRQHandler( &( xEthHandle ) );
}
#if 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very clear on the benefits of using the CubeIDE generated code for Ethernet driver and its initialization in this case. We are always defaulting to the ethernet driver present in the source/portable/NetworkInterface/STM32Hxx folder, even if the CubeIDE generates a different version. Also, there are couple of things that user needs to manually update in the project as you have listed in the readme.md to make it work with the autogenerated code, which I think defeats the purpose of the CubeIDE tool.

I believe there are a lot of users that doesn't rely on CubeIDE generated code (atleast for ethernet peripheral), removing ETH_IRQHandler from the NetworkInterface.c will break their code, unless they manually update it.

The only benefit that I see we get from the CubeIDE generated code are the two auto generated functions: HAL_ETH_MspInit() and HAL_ETH_MspDeInit(), which can be helpful in initializing/deinitializing the hardware (especially for the official ST development boards supported by the Cube IDE), but, again has to be manually tweaked for custom boards that runs on STM32Hxx. I think users can always refer the autogenerated version of these functions and implement their own version part of the application itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, @tony-josi-aws. I'm just sharing back the fixes plus the suggestions of how I have been able to integrate the lib to the project here. I didn't have any merge expectations specially on these hacks.

I totally agree with your comments, keeping ETH_IRQHandler at NetworkInterface.c seems pretty reasonable if one is able to remove that function from original driver to avoid conflict. This is not the case if these driver files keep getting overwritten by Cube IDE whenever I change a setting (so the conflict keeps coming back).

So in case of Cube IDE generated code I had to use such hacks to be able to disable parts of their code while keeping the project maintainable. I haven't thought of any other way... Unless maybe I could try to totally disable ETH from Cube IDE (which would also remove the define 'HAL_ETH_MODULE_ENABLED'). I don't know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miguelfreitas, yes, I usually disable code generation for ethernet from the Cube IDE and rely on the NetworkInterface.c. While disabling Cube IDE generated code for ethernet we have to manually define (uncomment) HAL_ETH_MODULE_ENABLED in Core\Inc\stm32h7xx_hal_conf.h.

STM32Hxx network interface was not fully updated to support the IPv6 related functionalities which got recently added to the main branch. The PRs: #970 and #965 (970 is an alternative approach for changes made in 965) should fix that. While updating those changes some of the build related fixes that you had made in your PR are already added in those PRs, which was required while testing. Will you be interested in updating your PR with respect to the latest changes or prefer us to do it on your behalf.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tony-josi-aws, thanks for this new info. I'd prefer you to to proceed with updating this or the other PR with the latest changes as you prefer since I'll be away on vacations for the next two weeks. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @miguelfreitas, I will update the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miguelfreitas, merging this PR. Thanks for contibuting to FreeRTOS+TCP.

@tony-josi-aws tony-josi-aws merged commit f89112c into FreeRTOS:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants