Skip to content

Conversation

@stbenn
Copy link
Contributor

@stbenn stbenn commented Nov 6, 2024

Transferred ethernet/MAC Kconfig options for stm32h7 to stm32h5. The IP for the ethernet peripheral is identical between the two chips, so the h5 implementation will start off the same as the h7 implementation.

This also cleaned up some leftover comments in the Kconfig, such as the AHB peripheral menu section comments.

Summary

Before porting the stm32h7 ethernet driver to the stm32h5, need to add the configuration options to the Kconfig. This just copied over the options but renamed STM32H7 to STM32H5.

Impact

The change currently does not effect hardware as there are no H5 ethernet drivers implemented. This allows the menuconfig to set the proper options for when ethernet support is added for the H5.

Testing

Not much to test other than going through the menuconfig and comparing to the H7. I ran the arm-13 job with the docker container, since it includes Nucleo-H563ZI board, and there were no build errors.

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium labels Nov 6, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 6, 2024

[Experimental Bot, please feedback here]

The PR mostly meets the NuttX requirements, but needs improvements in the "Testing" and "Impact" sections.

Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change.
  • Good Justification: The rationale for transferring configuration options due to identical IP between the two chips is sound.
  • Acknowledges Limited Testing: The author acknowledges the current limitations in testing due to the driver not being implemented yet.

Weaknesses:

  • Insufficient Testing: While acknowledging the limitations, the testing section is too weak. Simply stating "no build errors" isn't enough. Even without a driver, the author should demonstrate that the Kconfig options are selectable, savable, and result in the expected configuration settings being reflected in the build system (e.g., checking generated header files or build output). They should also test the menuconfig interface to ensure no unintended side effects or broken menus.
  • Incomplete Impact Assessment: While hardware impact is mentioned (correctly noting no driver yet), other impact areas are missing or underdeveloped.

Recommendations for Improvement:

  • Testing:

    • Demonstrate Kconfig Functionality: Step through the configuration options in menuconfig, save the configuration, and show that the expected preprocessor defines are generated. Include snippets of the relevant configuration file and the generated header file (or build output showing the defines) in the PR.
    • Regression Testing (if possible): Even if the Ethernet driver isn't available, are there any related configurations that could be impacted? Test those as well.
    • Target Specificity: Specify the exact configuration used in menuconfig (e.g., nsh). Mentioning arm-13 is too broad. Provide the full configuration name or even better, attach the .config file to the PR.
  • Impact: Address the following points more thoroughly:

    • Impact on build (will build process change)? While you say "no build errors", does adding these Kconfig options change anything in the build process itself? Think about dependencies, generated code, etc. If there's truly no impact, explicitly state "NO - The build process remains unchanged. The addition of Kconfig options does not introduce new dependencies or alter the existing build steps."
    • Impact on documentation (is update required / provided)? YES. Even if it's a small update, the documentation should reflect the new configuration options. Either update the documentation as part of the PR or create a follow-up issue to track the documentation update.
    • Impact on compatibility (backward/forward/interoperability)? Consider potential implications. For example, would a configuration saved with these new options be compatible with older NuttX versions? If not, mention that. If there's no impact, state that clearly.

By addressing these points, the PR will be much stronger and more likely to be accepted.

This is a combination of 2 commits:

H5 Kconfig ethernet options added.
  - This commit cleans up unecessary comments in the Kconfig and adds the the Ethernet/MAC menu.
    The menu is copied from the H7 menu, since the peripheral IP is identical.

Remove trailing whitespaces.

Update suggested mask
@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Nov 7, 2024

ci break should be fixed by: #14663

@xiaoxiang781216 xiaoxiang781216 merged commit ca74d81 into apache:master Nov 7, 2024
@stbenn stbenn deleted the eth-kconfig branch November 7, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants