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

net/can/: add statistics for recv, sent and drop #15715

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

occam25
Copy link
Contributor

@occam25 occam25 commented Jan 29, 2025

Summary

Add support for network statistics for CAN. It includes counters for received, sent and dropped frames.
This change was necessary for our application to monitor the dropped can frames reported in the field.
The implementation has mimicked the existing ones for tcp and udp statistics, adding a can_stats_s struct to store the recv, sent and drop counters with the already existing net_stats_t type.

Impact

With this change, users can access the CAN statistics through procfs, the same way they access to all the other network statistics. The CAN recv, sent, and drop counters have been added to the /proc/net/stat output following the same design already existing for other network devices. This change doesn't change any driver behavior beyond just adding the new statistics. It does not introduce any new dependencies beyond the needed header files needed to access the new can_stats_s struct and g_netstats variable. These includes are nuttx/net/netconfig.h and nuttx/net/can.h.
The CAN statistics implemented are only enabled if CONFIG_NET_STATISTICS and CONFIG_NET_CAN configurations are set.

Testing

Tested using a Ubuntu 22.04 and a stm32h7 custom board with CONFIG_NET_STATISTICS and CONFIG_NET_CAN configurations enabled. The test application opens a CAN interface using socketCAN and echoes all the frames read. Every two seconds we print out the CAN statistics and see the recv and sent counters increase accordingly. To test the drop counter, we stop reading the CAN interface from the application so the frames start accumulating in the network buffer. Eventually, that buffer gets full and we start seeing the drop counter increasing as expected.
This traces are printed after reading the counters from /proc/net/stat
CAN stats:
Received: 00000022
Sent: 00000022
Dropped: 00000010

@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Jan 29, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 29, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and what's missing:

  • Summary: While it states the what, it lacks the why and how.

    • Missing: Why is this change necessary? Is it for a specific application, to improve monitoring, or to address a deficiency? How exactly does the change implement the statistics tracking? Are new syscalls added? Are existing data structures modified?
    • Needs: More detail on the implementation approach.
  • Impact: Almost entirely missing. This section needs to be filled out thoughtfully.

    • Missing: All impact assessments.
    • Needs: Consider each point. For example, impact on the user is likely YES as they now have access to new information. Describe how users can access the statistics (new ioctl calls, procfs entries, etc.). Does it impact build size? Does it change any driver behavior beyond just adding statistics? Does it introduce any new dependencies?
  • Testing: Insufficient.

    • Missing: "Testing logs before change" and "Testing logs after change" are empty. Also, "stm32h7 custom board" is too vague.
    • Needs: Provide actual log output demonstrating the statistics working. Be specific about the board and configuration used (e.g., "stm32h743-nucleo with the nsh config"). Show the commands used to generate the statistics and how the statistics are retrieved. Show logs before the change to demonstrate that the stats were not available and logs after the change showing the stats being collected and retrieved.

In short, the PR description needs to be significantly more detailed to meet the NuttX requirements. It needs to provide the missing why and how in the summary, complete the impact assessment, and include actual test logs demonstrating the change in functionality.

@acassis
Copy link
Contributor

acassis commented Jan 29, 2025

Nice work Javier! Really well done PR and very detailed PR summary! Kudos!!!

include/nuttx/net/can.h Show resolved Hide resolved
net/can/can_callback.c Show resolved Hide resolved
Add support for network statistics for CAN.
It includes counters for receive, sent
and drop frames.

Signed-off-by: Javier Casas <[email protected]>
can_datahandler returns an unsigned value that represent the
number of bytes actually bufffered, but the call to iob_tryadd_queue
returns a negative value when it fails so that value can't be used as
can_datahandler return value because it is interpreted as unsigned so
a big number is wrongly returned when iob_tryadd_queue fails.

The fix is just setting the return value to zero when iob_tryadd_queue
fails, reflecting the actual meaning that no bytes have been buffered.
@occam25 occam25 force-pushed the Add_CAN_statistics branch from 1183831 to 982b48c Compare January 30, 2025 09:25
@acassis acassis merged commit 52dbfda into apache:master Jan 30, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Networking Effects networking subsystem 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.

4 participants