Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

imx9_flexcan: use small lock in arch/arm/src/imx9/imx9_flexcan.c
reason:
We would like to replace the big lock with a small lock.

Impact

imx9_flexcan

Testing

ci

@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 Dec 19, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 19, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a basic summary and identifies the impacted area, it lacks crucial details.

Here's a breakdown of what's missing:

  • Insufficient Summary: While the reason for the change is stated, the summary lacks specifics. It doesn't describe how the change works (the exact mechanism of replacing the lock) or what functional part of imx9_flexcan.c is modified. Mentioning functions, data structures, or specific code sections affected would be helpful. Linking a related NuttX issue is also highly recommended.

  • Incomplete Impact Assessment: Simply stating "imx9_flexcan" isn't sufficient. The PR needs to address all the listed impact categories (user impact, build impact, hardware impact, documentation, security, compatibility). For each, explicitly state "NO" or "YES" and provide a description if "YES." For example, even if the answer is "NO" for most, the PR should state "Impact on hardware: NO" for clarity. Since this change deals with locking, there's a potential impact on performance and real-time behavior, which needs to be addressed.

  • Inadequate Testing: "ci" is not enough. The PR must include:

    • Specific build host details (OS, CPU architecture, compiler version).
    • Specific target details (architecture, board, configuration).
    • Actual testing logs before and after the change demonstrating the intended effect. Just stating "ci" doesn't show what was tested or how the change improved things. Ideally, logs should show improved performance or demonstrate the resolution of a specific issue.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. It needs to explain the change thoroughly, analyze its impact comprehensively, and provide concrete evidence of testing.

@hujun260 hujun260 force-pushed the apache40 branch 2 times, most recently from f136b98 to e51a0ba Compare December 19, 2024 13:26
reason:
We would like to replace the big lock with a small lock.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
chip/stm32_gpio.c:44:11: note: '#pragma message: CONFIG_STM32_USE_LEGACY_PINMAP will be deprecated migrate board.h see tools/stm32_pinmap_tool.py'
   44 | #  pragma message "CONFIG_STM32_USE_LEGACY_PINMAP will be deprecated migrate board.h see tools/stm32_pinmap_tool.py"
      |           ^~~~~~~
CC:  pthread/pthread_mutexconsistent.c In file included from chip/stm32_flash.c:40:
chip/stm32f20xxf40xx_flash.c:53:4: warning: #warning "Default Flash Configuration Used - See Override Flash Size Designator" [-Wcpp]
   53 | #  warning "Default Flash Configuration Used - See Override Flash Size Designator"
      |    ^~~~~~~
board/stm32_ethernet.c:84:19: warning: 'g_phy_lock' defined but not used [-Wunused-variable]
   84 | static spinlock_t g_phy_lock = SP_UNLOCKED;
      |                   ^~~~~~~~~~
LD: nuttx

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 48c4391 into apache:master Dec 20, 2024
25 checks passed
@hujun260 hujun260 deleted the apache40 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: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants