Skip to content

gnrc_rpl: introduce control plane traffic statistics#5381

Merged
miri64 merged 3 commits into
RIOT-OS:masterfrom
cgundogan:pr/netstats_rpl
Sep 27, 2016
Merged

gnrc_rpl: introduce control plane traffic statistics#5381
miri64 merged 3 commits into
RIOT-OS:masterfrom
cgundogan:pr/netstats_rpl

Conversation

@cgundogan

@cgundogan cgundogan commented Apr 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Provide statistics about RX/TX DIOs, DISs, DAOs, DAO-ACKs.

based on #5178

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking GNRC labels Apr 21, 2016
@cgundogan cgundogan added this to the Release 2016.07 milestone Apr 21, 2016
@cgundogan cgundogan added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 21, 2016
@cgundogan cgundogan force-pushed the pr/netstats_rpl branch 2 times, most recently from fbafa77 to b6c7133 Compare April 26, 2016 08:53
@BytesGalore BytesGalore added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed State: waiting for other PR State: The PR requires another PR to be merged first labels May 31, 2016
@BytesGalore

BytesGalore commented May 31, 2016

Copy link
Copy Markdown
Member

I guess its not waiting anymore.

BytesGalore
BytesGalore reviewed May 31, 2016
View reviewed changes
Comment thread sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c Outdated
#endif
/* a leaf node should only react to unicast DIS */
&& (gnrc_rpl_instances[i].dodag.node_status != GNRC_RPL_LEAF_NODE)) {
if(gnrc_rpl_instances[i].dodag.node_status != GNRC_RPL_LEAF_NODE) {

@BytesGalore BytesGalore May 31, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please add a whitespace

@BytesGalore

BytesGalore commented May 31, 2016

Copy link
Copy Markdown
Member

Don't know if it makes sense, but how about also counting errors, e.g. invalid DIOs/DAOs?

@cgundogan cgundogan force-pushed the pr/netstats_rpl branch 4 times, most recently from ff426cf to 7403fb4 Compare June 1, 2016 12:30
@cgundogan cgundogan removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 1, 2016
@cgundogan

cgundogan commented Jun 1, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

@BytesGalore rebased onto master and did a little overhaul. The statistics aren't bound to the instances anymore, because they would get lost otherwise when a node leaves the dodag.

Your idea about counting errors is also nice, but let's concentrate on this for another PR (: getting too big again.

You can test by compiling gnrc_networking and calling rpl stats from any node.

OlegHahm
OlegHahm reviewed Jun 1, 2016
View reviewed changes
Comment thread sys/include/net/netstats.h Outdated
uint32_t dao_ack_tx_ucast_bytes; /**< unicast dao_ack sent in bytes */
uint32_t dao_ack_tx_mcast_count; /**< multicast dao_ack sent in packets */
uint32_t dao_ack_tx_mcast_bytes; /**< multicast dao_ack sent in bytes*/
} netstats_rpl_t;

@OlegHahm OlegHahm Jun 1, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't it make maybe more sense to put this into some RPL specific header?

@BytesGalore BytesGalore Jun 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

@BytesGalore BytesGalore Jun 3, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would it make sense to introduce a counter type to easy switching smaller/larger nummeric types, e.g. uint16_t, like:

typedef uint16_t netstat_rpl_cnt_t;
typedef struct {
    /* DIO */
    netstat_rpl_cnt_t dio_rx_ucast_count;        /**< unicast dio received in packets */
...
}netstats_rpl_t;

?

@kYc0o

kYc0o commented Jul 22, 2016

Copy link
Copy Markdown
Contributor

@cgundogan do you think you can address the comments during the next week?

@kYc0o

kYc0o commented Jul 25, 2016

Copy link
Copy Markdown
Contributor

Well, let's see for the next release.

@kYc0o kYc0o modified the milestones: Release 2016.10, Release 2016.07 Jul 25, 2016
@cgundogan

cgundogan commented Aug 17, 2016

Copy link
Copy Markdown
Member Author

I moved the rpl related netstats struct into a separate file as suggested by @OlegHahm

@cgundogan

cgundogan commented Aug 17, 2016

Copy link
Copy Markdown
Member Author

Somehow the comment of @BytesGalore regarding a configurable struct member type got lost (commented on commit? (; ). But we can also do this as a follow-up, what do you think @BytesGalore ?

@BytesGalore

BytesGalore commented Aug 17, 2016

Copy link
Copy Markdown
Member

@cgundogan you mean 8f1d438#r65735118 :

typedef uint16_t netstat_rpl_cnt_t;
typedef struct {
    /* DIO */
    netstat_rpl_cnt_t dio_rx_ucast_count;        /**< unicast dio received in packets */
...
}netstats_rpl_t;

no objection to put/discuss it in a followup PR

@BytesGalore

BytesGalore commented Aug 30, 2016

Copy link
Copy Markdown
Member

I would say please squash then ACK from my side

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 23, 2016
@OlegHahm

OlegHahm commented Sep 23, 2016

Copy link
Copy Markdown
Member

@cgundogan, ping!

@BytesGalore

BytesGalore commented Sep 27, 2016

Copy link
Copy Markdown
Member

@cgundogan I still would say please squash then ACK from my side

@cgundogan cgundogan removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 27, 2016
@cgundogan

cgundogan commented Sep 27, 2016

Copy link
Copy Markdown
Member Author

@BytesGalore squashed

@cgundogan cgundogan added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 27, 2016
@miri64 miri64 merged commit f99bbbf into RIOT-OS:master Sep 27, 2016
@cgundogan cgundogan deleted the pr/netstats_rpl branch September 27, 2016 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@BytesGalore BytesGalore BytesGalore approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@BytesGalore BytesGalore

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 Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2016.10

Development

Successfully merging this pull request may close these issues.

5 participants

@cgundogan @BytesGalore @kYc0o @OlegHahm @miri64