Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

can: Add g_ prefix to can_dlc_to_len and len_to_can_dlc. #13523

Closed
wants to merge 1 commit into from

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 <[email protected]>
@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