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 : move bytes2dlc/dlc2bytes to common header file #14759

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xucheng5
Copy link
Contributor

only keep can_bytes2dlc & can_dlc2bytes

Note: Please adhere to Contributing Guidelines.

Summary

Update this section with information on why change is necessary,
what it exactly does and how, if new feature shows up, provide
references (dependencies, similar problems and solutions), etc.

Impact

Update this section, where applicable, on how change affects users,
build process, hardware, documentation, security, compatibility, etc.

Testing

Update this section with details on how did you verify the change,
what Host was used for build (OS, CPU, compiler, ..), what Target was
used for verification (arch, board:config, ..), etc. Providing build
and runtime logs from before and after change is highly appreciated.

only keep can_bytes2dlc & can_dlc2bytes

Signed-off-by: xucheng5 <[email protected]>
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Area: Networking Effects networking subsystem Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Area: Drivers Drivers issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Nov 13, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 13, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it mentions keeping can_bytes2dlc and can_dlc2bytes, it lacks crucial information.

Specifically, it's missing:

  • Clear Explanation of the Change: The summary doesn't explain why only these two functions are being kept. What is being removed? What problem does this solve? What's the motivation?
  • Detailed Impact Assessment: The impact section is completely empty. This needs to be filled out describing the effects on users, build, hardware, documentation, security, and compatibility. Removing code almost certainly has some impact.
  • Testing Information: The testing section is also empty. What platforms were used for testing? What were the results? "Works as intended" is not sufficient; concrete examples of tests and their outcomes are needed. The logs sections are empty.

To make this PR acceptable, the author needs to:

  1. Explain the "why" behind the change. Provide a clear and concise explanation of the reason for removing the other functions. What problem did they cause? What are the benefits of removing them?
  2. Complete the Impact Assessment. Consider all aspects listed in the template (users, build, hardware, docs, security, compatibility) and explain how this change affects each. Even if the answer is "NO", it should be stated explicitly.
  3. Provide detailed testing information. List the build host(s) and target(s) used for verification. Include the actual testing logs from before and after the change to demonstrate the effect of the modification.

Only then will the PR meet the NuttX requirements.

Copy link
Contributor

@raiden00pl raiden00pl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not put common code in a separate header like include/nuttx/can_cmn.h. We shouldn't couple CAN chardev headers with SocketCAN headers. These are two separate implementations

@xucheng5
Copy link
Contributor Author

why not put common code in a separate header like include/nuttx/can_cmn.h. We shouldn't couple CAN chardev headers with SocketCAN headers. These are two separate implementations

'include nuttx/can_cmn.h' is ok

@acassis
Copy link
Contributor

acassis commented Nov 21, 2024

@xucheng5 I think it should be flexible enough to let some less common usage scenarios: i.e.: it could be possible to use the internal CAN peripheral from a MCU with SocketCAN and an external MCP2515 with CAN char dev. Using global variables shared by both will introduce issues to this case of use. I think your modifications are fine, but we need to test these usage case to confirm everything still working fine.

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: arm64 Issues related to ARM64 (64-bit) architecture Area: Documentation Improvements or additions to documentation Area: Drivers Drivers issues Area: Networking Effects networking subsystem Area: OS Components OS Components issues 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.

5 participants