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

Fix mpfs targets and remove unnecessary ones #15753

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

jlaitine
Copy link
Contributor

@jlaitine jlaitine commented Feb 4, 2025

Summary

  • Reduces number of mpfs CI targets to save some CI compilation time. The "network" and "usb" targets have been removed, and networking and usb support have been added to existing "hwtest" target instead.

  • Increase stack sizes for nsh and hwtest targets, "nsh" stack usage has grown significantly

Impact

Reduce CI time, make mpfs bootable again after breaking commits.

Testing

Tested on Microchip MPFS Icicle board

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Board: risc-v Size: S The size of the change in this PR is small labels Feb 4, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 4, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it lacks some important details. While it addresses the core questions, it's too brief and lacks the specificity expected in a good PR. Here's a breakdown of what's missing and how to improve it:

Missing/Needs Improvement:

  • Summary: While the "what" is explained, the "why" for the stack size increase is missing. Why has NSH stack usage grown significantly? This is crucial context. The summary should also explicitly state that this is a fix for a broken build (implied, but should be stated).
  • Impact: While CI time reduction is mentioned, quantify it if possible (e.g., "Reduces CI time by approximately X minutes"). The "make mpfs bootable again" should be more descriptive: "Fixes a regression that prevented MPFS targets from booting after [commit hash or PR #]." Address all other impact categories explicitly with "NO" or a detailed "YES" explanation. Even if the answer is "NO," explicitly stating it ensures reviewers don't have to guess.
  • Testing: This is the weakest part. "Tested on Microchip MPFS Icicle board" is insufficient. Provide:
    • Specific Build Host Details: OS version, compiler version (including minor version), any relevant build tools. e.g., "Ubuntu 22.04, GCC 11.3.0, make 4.3"
    • Specific Target Details: MPFS Icicle board revision, NuttX configuration used (e.g., nsh), any specific hardware setup.
    • Actual Test Logs (Before & After): These are required. Show the boot process failing before the change and succeeding after. Demonstrate the improved stack usage (e.g., output of a stack analysis tool or demonstration of previously failing functionality now working). Even a simple "booted successfully" message in the logs is better than nothing.
  • Related Issues: Link to any related NuttX or NuttX Apps issues/PRs that prompted this change.

Example of Improved Testing Section:

Testing

I confirm that changes are verified on local setup and works as intended:

* Build Host(s): Ubuntu 22.04, GCC 11.3.0, make 4.3
* Target(s): Microchip MPFS Icicle Kit rev. A, configurations: nsh, hwtest

Testing logs before change (nsh config):

```

Testing logs after change (nsh config):

<Paste logs showing successful boot and NSH functioning correctly>
NuttShell (NSH)
nsh> free
<Output of free command showing adequate stack space>

Testing logs before change (hwtest config - demonstrating network/USB functionality now present):

<Paste logs showing missing network/USB functionality>

Testing logs after change (hwtest config):

<Paste logs showing network/USB functionality working correctly>

By providing more detail and specifically addressing all the requirements, the PR will be much easier to review and merge.  This thoroughness demonstrates that the changes are well-tested and understood, increasing confidence in their correctness.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

@jlaitine please include the new board profile Documentation/ to icicle board

@jerpelea
Copy link
Contributor

jerpelea commented Feb 4, 2025

please split board and arch commits in 2 separate PRs
if we plan to maintain a LTS release commits must be grouped

@jlaitine
Copy link
Contributor Author

jlaitine commented Feb 5, 2025

please split board and arch commits in 2 separate PRs if we plan to maintain a LTS release commits must be grouped

Sure! With this it can be done. PR #15763

Please note, however, that you can't require that for the LTS. Kconfig changes under arch need to be done together with all the board file defconfigs. Examples for this are:

  • Kconfig change may require normalization of the board files in order for CI to pass
  • When adding new HW drivers, the config flags are added in Kconfig under arch, but they also need to be enabled in defconfigs in order to build them in the CI

For the LTS, you will need to make separate PR:s against the LTS repo, and picking commits separately (and likely backporting them). It is unrealistic to pick full PR:s from master.

OT: Although LTS is a good idea in general, I don't understand where the resources for maintaining and testing those will come in this project - it will be huge work. But I keep my fingers crossed and wish for the best for that!

@jlaitine jlaitine force-pushed the reduce_mpfs_targets branch from a95b27b to f967757 Compare February 5, 2025 07:10
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation and removed Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture labels Feb 5, 2025
@jlaitine
Copy link
Contributor Author

jlaitine commented Feb 5, 2025

@jlaitine please include the new board profile Documentation/ to icicle board

Done!

@jlaitine jlaitine force-pushed the reduce_mpfs_targets branch from f967757 to 5791ea0 Compare February 5, 2025 07:12
@jlaitine jlaitine requested a review from acassis February 5, 2025 07:13
@raiden00pl
Copy link
Member

OT: Although LTS is a good idea in general, I don't understand where the resources for maintaining and testing those will come in this project - it will be huge work. But I keep my fingers crossed and wish for the best for that!

+1

@jerpelea
Copy link
Contributor

jerpelea commented Feb 5, 2025

please move documentation to a separate PR

@jlaitine
Copy link
Contributor Author

jlaitine commented Feb 5, 2025

please move documentation to a separate PR

Hm? This PR was previously rejected because documentation of the "hwtest" defconfig (which I didn't even author originally) was missing. So, I just added it. Now, if I move the documentation out of this PR, will this be rejected then again for that reason?

If this PR is not liked for some othe reason, it is fine by me to just leave the board configs as they are! I was just thinking that saving CI time would be beneficial, as this was an issue previously....

@jerpelea
Copy link
Contributor

jerpelea commented Feb 6, 2025

please move documentation to a separate PR

Hm? This PR was previously rejected because documentation of the "hwtest" defconfig (which I didn't even author originally) was missing. So, I just added it. Now, if I move the documentation out of this PR, will this be rejected then again for that reason?

If this PR is not liked for some othe reason, it is fine by me to just leave the board configs as they are! I was just thinking that saving CI time would be beneficial, as this was an issue previously....

We plan to maintain LTS releases we will have to split PRs in areas that they touch

For this PR we have both arch and documentation changes
an easy fix:

  • create a branch called documentation and cherry-pick commit on that branch
  • PR the documentation and we will manually merge the PR
  • remove the documentation from reduce_mpfs_targets branch and force push
  • this PR will be automatically updated and can be merged

I am sorry for asking you to do extra work

…test"

Reduce the amount of configurations for CI.

Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
Signed-off-by: Jukka Laitinen <jukkax@ssrc.tii.ae>
@jlaitine jlaitine force-pushed the reduce_mpfs_targets branch from 5791ea0 to 94e24f7 Compare February 7, 2025 07:56
@github-actions github-actions bot removed the Area: Documentation Improvements or additions to documentation label Feb 7, 2025
@jlaitine
Copy link
Contributor Author

jlaitine commented Feb 7, 2025

I am sorry for asking you to do extra work

There is no extra work at all! If it is needed, it is needed. I was just confused of what was requested as there was conflicting review requests. I'd prefer to keep things which belong together in the same PR, but for me it is fine to split it into separate ones as well.

The Documentation is now separated to #15784

And it is great to see that now there is really some attention paid to the PRs before merging them ;)

@anchao anchao merged commit e23f2ed into apache:master Feb 8, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: risc-v 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.

8 participants