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

examples/netpkt: add ethercat test example for robot #2777

Merged
merged 1 commit into from
Oct 27, 2024

Conversation

zhhyu7
Copy link
Contributor

@zhhyu7 zhhyu7 commented Oct 26, 2024

Summary

add a simple example to test ethercat's sending and receiving.

Impact

Testing

x86_64 device

@nuttxpr
Copy link

nuttxpr commented Oct 26, 2024

[Experimental Bot, please feedback here]

The provided PR description partially meets the NuttX requirements.

Here's what's good:

  • Summary: It clearly states the purpose of the PR - adding a test example for EtherCAT.
  • Testing: It mentions the build host (x86_64) and implies the target is a device (presumably running NuttX). However, more details are needed (see below).

Here's what needs improvement:

  • Summary:

    • What functional part? Be specific about which part of the codebase (e.g., drivers/net/ethercat/) is affected.
    • How does it work? Briefly describe the example's logic - what data is sent/received, how it's verified, etc.
    • Related issues/PRs: Link any relevant issues or PRs in the NuttX or NuttX-Apps repositories.
  • Impact: Address ALL impact points, even if the answer is "NO." Provide brief explanations for "YES" answers:

    • New feature? This seems like a "YES" - clarify if it's for users or just for development/testing.
    • User impact? Likely "NO" if it's just a test example, but confirm.
    • Build impact? Does this add any new build dependencies or options?
    • Hardware impact? Does this example require specific hardware or EtherCAT configurations?
    • Documentation impact? Does this example warrant an update or addition to the documentation?
    • Security impact? Unlikely, but analyze if the example handles any sensitive data.
    • Compatibility impact? Unlikely, but analyze if it introduces any changes to the EtherCAT interface.
  • Testing:

    • Target: Specify the exact architecture (e.g., ARM Cortex-M4), board, and configuration used for testing.
    • Logs: Instead of placeholder comments, include actual relevant log snippets from before and after the change. These logs should demonstrate the example's functionality.

Recommendation:

Revise the PR description to address the missing information and provide more specific details. A well-structured and informative PR description increases the chances of quick review and merging.


if (argc >= 2)
{
ifname = argv[1];
Copy link

Choose a reason for hiding this comment

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

assigning variable length input to const char[4] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

assigning variable length input to const char[4] ?

Is it modified like this int main(int argc, FAR const char *argv[4])? Done.

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Oct 27, 2024

Choose a reason for hiding this comment

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

why make this change? all.main should have the same prototype
int main(int argc, char *argv[]);

@zhhyu7 zhhyu7 force-pushed the upstream-3 branch 3 times, most recently from d894177 to 3e35d3c Compare October 27, 2024 04:08
add a simple example to test ethercat's sending and receiving

Signed-off-by: zhanghongyu <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 429befe into apache:master Oct 27, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants