Skip to content

Conversation

@OceanfromXiaomi
Copy link
Contributor

@OceanfromXiaomi OceanfromXiaomi commented Sep 18, 2024

Summary

detail: Add g_ prefix to can_dlc_to_len and len_to_can_dlc.

Impact

Testing

neededby:odm_541753

detail: Add g_ prefix to can_dlc_to_len and len_to_can_dlc.

Change-Id: I71d48b99f95b6ec7134239bfb2a7df29b7754ba4
Signed-off-by: zhaohaiyang1 <zhaohaiyang1@xiaomi.com>
@lupyuen
Copy link
Member

lupyuen commented Sep 18, 2024

[Experimental Bot, please feedback here]

This PR seems incomplete to meet NuttX requirements. Here's why and how to improve it:

Missing Information:

  • Summary:

    • Why? What's the reasoning behind adding the g_ prefix? Is it for coding style consistency, namespace collision avoidance, or another reason?
    • What? Be specific. Which files and functions are affected?
    • How? Briefly explain the technical details of adding the prefix.
  • Impact:

    • You need to address all impact categories. Even if the answer is "NO", state it explicitly (e.g., "Impact on build: NO").
    • For this change, pay close attention to:
      • Impact on user: Will existing user code break? Will they need to update their code to use the new prefixed functions?
      • Impact on compatibility: Does this affect backward compatibility?
      • Impact on documentation: Does the documentation need to be updated to reflect the name change?
  • Testing:

    • Insufficient Detail: "Build Host(s)" and "Target(s)" sections need concrete examples. List the specific operating systems, architectures, boards, and configurations you used.
    • Missing Logs: You must provide actual testing logs from before and after the change. This demonstrates that the code works as intended and helps reviewers understand the context.

Example Improvements:

Summary:

  • Why? Add g_ prefix to can_dlc_to_len and len_to_can_dlc to follow NuttX coding style conventions for global symbols, improving code readability and maintainability.
  • What? This change modifies the declarations and definitions of can_dlc_to_len and len_to_can_dlc within the CAN driver source files to use the g_ prefix.
  • How? The prefix is simply added to the function names wherever they are used (declarations, definitions, calls).

Impact:

  • Is new feature added? Is existing feature changed? NO
  • Impact on user: YES. Existing user code calling these functions will need to be updated to use the prefixed names (e.g., g_can_dlc_to_len).
  • Impact on build: NO
  • Impact on hardware: NO
  • Impact on documentation: YES. The documentation for the CAN driver will be updated to reflect the new function names.
  • Impact on security: NO
  • Impact on compatibility: Backward compatibility is broken. A migration guide will be provided in the documentation.
  • Anything else to consider: N/A

Testing:

  • Build Host(s):
    • Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
    • macOS 11.6.1, Apple M1, Clang 13.0.0
  • Target(s):
    • Simulator (qemu-system-arm), ARMv7-M, STM32F4Discovery board config
    • Actual Hardware: [Specify your board and configuration here]

Testing logs before change: [Provide relevant log snippets]

Testing logs after change: [Provide relevant log snippets showing the prefixed functions are used correctly]

By providing this level of detail, you make it easier for maintainers to review and merge your PR quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants