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

arm/irq.h: fix ARCH_ARM7TDMI missing up_getusrsp after move to irq.h #15442

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Jan 7, 2025

Summary

Causing c5471evm/httpd ci break.

the default irq.h path is not involved when previous patch CI.

# include <arch/arm/irq.h>

Impact

fix #15425 break the c5471evm/httpd.
fix the arm32 default path no up_getusrsp in irq.h

Testing

CI-test, local build of c5471evm/httpd.

Causing c5471evm/httpd ci break.

Signed-off-by: buxiasen <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Jan 7, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 7, 2025

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. Here's why and how to fix it:

Missing Information/Insufficient Detail:

  • Summary: "Causing c5471evm/httpd ci break" is a symptom, not a summary. Explain what change is being made to fix the break. What was the root cause of the CI break? Be explicit. Mentioning the irq.h path is a start but doesn't explain the change itself. Is a function being added, removed, or modified?
  • Impact - User: Does this fix impact how the user interacts with the c5471evm/httpd functionality? Likely not, but explicitly stating "NO" is better.
  • Impact - Build: Probably "NO," but confirm. If the include paths were modified, you might need to mention that.
  • Impact - Hardware: Specify "c5471evm" explicitly under hardware impact. While arm32 is mentioned, being specific about the board is crucial.
  • Impact - Documentation: If the change affects how something works, even internally, consider if documentation needs updates (likely "NO" here, but state it).
  • Impact - Security, Compatibility, Other: Explicitly state "NO" for each if not applicable.
  • Testing: "CI-test" is insufficient. Provide specific CI run links. "Local build" needs details – which OS, compiler, etc. were used? Critically, you must provide actual log snippets showing the error before the fix and the successful operation after the fix. Empty code blocks as provided don't demonstrate anything.

How to Improve:

## Summary

This PR fixes a CI break for the c5471evm/httpd configuration introduced by a previous change (mention the commit hash or PR number if possible). The previous change inadvertently caused the default irq.h path to be incorrect, resulting in a missing `up_getusrsp` symbol during compilation.  This PR corrects the irq.h include path to ensure the correct definition is available.

Related NuttX Issue: #15425

## Impact

* Is new feature added? NO
* Is existing feature changed? NO (Bug fix restoring previous functionality)
* Impact on user: NO
* Impact on build: NO
* Impact on hardware: YES (c5471evm)
* Impact on documentation: NO
* Impact on security: NO
* Impact on compatibility: NO
* Anything else to consider? NO


## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
* Target: arm, c5471evm:default

Testing logs before change:

/path/to/build/directory/httpd: undefined reference to `up_getusrsp'
collect2: error: ld returned 1 exit status
make: *** [/path/to/build/directory/httpd] Error 1


Testing logs after change:

Linking /path/to/build/directory/httpd
... (other build output)
Successfully built target httpd


CI test results:
* [Link to successful CI run]

By providing specifics and addressing all the required points, the PR will be much easier to review and merge. Remember to replace the placeholders like "[Link to successful CI run]" with the actual information.

@jasonbu
Copy link
Contributor Author

jasonbu commented Jan 7, 2025

ci error should because of #15441

@xiaoxiang781216
Copy link
Contributor

let's merge this simple patch to unblock ci.

@xiaoxiang781216 xiaoxiang781216 merged commit 7e0e4de into apache:master Jan 7, 2025
11 of 25 checks passed
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: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants