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/notedump: Add a example to dump trace data by udp #3026

Closed
wants to merge 1 commit into from

Conversation

yangwei-x
Copy link

@yangwei-x yangwei-x commented Mar 16, 2025

Summary

Add a example to send the note trace data by udp

Usage:

nsh> notedump 192.168.31.9 6666

Impact

  • Add a Bultin App notedump when enable the notedump in example

Testing

  • Tested with board nucle-f746zg
NuttShell (NSH) NuttX-12.8.0
nsh> help
help usage:  help [-v] [<cmd>]

    .           cp          false       mkdir       rmdir       umount
    [           cmp         fdinfo      mkfifo      rmmod       unset
    ?           dirname     free        mkrd        route       uptime
    addroute    dd          help        mount       set         usleep
    alias       delroute    hexdump     mv          sleep       watch
    unalias     df          ifconfig    nslookup    source      xd
    arp         dmesg       insmod      pidof       test        wait
    basename    echo        kill        printf      time
    break       exec        pkill       ps          true
    cat         exit        ls          pwd         truncate
    cd          expr        lsmod       rm          uname

Builtin Apps:
    note        nsh         renew       telnetd
    notedump    ping        sh          trace
nsh> notedump 192.168.31.9 6666
Server address: 192.168.31.9
notedump_daemon: Opening /dev/note/ram
Sending note info to 192.168.31.9 6666
Sent 1396 bytes

@yangwei-x yangwei-x marked this pull request as draft March 16, 2025 13:42
@nuttxpr
Copy link

nuttxpr commented Mar 16, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. Here's why and how to fix it:

  • Summary: While it describes the "what," it lacks the "why." Why is sending trace data via UDP necessary? What problem does it solve? Is there a use case that motivates this addition? Also, the summary mentions TCP but the description says UDP. This discrepancy needs to be resolved. It needs to clearly state what functional part of the code is changed (e.g., addition to the apps/examples/ directory, new system call, modification to the networking stack). Finally, it needs to link a related NuttX issue if one exists (create one if it doesn't).

  • Impact: The impact section is incomplete. Address all the points:

    • Is new feature added?: YES
    • Is existing feature changed?: NO
    • Impact on user: YES (Users now have a new way to access trace data)
    • Impact on build: YES (Likely, as a new example app needs to be built. Explain what needs to be added to the configuration to enable it)
    • Impact on hardware: Probably NO (unless there are specific hardware dependencies). Justify the answer.
    • Impact on documentation: YES (Documentation needs to be added explaining how to use notedump, its options, and configuration.)
    • Impact on security: Potentially YES (Sending data over a network can have security implications if not handled carefully. Consider discussing this).
    • Impact on compatibility: Probably NO. Justify.
    • Anything else to consider: Mention any resource usage implications.
  • Testing: The testing section is insufficient.

    • Build Host(s): Provide complete details: OS version, specific compiler version (e.g., GCC 12.2.0).
    • Target(s): Be more specific than just "nucle-f746zg." State the NuttX configuration used (e.g., nsh).
    • Testing logs before change: Show the output of help before the change to demonstrate that notedump wasn't available.
    • Testing logs after change: While the provided logs show basic functionality, they don't demonstrate robust testing. Consider:
      • Different data sizes.
      • Error handling (what happens if the server isn't reachable?).
      • Different network conditions.

Example of Improved PR Description:

Add example to send trace data by UDP

## Summary

This change adds a new example application, `notedump`, to send NuttX note trace data over UDP. This is useful for debugging and monitoring systems remotely, especially in environments where serial access is limited or unavailable.  This enhances the existing tracing capabilities by providing a network-based transport mechanism.  This addresses [insert NuttX issue link here - create one if needed].

## Impact

* **Is new feature added?**: YES, the `notedump` application.
* **Is existing feature changed?**: NO
* **Impact on user**: YES. Users can now use the `notedump` command within NSH to stream trace data to a remote UDP server. They will need to configure the server's IP address and port.
* **Impact on build**: YES. The `apps/examples/notedump` needs to be added to the build system. To enable this example, add `CONFIG_EXAMPLES_NOTEDUMP=y` to your defconfig file.
* **Impact on hardware**: NO.  This change is purely software-based and does not introduce any new hardware dependencies.
* **Impact on documentation**: YES. Documentation will be updated in the `apps/examples/README.md` file to describe the usage of `notedump`, its command-line options, and build configuration.
* **Impact on security**:  Potentially YES. Transmitting trace data over a network exposes this information to potential interception. Users should be aware of this and take appropriate precautions, such as using a secure network or encrypting the data.
* **Impact on compatibility**: NO. This change does not affect existing functionality and should be backward compatible.
* **Anything else to consider**: The UDP transmission utilizes network resources. Excessive tracing might impact network performance in resource-constrained environments.

## Testing

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Ubuntu 22.04, GCC 12.2.0
* Target(s): STM32F746ZG (NUCLEO-F746ZG), `nsh` configuration.

Testing logs before change:

NuttShell (NSH) NuttX-12.8.0
nsh> help
... (output without notedump) ...


Testing logs after change:

NuttShell (NSH) NuttX-12.8.0
nsh> help
... (output with notedump included) ...
nsh> notedump 192.168.31.9 6666
... (successful output, including byte count) ...

nsh> notedump 192.168.31.100 6666 (invalid server IP)
... (output showing error handling) ...

nsh> notedump 192.168.31.9 6666 (with large amounts of trace data)
... (output demonstrating performance) ...

By addressing these points, your PR will be much clearer, more complete, and more likely to be accepted. Remember to keep the language concise and focused on the technical details.

@yangwei-x yangwei-x marked this pull request as ready for review March 16, 2025 14:01
@yangwei-x yangwei-x force-pushed the notedump branch 2 times, most recently from c8beebf to de11c70 Compare March 16, 2025 14:27
Add example to send the trace data by udp

Signed-off-by: Yang Wei <[email protected]>
PROGNAME = $(CONFIG_EXAMPLES_NOTEDUMP_PROGNAME)
PRIORITY = $(CONFIG_EXAMPLES_NOTEDUMP_PRIORITY)
STACKSIZE = $(CONFIG_EXAMPLES_NOTEDUMP_STACKSIZE)
MODULE = $(CONFIG_EXAMPLES_NOTEDUMP_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MODULE = $(CONFIG_EXAMPLES_NOTEDUMP_)
MODULE = $(CONFIG_EXAMPLES_NOTEDUMP)

* Included Files
****************************************************************************/

#ifdef EXAMPLES_UDP_HOST
Copy link
Contributor

Choose a reason for hiding this comment

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

where is EXAMPLES_UDP_HOST define?

Comment on lines +52 to +54
static uint32_t g_udpserver_ipv4;
static uint32_t g_udpserver_port;
static uint32_t note_sent_bytes;
Copy link
Contributor

Choose a reason for hiding this comment

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

move all global variable to stack


/* Now, sleep a bit. */

sleep(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sleep(1);

maybe we could remove this sleep since read note_fd is block mode

Comment on lines +41 to +47
#ifdef CONFIG_EXAMPLES_UDP_IPv6
# define AF_INETX AF_INET6
# define PF_INETX PF_INET6
#else
# define AF_INETX AF_INET
# define PF_INETX PF_INET
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Should we replace EXAMPLES_UDP_IPv6 with NET_IPv6 ?
  2. Could you explain where are AF_INETX and PF_INETX used for?

@yangwei-x yangwei-x marked this pull request as draft March 17, 2025 01:55
default n
depends on SCHED_INSTRUMENTATION_DUMP && NET_UDP
---help---
Enable the Note dump example
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the help text can include a short blurb about what the note dump example does? Something like: "This example reads data from the Note driver (/dev/note/ram) and sends it by UDP to the server-addr and server-port specified by command line arguments." WDYT?

@yangwei-x yangwei-x closed this Mar 29, 2025
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.

6 participants