Skip to content

Use small lock to protect resources related to i2c master and slave. - #15662

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
wangzhi16:lock_17
Jan 23, 2025
Merged

Use small lock to protect resources related to i2c master and slave.#15662
xiaoxiang781216 merged 1 commit into
apache:masterfrom
wangzhi16:lock_17

Conversation

@wangzhi16

Copy link
Copy Markdown
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Use small lock to protect resources related to i2c master and slave.

Impact

The underlying implementation of I2C master and slave.

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 Jan 23, 2025
@nuttxpr

nuttxpr commented Jan 23, 2025

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. While it provides a summary of the what, it lacks crucial details on the why and how. The impact section is too brief and doesn't address the specific points required. The testing section is inadequate, only mentioning CI which doesn't provide any concrete evidence of testing or the environment used.

Here's a breakdown of what's missing:

  • Summary:

    • Missing: Why is this lock necessary? What problem does it solve (e.g., race condition, data corruption)?
    • Missing: How does the lock work? Which specific resources are being protected? What type of lock is used?
    • Missing: Related issue references.
  • Impact: Needs to answer all the specific questions with NO/YES and provide descriptions for all YES answers. For example:

    • Impact on user: NO/YES (If YES: Will users need to change their code? Will there be any noticeable performance changes?)
    • Impact on build: NO/YES (If YES: Any new dependencies or configuration options?)
    • Impact on hardware: NO/YES (If YES: Which architectures/boards are affected?)
    • Impact on documentation: NO/YES (If YES: What needs to be documented?)
    • Impact on security: NO/YES (If YES: Does this fix a vulnerability? Introduce any new potential issues?)
    • Impact on compatibility: NO/YES (If YES: Does this break existing code? Change API behavior?)
  • Testing:

    • Missing: Build host details (OS, CPU, compiler).
    • Missing: Target details (architecture, board, configuration).
    • Missing: Actual testing logs before and after the change. "CI" is not sufficient; the reviewer needs to see evidence of the problem being fixed and the change working as intended.

The PR author needs to significantly expand these sections to meet the requirements. Simply saying "CI" is not enough; demonstrable proof of testing and detailed explanations of the change are crucial for a successful PR.

Comment thread arch/arm/src/stm32/stm32_i2c_v2.c Outdated
Signed-off-by: wangzhi16 <wangzhi16@xiaomi.com>
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: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crtical section should be replaced with spinlock as much as we can to improve SMP performance

4 participants