Skip to content

Conversation

@hujun260
Copy link
Contributor

Summary

use small lock in following files:

arch/arm/src/stm32f0l0g0/stm32_serial_v2.c
arch/arm/src/stm32f7/stm32_qencoder.c
arch/arm/src/stm32f7/stm32_serial.c
arch/arm/src/stm32h5/stm32_serial.c
arch/arm/src/stm32h7/stm32_qencoder.c
arch/arm/src/stm32h7/stm32_serial.c
arch/arm/src/stm32l4/stm32l4_qencoder.c
arch/arm/src/stm32l4/stm32l4_serial.c
arch/arm/src/stm32l5/stm32l5_serial.c
arch/arm/src/stm32u5/stm32_serial.c
arch/arm/src/stm32wb/stm32wb_serial.c
arch/arm/src/stm32wl5/stm32wl5_serial.c
arch/arm/src/tiva/cc13xx/cc13xx_enablepwr.c
arch/risc-v/src/rv32m1/rv32m1_serial.c

Impact

arch/arm/src/stm32f0l0g0/stm32_serial_v2.c
arch/arm/src/stm32f7/stm32_qencoder.c
arch/arm/src/stm32f7/stm32_serial.c
arch/arm/src/stm32h5/stm32_serial.c
arch/arm/src/stm32h7/stm32_qencoder.c
arch/arm/src/stm32h7/stm32_serial.c
arch/arm/src/stm32l4/stm32l4_qencoder.c
arch/arm/src/stm32l4/stm32l4_serial.c
arch/arm/src/stm32l5/stm32l5_serial.c
arch/arm/src/stm32u5/stm32_serial.c
arch/arm/src/stm32wb/stm32wb_serial.c
arch/arm/src/stm32wl5/stm32wl5_serial.c
arch/arm/src/tiva/cc13xx/cc13xx_enablepwr.c
arch/risc-v/src/rv32m1/rv32m1_serial.c

Testing

ci

arch/arm/src/stm32f0l0g0/stm32_serial_v2.c
arch/arm/src/stm32f7/stm32_qencoder.c
arch/arm/src/stm32f7/stm32_serial.c
arch/arm/src/stm32h5/stm32_serial.c
arch/arm/src/stm32h7/stm32_qencoder.c
arch/arm/src/stm32h7/stm32_serial.c
arch/arm/src/stm32l4/stm32l4_qencoder.c
arch/arm/src/stm32l4/stm32l4_serial.c
arch/arm/src/stm32l5/stm32l5_serial.c
arch/arm/src/stm32u5/stm32_serial.c
arch/arm/src/stm32wb/stm32wb_serial.c
arch/arm/src/stm32wl5/stm32wl5_serial.c
arch/arm/src/tiva/cc13xx/cc13xx_enablepwr.c
arch/risc-v/src/rv32m1/rv32m1_serial.c

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: M The size of the change in this PR is medium labels Dec 23, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 23, 2024

[Experimental Bot, please feedback here]

No, this PR summary does not adequately meet the NuttX requirements. Here's why and how to fix it:

Summary Problems:

  • Vague "Why": Just saying "use small lock" is insufficient. Explain why this is necessary. Is it for performance improvement? Reduced memory usage? Deadlock avoidance? Be specific.
  • Missing "What": What is the "small lock"? Is this a new locking mechanism? Replacing an existing one? Which one? Provide details.
  • Missing "How": How is this "small lock" implemented? Does it use spinlocks? Mutexes? A custom mechanism? Describe the technical changes.
  • Missing Issue References: If this relates to a reported issue, provide the link(s).

Impact Problems:

  • Incomplete Sections: Simply listing file paths is not helpful. Address each impact point individually and answer "NO" or "YES" followed by a description if "YES". For example:

    • Impact on user: NO (No user-visible changes) or YES (Users will need to reconfigure X because Y).
    • Impact on build: NO or YES (New Kconfig option Z needs to be selected). And so on for all the impact categories.
  • Missing Detail: Even if there are no impacts for a certain category, explicitly stating "NO" is helpful for reviewers.

Testing Problems:

  • Insufficient Information: "ci" is not enough. State:
    • The specific build hosts you tested on (OS, compiler, etc.)
    • The specific target hardware/simulators you tested on.
  • No Logs: You must provide actual logs demonstrating the behavior before and after the change. This is crucial for verifying the fix. Even if the change seems minor, logs prove it works as expected and doesn't introduce regressions.

Example of an Improved Summary:

This change replaces the standard mutex used in the serial driver and qencoder driver on several platforms with a lighter-weight spinlock. This is done to improve performance in interrupt contexts where the serial driver is used, as the previous mutex implementation could introduce unnecessary latency. The small lock is implemented using the irqsave() and irqrestore() functions to protect critical sections. This addresses issue #123 (link to issue).

Example of Improved Impact:

  • Impact on user: NO (No user-visible changes)
  • Impact on build: NO
  • Impact on hardware: YES (Affects serial driver and qencoder driver functionality on STM32F7, STM32H7, STM32L4, STM32L5, STM32U5, STM32WB, STM32WL5, STM32F0L0G0, Tiva CC13xx, and RISC-V RV32M1).
  • Impact on documentation: YES (Documentation will be updated to reflect the new locking mechanism).
  • Impact on security: NO
  • Impact on compatibility: NO
  • Anything else to consider: Potential for increased interrupt latency if spinlock is held for excessive durations. This will be monitored in testing.

Example of Improved Testing:

  • Build Host(s): Linux (Ubuntu 20.04), GCC 9.4.0
  • Target(s): STM32F746-Discovery board (using stm32f746g-disco:nsh config), QEMU (RISC-V RV32M1)

Testing logs before change:

... (actual logs demonstrating the issue) ...

Testing logs after change:

... (actual logs showing the issue is resolved) ...

By providing more specific information and actual test results, you'll significantly increase the chances of your PR being accepted. Remember, clarity and evidence are key to a successful contribution.

@xiaoxiang781216 xiaoxiang781216 merged commit c96b8cd into apache:master Dec 23, 2024
27 checks passed
@hujun260 hujun260 deleted the apache_50 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 Arch: risc-v Issues related to the RISC-V (32-bit or 64-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.

3 participants