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

drivertest: add arm-m signal through isr case #2877

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Dec 4, 2024

Summary

cmocka case for call signal in exception direct handler.

Impact

case only, fix cmake support for cmocka_driver_mps2_zerointerrupt

Testing

CI-test, local qemu mps2

@nuttxpr
Copy link

nuttxpr commented Dec 4, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, although it's missing some details. While concise, it could be improved by providing more specifics in the Impact and Testing sections. For example:

  • Impact: While it says "case only," it doesn't explicitly answer all the impact questions (e.g., Impact on user, build, hardware, documentation, security, compatibility). Even if the answer is "NO" for most, stating that explicitly is helpful for reviewers. The "fix cmake support" comment should be elaborated on – what exactly was fixed and why?
  • Testing: "CI-test, local qemu mps2" is a good start, but more detail would be beneficial. What specific CI tests were run? What configuration was used for qemu-mps2? While the template asks for "Testing logs before change" and "Testing logs after change," providing the full logs is usually not necessary. Instead, summarize the key differences or observed improvements. For example, "Before: Assertion failed. After: Test passed."

While the summary is brief, it does address the core questions. The provided information suggests that the changes are limited in scope (a new test case and a CMake fix), which simplifies the review process. However, providing explicit answers to all the questions, even if they are negative ("NO"), strengthens the PR and makes it easier for reviewers to quickly assess the changes.

@acassis
Copy link
Contributor

acassis commented Dec 8, 2024

@jasonbu please include documentation about drivertest, the entry is empty: https://nuttx.apache.org/docs/latest/applications/testing/drivertest/index.html
Also please remove the Vela specific reference, since it is integrated into mainline is doesn't depend on Vela.

@xiaoxiang781216 xiaoxiang781216 merged commit 1f8b9aa into apache:master Dec 10, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants