net stats: move layer 2 netstats from netdev driver to gnrc_netif by smlng · Pull Request #9793 · RIOT-OS/RIOT

smlng · 2018-08-17T15:26:26Z

Contribution description

Currently handling and collection of layer 2 netstats is rather distributed, i.e. some is handled in the low level netdev drivers other parts are in GNRC. This PR move handling of layer 2 netstats to gnrc_netif. This is done step by step so each commit should be one after another.

Testing

use tests/gnrc_netif or examples/gnrc_networking and transmit something, then lookup ifconfig.

Issues/PRs references

miri64 · 2018-08-18T20:02:34Z

Mh this somewhat collides with the efforts @bergzand is doing in disentangling netdev into a layered system. Can you two please sync on that.

smlng · 2018-08-18T21:50:07Z

@miri64 we did ... via IRC, that's (also) why I asked for his review here - and yours bc you know GNRC best 😄

miri64 · 2018-08-18T22:18:42Z

Ah, I didn't know about that ;-) (I don't follow the IRC that closely). Then what spoke against putting those stat counters into its own netdev layer instead of putting it in gnrc_netif (where it is misplaced IMHO, since then other stacks would need to re-implement them).

bergzand · 2018-08-22T08:47:15Z

Mostly that this is the easiest solution at the moment. AFAIK there are no other network stacks using or exposing netstats, so I don't see problems with moving the implementation to GNRC specific code.

IMHO it can be easily moved to a separate netdev layer if necessary, but for now I'm happy with this as it removes another cleanup step I would have to do before being able to implement a layered netdev structure

sys/net/gnrc/netif/gnrc_netif.c

bergzand

Completely missed that this was updated. I have some minor issue remaining, changes look good otherwise.

sys/net/gnrc/netif/gnrc_netif_ieee802154.c

sys/net/gnrc/netif/gnrc_netif_raw.c

sys/net/gnrc/netif/gnrc_netif_ethernet.c

smlng · 2018-10-30T07:40:31Z

@bergzand comments addressed, maybe today HnA provides a merge window?

bergzand · 2018-11-16T10:39:22Z

@smlng What do you think, do you want to have every device tested individually? I'm willing to trust the code as long as it compiles and with the three gnrc_netif variants tested.

jnohlgard

@smlng A question inline below.
I don't intend to block this PR as I would like to see this merged soon to reduce the number of open PRs on the netdev/netif parts to move forward on the refactoring project.

sys/net/gnrc/netif/gnrc_netif.c

…T-OS#9793

jnohlgard · 2018-12-12T14:44:06Z

esp_eth_netdev needs adaptation:

make: Entering directory '/tmp/dwq.0.012982471714853783/906cdea00a9ea8b7aef8f4696ae664b9/examples/gnrc_networking'
ESP32_SDK_DIR should be defined as /path/to/esp-idf directory
ESP32_SDK_DIR is set by default to /opt/esp/esp-idf
Building application "gnrc_networking" for "esp32-olimex-evb" with MCU "esp32".

esp_eth_netdev.c: In function '_esp_eth_init':
esp_eth_netdev.c:173:19: error: 'netdev_t {aka struct netdev}' has no member named 'stats'
     memset(&netdev->stats, 0, sizeof(netstats_t));
                   ^
esp_eth_netdev.c: In function '_esp_eth_send':
esp_eth_netdev.c:219:15: error: 'netdev_t {aka struct netdev}' has no member named 'stats'
         netdev->stats.tx_success++;
               ^
esp_eth_netdev.c:220:15: error: 'netdev_t {aka struct netdev}' has no member named 'stats'
         netdev->stats.tx_bytes += dev->tx_len;
               ^
esp_eth_netdev.c:226:15: error: 'netdev_t {aka struct netdev}' has no member named 'stats'
         netdev->stats.tx_failed++;
               ^
esp_eth_netdev.c: In function '_esp_eth_recv':
esp_eth_netdev.c:280:15: error: 'netdev_t {aka struct netdev}' has no member named 'stats'
         netdev->stats.rx_count++;
               ^
esp_eth_netdev.c:281:15: error: 'netdev_t {aka struct netdev}' has no member named 'stats'
         netdev->stats.rx_bytes += size;
               ^

smlng · 2019-01-24T08:26:15Z

ping @bergzand, I don't wanna merge this myself and w/o your thumps up

miri64 · 2019-01-24T11:25:08Z

Just from experiences with refactorings I also would rather wait until after the FF to merge this.

smlng · 2019-01-24T12:29:34Z

@miri64 agreed, actually I wasn't pushing for getting this into the release but rather getting of my desk with open PRs 😄

bergzand · 2019-01-28T18:28:14Z

sys/net/gnrc/link_layer/gomach/gomach_internal.c

    gomach_preamble_ack_hdr.phase_in_us = gnrc_gomach_phase_now(netif);

-    pkt = gnrc_pktbuf_add(NULL, &gomach_preamble_ack_hdr, sizeof(gomach_preamble_ack_hdr),
+    gnrc_pktsnip_t *pkt = gnrc_pktbuf_add(NULL, &gomach_preamble_ack_hdr, sizeof(gomach_preamble_ack_hdr),


Unrelated change?

In theory yes, as it is not related to netstats. However, as I touched the file the static code analyser found (that is: complained) that the previous value of pkt (which is NULL see above) was not used before being overwritten. To me this seems a bit overly correct, but in the end I need to please the CI to get the green button.

TL;DR: change is necessary to make CI happy 😦

TL;DR: change is necessary to make CI happy frowning

Makes sense

I prefer variable definitions to be at the top of a block so one can have an overview and consider alignment (or rather more important: resulting padding i.e. waste from misalignment) on the stack, so I would prefer to just remove the = NULL above and keep this line as it was. E.g. I don't know the size of gnrc_gomach_frame_preamble_ack_t so I would prefer it to be defined / allocated last, so the padding "hole" is at the end.

bergzand · 2019-01-28T18:45:14Z

@smlng Minor remark above, otherwise good to go I'd say

smlng · 2019-01-31T13:51:10Z

@miri64 (re)adapted as suggested, lets see what Murdock thinks

smlng · 2019-02-01T08:12:50Z

@miri64 can I squash?

miri64 · 2019-02-01T08:34:35Z

Yes please :-)

This the first step in moving the collection of layer 2 netstats from the low level driver to a central location, ie. gnrc_netif, to avoid code duplication.

This removes the netopt to get layer 2 netstats from netdev.

smlng · 2019-02-01T08:41:22Z

done

miri64 · 2019-02-01T09:13:35Z

cpu/esp32/esp-wifi/esp_wifi_netdev.c

-        netdev->stats.tx_success++;
-        netdev->stats.tx_bytes += dev->tx_len;
-        #endif
+#ifdef MODULE_NETSTATS_L2


Wait... This changes behavior. Before this was not in the #ifdef block.

mhm, right, will remove ... can I amend directly?

Removing usage of netdev->stats in all net drivers, as it is handled by gnrc_netif.

miri64

Re-ACK and go. My last-minute comment was addressed.

gschorcht · 2019-02-07T13:16:40Z

BTW, esp-wifi isn't compilable anymore with this PR. I will provide a fix.

…T-OS#9793

smlng added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation GNRC CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 17, 2018

smlng requested review from bergzand and miri64 August 17, 2018 15:26

smlng force-pushed the pr/netstats_l2 branch from dfc3d9a to 13a7cb1 Compare August 17, 2018 19:28

bergzand requested changes Aug 22, 2018

View reviewed changes

sys/net/gnrc/netif/gnrc_netif.c Outdated Show resolved Hide resolved

smlng force-pushed the pr/netstats_l2 branch from 13a7cb1 to 279cbac Compare August 28, 2018 14:25

bergzand mentioned this pull request Sep 1, 2018

WIP: driver/winc1500: Initial port for Wi-Fi module against NETDEV_ETH. #7433

Closed

4 tasks

miri64 removed their request for review September 25, 2018 19:15

bergzand requested changes Oct 3, 2018

View reviewed changes

sys/net/gnrc/netif/gnrc_netif_ieee802154.c Outdated Show resolved Hide resolved

sys/net/gnrc/netif/gnrc_netif_raw.c Outdated Show resolved Hide resolved

sys/net/gnrc/netif/gnrc_netif_ethernet.c Outdated Show resolved Hide resolved

miri64 removed the GNRC label Oct 5, 2018

bergzand self-assigned this Oct 13, 2018

smlng force-pushed the pr/netstats_l2 branch 2 times, most recently from 839822b to 97f240a Compare October 30, 2018 07:39

bergzand mentioned this pull request Nov 13, 2018

RFC: A layered approach to netdev #7736

Open

bergzand approved these changes Nov 16, 2018

View reviewed changes

bergzand mentioned this pull request Nov 16, 2018

netstats: at86rf2xx: tx_bytes only includes 15.4. header, no payload #10407

Closed

jnohlgard reviewed Dec 10, 2018

View reviewed changes

sys/net/gnrc/netif/gnrc_netif.c Show resolved Hide resolved

jnohlgard pushed a commit to jnohlgard/RIOT that referenced this pull request Dec 10, 2018

squash kw41zrf: Remove references to netdev_t::stats, required by RIO…

bff7cf1

…T-OS#9793

smlng force-pushed the pr/netstats_l2 branch from 97f240a to 5b9abce Compare December 11, 2018 08:09

jnohlgard pushed a commit to jnohlgard/RIOT that referenced this pull request Dec 11, 2018

squash kw41zrf: Remove references to netdev_t::stats, required by RIO…

0f13878

…T-OS#9793

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 22, 2019

bergzand reviewed Jan 28, 2019

View reviewed changes

bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 28, 2019

smlng added 2 commits February 1, 2019 10:35

netstats: collect layer 2 netstats in gnrc_netif

6d0384f

This the first step in moving the collection of layer 2 netstats from the low level driver to a central location, ie. gnrc_netif, to avoid code duplication.

netdev: remove netapi call for layer 2 netstats

a0324bd

This removes the netopt to get layer 2 netstats from netdev.

smlng force-pushed the pr/netstats_l2 branch from 8ae52d3 to a05df38 Compare February 1, 2019 08:35

miri64 requested changes Feb 1, 2019

View reviewed changes

smlng added 4 commits February 1, 2019 11:25

netdev: remove layer2 netstats from netdev drivers

6183d5f

Removing usage of netdev->stats in all net drivers, as it is handled by gnrc_netif.

netstats_l2: remove obsolete struct from netdev_t

b8d9610

netstats_l2: remove obsolete struct from xbee_t

0369850

gomach: fix reassign before usage error

734524a

smlng force-pushed the pr/netstats_l2 branch from a05df38 to 734524a Compare February 1, 2019 09:26

miri64 approved these changes Feb 1, 2019

View reviewed changes

miri64 merged commit 49f2715 into RIOT-OS:master Feb 1, 2019

gschorcht mentioned this pull request Feb 7, 2019

cpu/esp8266: removes netstats in esp wifi netdev driver #10965

Merged

danpetry added this to the Release 2019.04 milestone Mar 12, 2019

benemorius pushed a commit to benemorius/RIOT that referenced this pull request Jun 22, 2019

squash kw41zrf: Remove references to netdev_t::stats, required by RIO…

f33849c

…T-OS#9793

smlng deleted the pr/netstats_l2 branch June 25, 2019 08:43

benemorius pushed a commit to benemorius/RIOT that referenced this pull request Jun 29, 2019

squash kw41zrf: Remove references to netdev_t::stats, required by RIO…

f25f1a5

…T-OS#9793

Conversation

smlng commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing

Issues/PRs references

Uh oh!

miri64 commented Aug 18, 2018

Uh oh!

smlng commented Aug 18, 2018

Uh oh!

miri64 commented Aug 18, 2018

Uh oh!

bergzand commented Aug 22, 2018

Uh oh!

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smlng commented Oct 30, 2018

Uh oh!

bergzand commented Nov 16, 2018

Uh oh!

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnohlgard commented Dec 12, 2018

Uh oh!

smlng commented Jan 24, 2019

Uh oh!

miri64 commented Jan 24, 2019

Uh oh!

smlng commented Jan 24, 2019

Uh oh!

bergzand Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

smlng Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

bergzand Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

bergzand commented Jan 28, 2019

Uh oh!

smlng commented Jan 31, 2019

Uh oh!

smlng commented Feb 1, 2019

Uh oh!

miri64 commented Feb 1, 2019

Uh oh!

smlng commented Feb 1, 2019

Uh oh!

miri64 Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

smlng Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

gschorcht commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

smlng commented Aug 17, 2018 •

edited

Loading