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

boards/stm32/nucleo-f429zi: update netnsh defconfig #15727

Merged

Conversation

LuchianMihai
Copy link
Contributor

@LuchianMihai LuchianMihai commented Jan 30, 2025

Summary

This PR updates the netnsh defconfig for nucleo-f429zi board, as currently the board would not get ip.
Few changes comes with this PR:

  • nucleo-f429zi board uses lan8742 Ethernet Transceiver (PHY). My understanding is that MAC layer should be handled by the mcu as this transceiver will not. I've enabled CONFIG_NETINIT_NOMAC option.
  • I've enabled CONFIG_NETINIT_DHCPC and CONFIG_NETUTILS_TELNETD as I wanted to get a telnet shell and did not care about ip handling
  • I've disabled legacy pinmap and updated the board.h config to get the board to compile

Impact

Should be minimal, I guess compiles with and without this change, no source code changes are introduced.

Testing

No automated testing. Configured/compiled/run the example manually. At least locally it works. Pipelines will check if the regression still passes.

nsh> mount -t procfs /proc
nsh> ifconfig
eth0    Link encap:Ethernet HWaddr 00:e0:de:ad:be:ef at UP mtu 1486
        inet addr:192.168.88.253 DRaddr:192.168.88.1 Mask:255.255.255.0

lo      Link encap:Local Loopback at RUNNING mtu 1518
        inet addr:127.0.0.1 DRaddr:127.0.0.1 Mask:255.0.0.0

             IPv4   TCP   UDP  ICMP
Received     0004  0000  0003  0001
Dropped      0000  0000  0000  0000
  IPv4        VHL: 0000   Frg: 0000
  Checksum   0000  0000  0000  ----
  TCP         ACK: 0000   SYN: 0000
              RST: 0000  0000
  Type       0000  ----  ----  0000
Sent         0004  0000  0003  0001
  Rexmit     ----  0000  ----  ----
nsh> ping google.com
PING 172.217.17.142 56 bytes of data
56 bytes from 172.217.17.142: icmp_seq=0 time=60.0 ms
56 bytes from 172.217.17.142: icmp_seq=1 time=50.0 ms
56 bytes from 172.217.17.142: icmp_seq=2 time=50.0 ms
56 bytes from 172.217.17.142: icmp_seq=3 time=50.0 ms
56 bytes from 172.217.17.142: icmp_seq=4 time=50.0 ms
56 bytes from 172.217.17.142: icmp_seq=5 time=50.0 ms

@github-actions github-actions bot added Board: arm Size: S The size of the change in this PR is small labels Jan 30, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 30, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

This PR does not fully meet the NuttX requirements, although it's a good start. Here's a breakdown of what's missing and how to improve it:

Missing/Insufficient Information:

  • Summary: While the summary explains the "why" and the "what," it lacks clarity on the "how." Saying you "enabled CONFIG_NETINIT_NOMAC" isn't enough. Explain why this configuration change is necessary in relation to the lan8742. For example, "The lan8742 doesn't handle the MAC layer, requiring the MCU to manage it. Enabling CONFIG_NETINIT_NOMAC delegates MAC layer processing to the MCU." Be more explicit about the pinmap changes. What legacy pinmap was disabled? What specific changes were made to board.h?

  • Impact: The impact section is too vague. "Minimal" is not acceptable. Address each point specifically. Even if the answer is "NO," state it explicitly. Example:

    • Impact on user: YES. Users wishing to use networking on the nucleo-f429zi board will need to use this updated defconfig.
    • Impact on build: NO.
    • Impact on hardware: YES. This change specifically targets the nucleo-f429zi board and its lan8742 Ethernet transceiver.
    • Impact on documentation: YES/NO (Specify if documentation needs updating or if you've updated it as part of the PR).
    • Impact on security: NO. (or YES if applicable, explain why).
    • Impact on compatibility: NO (or YES if applicable, explain why. Does this break compatibility with older configurations or software that uses the previous pinmap?)
  • Testing: This is the weakest part. "No automated testing" isn't sufficient for a robust PR.

    • Be Specific about your Setup: Detail your build host OS, version, compiler version, NuttX version. For the target, provide the full board:config string.
    • Provide Meaningful Logs: "Works" is not a log. Show the output of ifconfig before and after the change to demonstrate that the board wasn't getting an IP address previously and now is. Include any relevant kernel messages related to network initialization. If you used telnet, show a successful telnet login.
    • Consider adding a simple networking test: Even a basic ping test would significantly improve the testing section.

Example of Improved Testing Section:

Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s):  macOS Ventura 13.5, Apple M2, GCC 12.2, NuttX commit hash: <insert commit hash>
* Target(s): sim:nucleo-f429zi:nsh

Testing logs before change:

nsh> ifconfig
... (output showing no IP address assigned)


Testing logs after change:

nsh> ifconfig
eth0 Link encap:Ethernet HWaddr XX:XX:XX:XX:XX:XX
inet addr:192.168.1.100 Bcast:192.168.1.255 Mask:255.255.255.0
UP BROADCAST RUNNING MULTICAST MTU:1500 Metric:1

nsh> ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
64 bytes from 192.168.1.1: icmp_seq=0 ttl=64 time=1.579 ms
...


By addressing these points, you'll create a much stronger PR that is more likely to be accepted and merged quickly. Remember, the goal is to make it as easy as possible for reviewers to understand and verify your changes.

Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

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

Please add a commit log message.

@hartmannathan
Copy link
Contributor

Please add a commit log message.

You can copy-and-paste most of the PR description and use it as the commit log.

@LuchianMihai LuchianMihai force-pushed the update-nucleo-f429zi-netnsh-config branch from 0d5be35 to 5bb782d Compare January 30, 2025 15:26
This PR updates the netnsh defconfig for nucleo-f429zi board, as currently the board would not get ip.
Few changes comes with this PR:
* enabled CONFIG_NETINIT_NOMAC option.
* enabled CONFIG_NETINIT_DHCPC and CONFIG_NETUTILS_TELNETD as I wanted to get a telnet shell and did not care about ip handling
* disabled legacy pinmap and updated the board.h config to get the board to compile
@LuchianMihai LuchianMihai force-pushed the update-nucleo-f429zi-netnsh-config branch from 5bb782d to 30f2073 Compare January 30, 2025 16:15
migrate from legacy pinmap for nucleo-f429zi board nsh config
Copy link
Member

@raiden00pl raiden00pl left a comment

Choose a reason for hiding this comment

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

thanks !

@hartmannathan
Copy link
Contributor

CI tests failed because:

curl: (6) Could not resolve host: www.segger.com
2025-01-30T17:45:55.1312533Z make[1]: *** [segger/Make.defs:82: segger/SystemView_Src_V356.zip] Error 6
2025-01-30T17:45:55.1313490Z make[1]: Target 'context' not remade because of errors.
2025-01-30T17:45:55.1314882Z make: *** [tools/Unix.mk:458: drivers/.context] Error 2
2025-01-30T17:45:55.1317746Z make: Target 'all' not remade because of errors.
2025-01-30T17:45:55.1357161Z /github/workspace/sources/nuttx/tools/testbuild.sh: line 385: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
2025-01-30T17:45:55.1410569Z   [1/1] Normalize stm32f429i-disco/systemview
2025-01-30T17:46:

Can't we somehow cache all the required files instead of downloading them every time?

This error is not related to anything in the PR, but we should try to get passing CI tests...

@xiaoxiang781216 xiaoxiang781216 merged commit 4606f1f into apache:master Jan 31, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants