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

nuttx/arch: remove the custom board check in up_testset implementation #14963

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

extinguish
Copy link

@extinguish extinguish commented Nov 27, 2024

Summary

The up_testset implementation is common code, should not add custom board check.
If there are new boards that need to use their own up_testset() implementation, for example, CONFIG_ARCH_CHIP_1XXX, has provided thier own up_testset() implementation, in order to working with CONFIG_ARCH_CHIP_1XXX,
we need to change the implemetaton to:

#if defined(CONFIG_ARCH_HAVE_TESTSET) \
    && !defined(CONFIG_ARCH_CHIP_LC823450) \
    && !defined(CONFIG_ARCH_CHIP_CXD56XX) \
    && !defined(CONFIG_ARCH_CHIP_RP2040)  \
    && !defined(CONFIG_ARCH_CHIP_1XXX)
static inline_function spinlock_t up_testset(volatile spinlock_t *lock)
{

which would make the macro check becoming longer and longer;
beside from this, if the board of CONFIG_ARCH_CHIP_1XXX will not upstream to github, then this code will also cannot upstream to github;

beside from the upper cause, another reason for introducing a new configuration:CONFIG_ARCH_HAVE_CUSTOM_TESTSET is that generally a board belongs to an architecture. For example, CONFIG_ARCH_CHIP_LC823450 belongs to ARMV7_M. Then ARMV7_M itself has CONFIG_ARCH_HAVE_TESTSET enabled.
In this case, all the boards under ARMV7_M have CONFIG_ARCH_HAVE_TESTSET enabled by default.
However, CONFIG_ARCH_CHIP_LC823450 doesn't require CONFIG_ARCH_HAVE_TESTSET. We can't turn off CONFIG_ARCH_HAVE_TESTSET for all the boards under the entire ARMV7_M just for the sake of this single CONFIG_ARCH_CHIP_LC823450.
Therefore, we have introduced a new option called CONFIG_ARCH_HAVE_CUSTOM_TESTSET.

Impact

If some boards need custom up_testset implementation, should not to add macro control on arch/arm/include/spinlock.h, just modify the board config inside arch/arm/Kconfig will work.

Testing

has passed ostest

@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 Nov 27, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 27, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although the testing section could be improved.

Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the problem (increasingly complex macro checks in up_testset), the proposed solution (CONFIG_ARCH_HAVE_CUSTOM_TESTSET), and the reasoning behind it (avoiding long macros and accommodating non-upstreamed boards). The explanation regarding architecture-level vs. board-level configuration is helpful.
  • Impact Description: The impact section clearly states the change for users (modifying board config) and avoids unnecessary changes to the core code.
  • Addresses a Real Problem: The proposed change tackles maintainability issues arising from conditional compilation.

Weaknesses:

  • Testing is Insufficient: "has passed ostest" is not enough detail. You need to provide:
    • Specific build host information (OS, compiler version, etc.)
    • Specific target information (architecture, board, configuration)
    • Actual log output before and after the change, demonstrating the difference in behavior or resource usage. Even if the output is identical, showing the logs proves that the tests still pass with the new configuration.

Recommendations to improve the PR:

  1. Expand the Testing Section: Provide the specific details mentioned above. Show the commands used to run the tests. If ostest is a suite of tests, list the specific tests within that suite that were run. If there's a noticeable improvement (e.g., reduced code size, faster execution), quantify it.
  2. Consider a Before/After Code Example: Include a small snippet showing how the Kconfig would be modified for a board using a custom up_testset. This would enhance clarity for reviewers.

By addressing these points, you'll make your PR stronger and easier for reviewers to assess.

the up_testset implementation is common code, should not add custom
board check

Signed-off-by: guoshichao <[email protected]>
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 Area: OS Components OS Components issues 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.

4 participants