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/netstats: L1/L2 per neighbor statistics #14448

Merged
merged 6 commits into from
Feb 9, 2021

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jul 6, 2020

Contribution description

This is a rebase of a previous PR to add a database for neighborhood metrics.
Those are needed for generating better routes with RPL (MR-HOF #7450).

I think it makes sense to attach this directly to the netifnetdev as originally proposed as those statistics are per-interface anyway.

Only rebase & slight cleanup done so far.

Testing procedure

When running examples/gnrc_networking you should now have a neigh command that prints statistics about neighbors.

Issues/PRs references

taken over from #6873
needed for #14623

@benpicco benpicco added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 6, 2020
@benpicco benpicco requested review from chrysn and bergzand July 6, 2020 15:16
@benpicco benpicco marked this pull request as ready for review July 15, 2020 22:23
@benpicco benpicco force-pushed the l2-peerstats-rebased branch from 3087601 to 3e98f6c Compare September 26, 2020 22:14
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 26, 2020
@benpicco benpicco force-pushed the l2-peerstats-rebased branch 2 times, most recently from 07c21fa to e199d59 Compare September 26, 2020 22:32
@benpicco benpicco force-pushed the l2-peerstats-rebased branch 2 times, most recently from b6e89b2 to c6a175d Compare October 9, 2020 23:09
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 10, 2020
@benpicco benpicco force-pushed the l2-peerstats-rebased branch from 57db5fe to 582786d Compare October 10, 2020 22:21
@chrysn
Copy link
Member

chrysn commented Oct 12, 2020

I've given this a first practical test by adding all the netstats_neighbor_* from the other example to an gnrc_border_router, where the BR is a nrf52840dongle that talks to a particle-xenon and another nrf52840dongle; both leaf nodes were continuously being pinged from upstream, and I varied the transmission properties by moving water containers into the leaf node's near field (ie touched the board).

In the quick test, the LQI and RSSI values were plausible ("-73 dBm 90" -- not great, not terrible). Other data stayed empty (while the received counter went up, sent and average tx time was 0 throughout) until I enabled more netstats (l2, ipv6, rpl) that were already active in the GNRC example -- are they actual dependencies?

ETX stayed at 200% for all links; I presume that'd need additional work for the nrf802154 driver?

When I powered off one of the leaves, its stats stayed constant (with the fresh=16) for minutes, even after I tried pinging them from the netstats board. That's consistent with the 10 minutes default before halving. Is that a good strategy? Allowing radio contact every five minutes for just a few packages to come through suffices to get what appears on the neigh interface to be a perfectly viable link, even though it only works while some door is open.

On the BR node, I'm seeing stats not only for the 6lo interfaces but for all. It's probably not urgent in this initial iteration, but is there a roadmap to not storing practically irrelevant stats there? (CDC-ECM is quite stable ;-) )

@bergzand
Copy link
Member

ETX stayed at 200% for all links; I presume that'd need additional work for the nrf802154 driver?

ETX relies on NETOPT_TX_RETRIES_NEEDED. @benpicco I don't want to shove more work on your plate, however do you think you could add an ETX_INVALID value for when the driver doesn't implement the netopt mentioned here? I don't mind if you postpone this to a follow up (not necessarily by you).

@benpicco
Copy link
Contributor Author

benpicco commented Oct 12, 2020

ETX stayed at 200% for all links; I presume that'd need additional work for the nrf802154 driver?

Can you try with USEMODULE += netdev_ieee802154_submac and #15208

On the BR node, I'm seeing stats not only for the 6lo interfaces but for all. It's probably not urgent in this initial iteration, but is there a roadmap to not storing practically irrelevant stats there? (CDC-ECM is quite stable ;-) )

Good point!

When I powered off one of the leaves, its stats stayed constant (with the fresh=16) for minutes, even after I tried pinging them from the netstats board. That's consistent with the 10 minutes default before halving. Is that a good strategy? Allowing radio contact every five minutes for just a few packages to come through suffices to get what appears on the neigh interface to be a perfectly viable link, even though it only works while some door is open.

Hmm I must admit that I didn't look much into how the 'freshness' is supposed to be used.
Maybe @bergzand still remembers? 😉

however do you think you could add an ETX_INVALID value for when the driver doesn't implement the netopt mentioned here?

How about just leaving it at 0 and then handling it like LQI and RSSI (if it's 0 set the value directly, otherwise use EWMA).

Then we can always handle 0 as invalid.

In an ideal world, all drivers would already be ported to the sub-mac and then we would fall back to software retransmissions if ETX stats are requested and the hardware does not support reporting them.

@chrysn
Copy link
Member

chrysn commented Oct 12, 2020

USEMODULE += netdev_ieee802154_submac and #15208

That makes it go down from 200% to 100% initially (as confidence is gained that the device is there), but levels at 100% even when I observe a loss of around 3% (which in first approximation means 105% ETX) or unplug the device for a minute. Funnily, it even keeps going down to 100% if, after a reboot, it is unplugged when the ETX is around 105%.

@benpicco
Copy link
Contributor Author

benpicco commented Oct 12, 2020

Ah retrans always gets re-set after send - try 4871b00.
Although I should probably just use the new ieee802154_tx_info_t.

edit nvm, still reports 0

@benpicco
Copy link
Contributor Author

ACKs are not enabled by default with the sub-MAC.
With

ifconfig 6 ack_req

I get the expected results.

miri64
miri64 previously requested changes Feb 8, 2021
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Some last style and code improvement nits remain. Other then that, this ready to merge, so please squash immediately.

@@ -0,0 +1 @@
../driver_netdev_common/main.c
Copy link
Member

Choose a reason for hiding this comment

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

I like when people know how to use Git features 🙂

@benpicco benpicco force-pushed the l2-peerstats-rebased branch 4 times, most recently from 7946254 to f1c3509 Compare February 8, 2021 19:43
@benpicco
Copy link
Contributor Author

benpicco commented Feb 8, 2021

Rebased again to the merge of #15694
Resolved the conflict gnrc_netif _send().

@benpicco benpicco requested a review from maribu February 8, 2021 19:44
@benpicco benpicco force-pushed the l2-peerstats-rebased branch from f1c3509 to 26ec165 Compare February 8, 2021 19:48
@miri64 miri64 dismissed their stale review February 8, 2021 20:25

Soft-ACK from my side. @maribu could you give this one final look regarding the usage of tx_sync in gnrc_netif's _send? Just to be on the save side.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

looks good to me

@miri64
Copy link
Member

miri64 commented Feb 9, 2021

Please squash

@benpicco benpicco force-pushed the l2-peerstats-rebased branch from 58861f6 to cc9c58a Compare February 9, 2021 11:28
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK!

@benpicco benpicco merged commit 5fba2c8 into RIOT-OS:master Feb 9, 2021
@benpicco benpicco deleted the l2-peerstats-rebased branch February 9, 2021 13:54
@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
@kaspar030 kaspar030 added the Release notes: added Set on PRs that have been processed into the release notes for the current release. label Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Release notes: added Set on PRs that have been processed into the release notes for the current release. Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants