Skip to content

Conversation

@OceanfromXiaomi
Copy link
Contributor

@OceanfromXiaomi OceanfromXiaomi commented Sep 30, 2024

Summary

add tx_confirm ability in upperCAN driver.

Impact

need merge with apache/nuttx-apps#2635

Testing

test with flagchip

add tx_confirm function in upperCAN driver1

Signed-off-by: zhaohaiyang1 <zhaohaiyang1@xiaomi.com>
@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 Sep 30, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 30, 2024

[Experimental Bot, please feedback here]

This PR summary needs substantial improvement to meet the NuttX requirements. Here's a breakdown of what's missing and how to address it:

Summary:

  • Insufficient Detail: Simply stating "add tx_confirm ability" is too vague.
    • What is tx_confirm? Explain the functionality in plain English. Is it about confirming successful transmission of CAN messages?
    • Why is this change necessary? What problem does it solve? Is it a new feature request? Does it improve performance or reliability?
    • What code is affected? Be specific about the driver name and the files modified. For example, "Modifies the upperCAN driver (drivers/can/uppercan.c) to..."

Impact:

  • All sections are blank. You must address each impact area. Even if the answer is "NO", state it explicitly. For example:
    • Is new feature added? YES - Adds tx_confirm functionality to the upperCAN driver.
    • Impact on user? YES - Users can now leverage the tx_confirm API for... [explain how they use it and the benefits]
    • Impact on build? NO
    • Impact on hardware? [Depends on the change. Does it require specific hardware support?]
    • Impact on documentation? YES - Documentation needs to be updated to describe the tx_confirm feature, its usage, and any API changes.
    • Impact on security? [Consider if the change introduces any new security considerations]
    • Impact on compatibility? [Does it break compatibility with previous versions? Does it introduce compatibility concerns with certain hardware?]

Testing:

  • Provide Specifics:
    • Build Host(s): List the operating system, CPU architecture, and compiler used. For example: "Linux (Ubuntu 20.04), x86_64, GCC 9.4.0".
    • Target(s): Specify the target architecture(s) and board configurations tested on. Use clear identifiers. For example:
      • "sim:qemu-arm"
      • "ARM: STM32F4 Discovery board"
    • Testing Logs: You must provide actual testing logs both before and after your changes. These logs demonstrate the issue you're fixing or the functionality you're adding.

Example (Improved Summary):

## Summary

This PR implements a message transmission confirmation mechanism (`tx_confirm`) within the upperCAN driver (`drivers/can/uppercan.c`). The `tx_confirm` feature allows applications to receive explicit confirmation from the driver when a CAN message has been successfully transmitted to the CAN bus. This addresses the need for reliable message delivery tracking in applications sensitive to CAN bus communication issues.

This change introduces a new API function, `uppercan_tx_confirm()`, which applications can use to register for transmission confirmation callbacks.

Related NuttX Issue: #[Insert issue number if applicable]

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: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants