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

nuttx/can.h: support timestamp for can frame. #14096

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

OceanfromXiaomi
Copy link
Contributor

@OceanfromXiaomi OceanfromXiaomi commented Oct 11, 2024

Summary

nuttx/can.h: support timestamp for can frame
and update "can.rst" file for add struct timeval ch_ts info.

Impact

NULL.

Testing

tested on the board with cortex-M architecture.

@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Oct 11, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Here's why and how to improve it:

  • Insufficient Summary:
    • Missing Why: You need to explain the reason for adding timestamps. What problems does it solve? What use cases does it enable?
    • Missing How: Be specific about how timestamps are implemented. What data structure is used? Where is the timestamp information stored? How is it accessed?
  • Incomplete Impact:
    • Potential Hardware Impact: Adding timestamps might impact hardware timers or require driver modifications. Address this possibility.
    • Documentation: If this adds a new feature, documentation is crucial. Mention if documentation is included or needs to be added.
  • Vague Testing:
    • Insufficient Detail: Specify the exact board and configuration used.
    • Missing Logs: Provide relevant snippets of logs showing the timestamps working as expected.

Example of an Improved Summary:

"This PR adds support for CAN frame timestamps to provide more precise timing information for applications requiring accurate event sequencing or time-sensitive control. A new struct can_msg_ts structure is introduced that includes the standard struct can_msg fields along with a microsecond timestamp. The timestamp is captured by the CAN driver when a frame is received or transmitted. This change enables applications to analyze CAN bus traffic with higher temporal resolution."

drivers/can/Kconfig Outdated Show resolved Hide resolved
@acassis
Copy link
Contributor

acassis commented Oct 11, 2024

@OceanfromXiaomi please include a driver example to be used as reference, also please update Documentation/

@OceanfromXiaomi
Copy link
Contributor Author

l

let me ask, how to deal with "include a driver example to be used as reference"

@acassis
Copy link
Contributor

acassis commented Oct 14, 2024

l

let me ask, how to deal with "include a driver example to be used as reference"

@OceanfromXiaomi just implement the usage of CONFIG_CAN_TIMESTAMP in the can driver

@OceanfromXiaomi
Copy link
Contributor Author

l

let me ask, how to deal with "include a driver example to be used as reference"

@OceanfromXiaomi just implement the usage of CONFIG_CAN_TIMESTAMP in the can driver

@acassis OK, I will code this driver example.

@OceanfromXiaomi
Copy link
Contributor Author

@OceanfromXiaomi please include a driver example to be used as reference, also please update Documentation/

@acassis please tell me update what and where in Documentation/, Thank you~

@OceanfromXiaomi OceanfromXiaomi force-pushed the can-timestamp branch 2 times, most recently from cd41109 to c867fec Compare October 15, 2024 14:05
@acassis
Copy link
Contributor

acassis commented Oct 15, 2024

@OceanfromXiaomi please include a driver example to be used as reference, also please update Documentation/

@acassis please tell me update what and where in Documentation/, Thank you~

https://nuttx.apache.org/docs/latest/components/drivers/character/can.html

@github-actions github-actions bot added the Area: Documentation Improvements or additions to documentation label Oct 16, 2024
@xiaoxiang781216
Copy link
Contributor

@OceanfromXiaomi remove the merged patch from pr.

and update "can.rst" file for add struct timeval ch_ts info.

Signed-off-by: zhaohaiyang1 <[email protected]>
@GUIDINGLI GUIDINGLI merged commit 9985b05 into apache:master Oct 16, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation 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.

5 participants