Skip to content

use small lock #15297

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_41
Dec 23, 2024
Merged

use small lock #15297
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_41

Conversation

@hujun260
Copy link
Copy Markdown
Contributor

Summary

use small lock in:
arch/arm/src/imxrt/imxrt_wdog.c
arch/arm/src/kinetis/kinetis_edma.c
arch/arm/src/lc823450/lc823450_dvfs2.c
arch/arm/src/lc823450/lc823450_timer.c
arch/arm/src/lpc54xx/lpc54_lowputc.c

Impact

none

Testing

ci

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-bit) architecture Size: M The size of the change in this PR is medium labels Dec 20, 2024
@nuttxpr
Copy link
Copy Markdown

nuttxpr commented Dec 20, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it's missing crucial details. While it provides a summary of what changed, it lacks the why. The impact section is too brief, and the testing section is insufficient.

Here's a breakdown of what's missing and how to improve it:

  • Summary: Needs to explain the rationale behind using smaller locks. What problem does this solve? Is it a performance optimization? A bug fix? Are there deadlocks being addressed? Be explicit. Also, specify the issue number if one exists.

  • Impact: While stating "none" is concise, it's likely inaccurate. Consider these points:

    • Performance: Does using smaller locks improve performance? If so, quantify it if possible. Even a qualitative assessment (e.g., "Slight performance improvement expected") is better than nothing.
    • Memory usage: Do the smaller locks reduce memory usage?
    • Real-time behavior: Does this change affect real-time guarantees?

    If the answer truly is "no impact" in all these areas, explicitly state it for each point (e.g., "Impact on performance: NO").

  • Testing: "ci" is not sufficient. While CI passing is important, the PR needs to show specific tests performed locally to validate the change. This includes:

    • Build Host(s): Specify the OS, CPU architecture, and compiler used.
    • Target(s): List the specific architectures and boards tested. "sim" is not enough; specify which simulator.
    • Logs: Provide relevant logs before and after the change to demonstrate the improvement or fix. If the change is a performance improvement, include timing measurements. If it's a bug fix, show the erroneous behavior before and the corrected behavior after.

Example of an improved Testing section:

Testing

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

* Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* Target(s):
    * sim:qemu_armv7m (imxrt1064-evk config)
    * ARM:  STM32F429I-DISC1 (default config)


Testing logs before change (imxrt1064-evk):

[log output showing previous behavior, e.g., timing measurements or error messages]


Testing logs after change (imxrt1064-evk):

[log output showing improved behavior, e.g., faster timing or absence of errors]

By providing more detail and specific evidence, you'll make the review process smoother and increase the chances of your PR being accepted.

Comment thread arch/arm/src/lpc54xx/lpc54_lowputc.c Outdated
Comment thread arch/arm/src/lc823450/lc823450_timer.c Outdated
Comment thread arch/arm/src/lpc54xx/lpc54_lowputc.c Outdated
@hujun260 hujun260 force-pushed the apache_41 branch 2 times, most recently from 3b82dd2 to 007b413 Compare December 20, 2024 10:28
Comment thread arch/arm/src/kinetis/kinetis_edma.c Outdated
Comment thread arch/arm/src/imxrt/imxrt_wdog.c
@hujun260 hujun260 force-pushed the apache_41 branch 2 times, most recently from c035d05 to 3057f4c Compare December 22, 2024 02:50
Comment thread arch/arm/src/kinetis/kinetis_edma.c
Comment thread arch/arm/src/kinetis/kinetis_edma.c
arch/arm/src/imxrt/imxrt_wdog.c
arch/arm/src/kinetis/kinetis_edma.c
arch/arm/src/lc823450/lc823450_dvfs2.c
arch/arm/src/lc823450/lc823450_timer.c
arch/arm/src/lpc54xx/lpc54_lowputc.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 46c2d46 into apache:master Dec 23, 2024
@hujun260 hujun260 deleted the apache_41 branch February 5, 2025 02:58
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 Board: arm Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants