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

[RFC] net/netstats L1/L2 per neighbor transfer statistics #6873

Closed
wants to merge 8 commits into from

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Apr 8, 2017

This WIP adds functionality for recording statistics split out per peer. The goal of these statistics are for RPL parent selection, for example based on the ETX of the connection to the potential parent.

A struct with these statistics is defined. At this moment an array of length 8 is added to the netdev struct. I went for this approach because ETX statistics can be radio dependent and if a device has multiple radios these statistics are clearly dependent on the radio. Statistics are updated on packets received from the neighbor or transmission to the neighbor.

Because the netdev callback used to process transmission result has no knowledge of the transmitted frame itself, I decided to implement a small ring buffer like construction to link transfers to the callback. This ring buffer assumes that the order of packets send to the radio and the callbacks are in the same order. The ring buffer consists of a array of netstats_peer_t pointers and 2 indexes for reading/writing positions. Multicast/broadcast transmissions are recorded as NULL ptr's preserving the order of this buffer.

RSSI and LQI values are kept as exponential weighted moving averages, this to have some average function over this data while keeping the stored data small. I prefer to store the RSSI values in dBm, but the current radio's return the raw values instead of dBm converted values.

I've added a netopt call to retrieve the statistics. I've used this to extend the ifconfig tool with these statistics.

This PR is a work in progress. It's probably not feasible to have this merged before the feature deadline.

I would like some feedback on the architecture build for this. If a different approach is preferred, I can rewrite this. Also, if a different algorithm for certain parts is preferred, I'd also like to hear

Todo:

  • ETX calculations
  • Implement byte counters
  • Some kind of freshness indication
  • RSSI in dBm, Needs a different PR to fix this in the radio drivers

@bergzand bergzand force-pushed the l2-peerstats branch 2 times, most recently from 652e815 to d6bd7e8 Compare April 8, 2017 19:38
@bergzand bergzand changed the title WIP: net/netstats L1/L2 per peer transfer statistics [RFC] net/netstats L1/L2 per peer transfer statistics WIP Apr 11, 2017
@bergzand
Copy link
Member Author

Added RFC tag :)

@miri64 miri64 added Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels May 30, 2017
@bergzand bergzand force-pushed the l2-peerstats branch 2 times, most recently from 09559e4 to 5129d9d Compare June 15, 2017 15:04
@bergzand
Copy link
Member Author

Rebased and resolved merge conflict

@bergzand
Copy link
Member Author

bergzand commented Aug 6, 2017

Rework of implementation, including a rename.

Now based on #7447 and #7448, works better with #6895.
ETX and freshness is implemented.

The byte and packet counters are under an extra pseudomodule because they are not necessary for application requiring ETX such as RPL MRHOF.

@bergzand bergzand changed the title [RFC] net/netstats L1/L2 per peer transfer statistics WIP [RFC] net/netstats L1/L2 per neighbor transfer statistics WIP Aug 6, 2017
@bergzand bergzand changed the title [RFC] net/netstats L1/L2 per neighbor transfer statistics WIP [RFC] net/netstats L1/L2 per neighbor transfer statistics Aug 7, 2017
@bergzand
Copy link
Member Author

bergzand commented Aug 7, 2017

No longer WIP.

@miri64 miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Aug 7, 2017
@bergzand
Copy link
Member Author

bergzand commented Aug 8, 2017

Prevents a segfault when no empty/stale entries are present and added support for ethernet devices.

Note that ETX doesn't really make sense with tap because there is no L2 ack mechanism, but it simplifies testing a lot.

@smlng smlng removed this from the Release 2018.01 milestone Jan 15, 2018
@miri64
Copy link
Member

miri64 commented Mar 15, 2018

@bergzand I assume you are planning to port this to #8198 in the future?

@bergzand
Copy link
Member Author

Yes, At some point I hope to build a v2 of this based on #8198

@tcschmidt
Copy link
Member

@bergzand #8198 is merged, how to adapt right now?

@bergzand
Copy link
Member Author

@tcschmidt As requested here #8198 contained only the initial headers and code required for the layered approach. As long as the netdev stack is not adapted to actually use the code, it is not possible to reworking this PR in a useful way.

@benpicco
Copy link
Contributor

benpicco commented Jun 2, 2019

@bergzand is this still true?
What needs to be done before it's possible to add neighbor statistics into the stack and where should they go now?

@miri64
Copy link
Member

miri64 commented Jul 26, 2019

Ping @bergzand?

@bergzand
Copy link
Member Author

@miri64 @benpicco Unfortunately I don't have time to work on this. I'm also not sure where this functionality should be hooked into the stack. Mostly as there are now multiple ideas floating around on how the link/phy layer should evolve.

@chrysn
Copy link
Member

chrysn commented Sep 11, 2019

If this were resolved, would there be much work left to do to implement MRHOF? (The contiki users around me are kind of reluctant to switch that off).

@bergzand
Copy link
Member Author

If this were resolved, would there be much work left to do to implement MRHOF? (The contiki users around me are kind of reluctant to switch that off).

Around 350 lines of code depending a bit on how much things changed in two years

@stale
Copy link

stale bot commented Mar 14, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 14, 2020
@stale stale bot closed this Apr 14, 2020
@benpicco
Copy link
Contributor

netstats_nb shouldn't have a dependency on netdev.

This might actually be useful.
I think there is a case for allowing device-specific extra information per-neighbor.
E.g. with at86rf215 you might want to use different data rates for different nodes, depending on how good the link quality is.
You could also talk to MR-O-QPSK nodes and legacy O-QPSK nodes simultaneously if there was a way to store which node used which modulation.

@miri64 miri64 reopened this Apr 15, 2020
@stale stale bot closed this May 16, 2020
@miri64 miri64 removed the State: stale State: The issue / PR has no activity for >185 days label May 18, 2020
@miri64
Copy link
Member

miri64 commented May 18, 2020

Would have thought the bot removes the stale label if you re-open the issue...

@miri64 miri64 reopened this May 18, 2020
@benpicco
Copy link
Contributor

@miri64 @benpicco Unfortunately I don't have time to work on this. I'm also not sure where this functionality should be hooked into the stack. Mostly as there are now multiple ideas floating around on how the link/phy layer should evolve.

I can take over. From what I gathered by talking to @miri64 was that it should not be part of GNRC (so not extending the NIB) but rather a separate L2 database - so pretty much what you did here.

@benpicco benpicco self-requested a review June 12, 2020 20:12
@miri64 miri64 added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Jun 23, 2020
Comment on lines +43 to +44
/* find the oldest inactive entry to replace. Empty entries are infinity old */
netstats_nb_t *netstats_nb_get_or_create(netdev_t *dev, const uint8_t *l2_addr, uint8_t len)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to evict the entry with the worst signal?

Comment on lines +78 to +82
uint32_t tx_count; /**< Number of sent frames to this peer */
uint32_t tx_failed; /**< Number of failed transmission tries to this peer */
uint32_t rx_count; /**< Number of received frames */
uint32_t tx_bytes; /**< Bytes sent */
uint32_t rx_bytes; /**< Bytes received */
Copy link
Contributor

Choose a reason for hiding this comment

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

If those are just for printing out information, I'd rather drop them - 20 bytes / neighbor is quite a lot to keep around for curiosity's sake.

@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jan 6, 2021
@stale stale bot closed this Feb 9, 2021
@miri64
Copy link
Member

miri64 commented Feb 9, 2021

Very timely, stale bot, very timely ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: stale State: The issue / PR has no activity for >185 days 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.

10 participants