Skip to content

[BREAKING] boards/defconfig: remove unused config about dd #16198

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Donny9
Copy link
Contributor

@Donny9 Donny9 commented Apr 13, 2025

Summary

boards/defconfig: remove unused config about dd

Impact

see apache/nuttx-apps#3057

Testing

sim:nsh

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

nuttxpr commented Apr 13, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and points to a related PR for impact, it lacks crucial details. Specifically:

  • Summary: Needs more detail. Why is the dd config being removed? Is it obsolete, redundant, causing a conflict, etc.? How exactly is it being removed (deleted, commented out, etc.)?
  • Impact: Linking to another PR is insufficient. The impact needs to be explicitly stated within the PR itself. While the linked PR might provide context, reviewers shouldn't have to chase down external links to understand the impact. Explicitly address all the impact checklist items (user, build, hardware, documentation, security, compatibility).
  • Testing: "sim:nsh" is inadequate. What tests were run? What were the results of the tests, both before and after the change? Provide concrete log output or descriptions of observed behavior demonstrating that the change works as intended and hasn't introduced regressions. Just stating the target platform doesn't prove anything.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements.

@lupyuen
Copy link
Member

lupyuen commented Apr 14, 2025

Sorry @cederom : Should we consider this PR a Breaking Change? Because CI Jobs for NuttX Kernel won't succeed, unless we patch NuttX Apps? Thanks :-)

@jerpelea jerpelea added the breaking change This change requires a mitigation entry in the release notes. label Apr 14, 2025
@lupyuen
Copy link
Member

lupyuen commented Apr 14, 2025

Hi @Donny9: Since this is a Breaking Change, could you please follow the Breaking Change Handling Process, and start the voting on the Mailing List? Thanks :-)
https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#113-breaking-changes-handling-process

(1) Please remember to indicate that voting will be open for 72 hours
(2) Vote +1 to Accept the Breaking PR, -1 to Reject the Breaking PR
(So that we won't have -1 votes concerning other issues)

@lupyuen lupyuen marked this pull request as draft April 14, 2025 10:16
lupyuen added a commit to lupyuen/nuttx-release that referenced this pull request Apr 14, 2025
@lupyuen
Copy link
Member

lupyuen commented Apr 14, 2025

Hi @Donny9: Because the Normal CI Checks won't work for this Breaking PR, I'm now running the Special CI Checks (see below) on our NuttX Build Farm. The CI Checks will complete in 36 hours, the CI Logs will appear here: https://gist.github.com/nuttxpr

https://github.com/lupyuen/nuttx-release/blob/main/run-ci-special.sh

## Repeat forever for All CI Jobs
for (( ; ; )); do
  for job in \
    arm-01 arm-02 arm-03 arm-04 \
    arm-05 arm-06 arm-07 arm-08 \
    arm-09 arm-10 arm-11 arm-12 \
    arm-13 arm-14 \
    arm64-01 \
    other \
    risc-v-01 risc-v-02 risc-v-03 risc-v-04 \
    risc-v-05 risc-v-06 risc-v-07 \
    sim-01 sim-02 sim-03 \
    x86_64-01 \
    xtensa-01 xtensa-02 xtensa-03
  do
  ## Run the CI in Docker Container
  ## If CI Test Hangs: Kill it after 3 hours
  sudo docker run -it \
    ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest \
    /bin/bash -c "
    set -x ;
    uname -a ;
    cd ;
    pwd ;
    git clone https://github.com/Donny9/incubator-nuttx nuttx --branch sem ;
    git clone https://github.com/Donny9/incubator-nuttx-apps apps --branch system_dd ;
    pushd nuttx ; git reset --hard HEAD ; echo NuttX Source: https://github.com/apache/nuttx/tree/\$(git rev-parse HEAD) ; popd ;
    pushd apps  ; git reset --hard HEAD ; echo NuttX Apps: https://github.com/apache/nuttx-apps/tree/\$(git rev-parse HEAD) ; popd ;
    sleep 10 ;
    cd nuttx/tools/ci ;
    ( sleep 10800 ; echo Killing pytest after timeout... ; pkill -f pytest )&
    (./cibuild.sh -c -A -N -R testlist/$job.dat || echo '***** BUILD FAILED') ;
  "
done

@cederom
Copy link
Contributor

cederom commented Apr 14, 2025

Hmm this one looks only like removing dd from defconfigs? So the real dd replacement is somewhere else? I wonder if it is really necessary to remove built-in nsh dd and not just add external dd as another option (both should be exlusive that way)?

@xiaoxiang781216
Copy link
Contributor

Hmm this one looks only like removing dd from defconfigs? So the real dd replacement is somewhere else?

since system dd is enable automatically in no small config:
https://github.com/apache/nuttx-apps/pull/3057/files#diff-9745c2aa3cbde9afc05370bbba5eb67b7bc7b98d1330684ae546de7fb762861fR8

I wonder if it is really necessary to remove built-in nsh dd and not just add external dd as another option (both should be exlusive that way)?

then, both implementation will unsync, which already happen now, please review apache/nuttx-apps#3057 to find the difference.

@lupyuen lupyuen changed the title boards/defconfig: remove unused config about dd [BREAKING] boards/defconfig: remove unused config about dd Apr 15, 2025
@lupyuen
Copy link
Member

lupyuen commented Apr 15, 2025

Sorry @Donny9 I think we need to update imxrt1020-evk:usdhc?

Configuration/Tool: imxrt1020-evk/usdhc,CONFIG_ARM_TOOLCHAIN_GNU_EABI
  [1/1] Normalize imxrt1020-evk/usdhc
52d51
< CONFIG_NSH_CMDOPT_DD_STATS=y

Do we need to update all defconfigs containing CONFIG_NSH_CMDOPT_DD_STATS?

@Donny9
Copy link
Contributor Author

Donny9 commented Apr 15, 2025

Sorry @Donny9 I think we need to update imxrt1020-evk:usdhc?

Configuration/Tool: imxrt1020-evk/usdhc,CONFIG_ARM_TOOLCHAIN_GNU_EABI
  [1/1] Normalize imxrt1020-evk/usdhc
52d51
< CONFIG_NSH_CMDOPT_DD_STATS=y

Do we need to update all defconfigs containing CONFIG_NSH_CMDOPT_DD_STATS?

okay~

@lupyuen
Copy link
Member

lupyuen commented Apr 15, 2025

Sorry @Donny9: qemu-armv7a:full is failing, would you know why?
https://gist.github.com/nuttxpr/82af5220c78137defbc00af0612a28b1#file-ci-arm-05-log-L1307

Cmake in present: qemu-armv7a/full,CONFIG_ARM_TOOLCHAIN_GNU_EABI
Configuration/Tool: qemu-armv7a/full,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2025-04-15 04:37:13
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Select HOST_LINUX=y
-- Build type:  
-- Host:    Linux/x86_64
-- Target:  NuttX/arm
-- Machine: arm
-- Vendor: none
-- Host:    Linux/x86_64
-- Target:  NuttX/arm
-- Machine: arm
-- C_FLAGS :  -Wall -Wextra
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Building NuttX...
On branch sem
Your branch is up to date with 'origin/sem'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   boards/arm/qemu/qemu-armv7a/configs/full/defconfig

Update: olimex-stm32-p407:knsh is also failing
https://gist.github.com/nuttxpr/421db0dad550b0220d00f70b6523ab82#file-ci-arm-09-log-L938-L961

Configuration/Tool: olimex-stm32-p407/knsh,CONFIG_ARM_TOOLCHAIN_GNU_EABI
  [1/1] Normalize olimex-stm32-p407/knsh
29d28
< CONFIG_NSH_DISABLE_DD=y

@github-actions github-actions bot added the Area: Documentation Improvements or additions to documentation label Apr 15, 2025
@Donny9
Copy link
Contributor Author

Donny9 commented Apr 15, 2025

Sorry @Donny9: qemu-armv7a:full is failing, would you know why? https://gist.github.com/nuttxpr/82af5220c78137defbc00af0612a28b1#file-ci-arm-05-log-L1307

Cmake in present: qemu-armv7a/full,CONFIG_ARM_TOOLCHAIN_GNU_EABI
Configuration/Tool: qemu-armv7a/full,CONFIG_ARM_TOOLCHAIN_GNU_EABI
2025-04-15 04:37:13
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Select HOST_LINUX=y
-- Build type:  
-- Host:    Linux/x86_64
-- Target:  NuttX/arm
-- Machine: arm
-- Vendor: none
-- Host:    Linux/x86_64
-- Target:  NuttX/arm
-- Machine: arm
-- C_FLAGS :  -Wall -Wextra
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Building NuttX...
On branch sem
Your branch is up to date with 'origin/sem'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   boards/arm/qemu/qemu-armv7a/configs/full/defconfig

Update: olimex-stm32-p407:knsh is also failing https://gist.github.com/nuttxpr/421db0dad550b0220d00f70b6523ab82#file-ci-arm-09-log-L938-L961

Configuration/Tool: olimex-stm32-p407/knsh,CONFIG_ARM_TOOLCHAIN_GNU_EABI
  [1/1] Normalize olimex-stm32-p407/knsh
29d28
< CONFIG_NSH_DISABLE_DD=y

@lupyuen i don't find any issue about olimex-stm32-p407/knsh's defconfig at my local enviroment, please check again~

@lupyuen
Copy link
Member

lupyuen commented Apr 15, 2025

@Donny9 Can you check this error? Thanks!
https://gist.github.com/nuttxpr/5bc91eefff4d15fb1a58e40c252ead2a#file-ci-arm-13-log-L212

Configuration/Tool: stm32l0538-disco/nsh,CONFIG_ARM_TOOLCHAIN_CLANG
/tools/gcc-arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.2.1/../../../../arm-none-eabi/bin/ld: nuttx section `.text' will not fit in region `flash'
/tools/gcc-arm-none-eabi/bin/../lib/gcc/arm-none-eabi/13.2.1/../../../../arm-none-eabi/bin/ld: region `flash' overflowed by 3088 bytes
Memory region         Used Size  Region Size  %age Used
           flash:       68624 B        64 KB    104.71%
            sram:        3556 B         8 KB     43.41%

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

@Donny9: The Special CI Checks for this PR are all successful except for stm32l0538-disco:nsh. Please remember to fix stm32l0538-disco:nsh and we'll rerun the Special CI Checks thanks!

@raiden00pl
Copy link
Member

@Donny9 feel free to add CONFIG_DEFAULT_SMALL=y to stm32l0538-disco/nsh

@Donny9
Copy link
Contributor Author

Donny9 commented Apr 17, 2025

@Donny9 feel free to add CONFIG_DEFAULT_SMALL=y to stm32l0538-disco/nsh

@raiden00pl I found that the reason for the flash overflow issue is the system dd calling the clock_gettime function. Therefore, I added the CONFIG_SYSTEM_DD_STATS option to enable this function(like origin config NSH_CMDOPT_DD_STATS), with the default value set to 'y'. Let's disable this configuration to reduce flash memory usage and maintain consistency with the original configuration.

@Donny9
Copy link
Contributor Author

Donny9 commented Apr 17, 2025

@Donny9: The Special CI Checks for this PR are all successful except for stm32l0538-disco:nsh. Please remember to fix stm32l0538-disco:nsh and we'll rerun the Special CI Checks thanks!

Done~

@lupyuen
Copy link
Member

lupyuen commented Apr 17, 2025

Cool! The Special CI Checks are still running, we'll finish the checks in about 6 hours.
https://gist.github.com/nuttxpr

@lupyuen lupyuen marked this pull request as ready for review April 17, 2025 11:51
@lupyuen
Copy link
Member

lupyuen commented Apr 17, 2025

@Donny9 Please close the voting. Thanks!
This Breaking PR compiles with the NuttX Breaking Changes Handling Process:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Board: arm Board: openrisc Board: z80 breaking change This change requires a mitigation entry in the release notes. 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.

9 participants