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.

Sign up for GitHub

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

Jump to bottom

rpl: update routing table information for all dodags #2607

Merged
merged 1 commit into from
Apr 14, 2015
Merged

rpl: update routing table information for all dodags #2607

merged 1 commit into from
Apr 14, 2015

Conversation

cgundogan
Copy link
Member

In the current implementation only the default dodag's routing table information is updated periodically. This PR extends this to iterate over all used dodags instead.

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Mar 16, 2015
@cgundogan cgundogan added Area: network Area: Networking and removed Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Mar 16, 2015
@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from 88f760a to 541352a Compare March 17, 2015 10:41
@BytesGalore BytesGalore mentioned this pull request Mar 17, 2015
Closed
21 tasks
@OlegHahm OlegHahm added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 18, 2015
@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from 541352a to 4f213f6 Compare March 19, 2015 10:46
@cgundogan
Copy link
Member Author

rebased to address @OlegHahm's comment mentioned #2622

@OlegHahm OlegHahm removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 19, 2015
@OlegHahm
Copy link
Member

needs rebasing

@cgundogan
Copy link
Member Author

@OlegHahm according to github this PR can be automatically merged 😕

@OlegHahm
Copy link
Member

But cgundogan@d7722c1 which is part of this PR got already merged, didn't it?

@cgundogan
Copy link
Member Author

ah, you are right. will rebase :bowtie:

@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from 4f213f6 to dbe86b7 Compare March 19, 2015 18:02
@cgundogan
Copy link
Member Author

rebased to current master

@cgundogan
Copy link
Member Author

@OlegHahm any objection?

BytesGalore
BytesGalore reviewed Mar 30, 2015
View reviewed changes
sys/net/routing/rpl/rpl.c
}
else {
rt[i].lifetime = rt[i].lifetime - RPL_LIFETIME_STEP;
for (uint8_t i = 0; i < rpl_max_routing_entries; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be outside the DODAG iteration

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, I will update and rebase this PR

@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from dbe86b7 to 95a28f9 Compare March 31, 2015 08:13
@cgundogan
Copy link
Member Author

addressed @BytesGalore comment. Rebased

@BytesGalore BytesGalore mentioned this pull request Mar 31, 2015
Merged
@OlegHahm OlegHahm force-pushed the master branch 2 times, most recently from 9f184dd to 45554bf Compare March 31, 2015 13:01
@cgundogan
Copy link
Member Author

ping @OlegHahm @BytesGalore @gebart . any ACKs? The change is small but it's important to handle the lifetimes of multiple dodags. Otherwise only the dodag's lifetime gets decreased, which is returned by rpl_get_my_dodag(). I want to eliminate the dependency to this function (rpl_get_my_dodag()) in the long(actually short) run.

BytesGalore
BytesGalore reviewed Apr 7, 2015
View reviewed changes
sys/net/routing/rpl/rpl.c
if (rt[i].lifetime <= 1) {
memset(&rt[i], 0, sizeof(rt[i]));
for (my_dodag = rpl_dodags, end = my_dodag + RPL_MAX_DODAGS; my_dodag < end; my_dodag++) {
if (my_dodag->used) {
Copy link
Member

Choose a reason for hiding this comment

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

Just for my curiosity, is this check needed?
When the node joins a DODAG the value is set and never touched again (if I'm not missed something).
I can't find any occurrences where the ->used entry of a DODAG is set to 0.

update: I know that this value is initialized with 0 but maybe we can find a way to rip this member entry

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we do not really support a proper way of leaving a dodag. But this will change with subsequent PRs. The idea of this check is to ignore all dodags, which are stored in memory (as left-overs) and only handle the active ones.

@BytesGalore
Copy link
Member

From the code side it looks good, and works for at least one DODAG (I assume it will also work for multiple DODAGS) 👍, so ACK from my side

OlegHahm
OlegHahm reviewed Apr 9, 2015
View reviewed changes
sys/net/routing/rpl/rpl.c

if (my_dodag != NULL) {
rt = rpl_get_routing_table();
for (uint8_t i = 0; i < rpl_max_routing_entries; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

You could change the uint8_t to unsigned int while you're at it.

@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from 95a28f9 to ee1dd43 Compare April 13, 2015 16:15
@cgundogan
Copy link
Member Author

rebased and addressed @OlegHahm's comment

@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 Apr 13, 2015
OlegHahm
OlegHahm reviewed Apr 13, 2015
View reviewed changes
sys/net/routing/rpl/rpl.c
for (my_dodag = rpl_dodags, end = my_dodag + RPL_MAX_DODAGS; my_dodag < end; my_dodag++) {
if (my_dodag->used) {
/* Parent is NULL for root too */
if (my_dodag->my_preferred_parent != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

if ((my_dodag->used) && (my_dodag->my_preferred_parent != NULL)) {

saves a level of indentation.

@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from 26c0272 to 5557778 Compare April 13, 2015 16:47
@cgundogan
Copy link
Member Author

addressed @OlegHahm's comment and combined both if-statements.
I also took the liberty of removing the meaningless comment "/* Parent is NULL for root too */"

@OlegHahm
Copy link
Member

ACK. Please squash.

@cgundogan cgundogan force-pushed the rpl_update_rt_table_for_all_dodags branch from 5557778 to d27cd45 Compare April 13, 2015 16:59
@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 Apr 13, 2015
@cgundogan
Copy link
Member Author

squashed

@BytesGalore
Copy link
Member

My ACK holds, when travis is happy merge at will

@cgundogan
Copy link
Member Author

travis is happy => GO

cgundogan added a commit that referenced this pull request Apr 14, 2015
@cgundogan
Merge pull request #2607 from cgundogan/rpl_update_rt_table_for_all_d…
ac5e9af 
…odags

rpl: update routing table information for all dodags
@cgundogan cgundogan merged commit ac5e9af into RIOT-OS:master Apr 14, 2015
@cgundogan cgundogan deleted the rpl_update_rt_table_for_all_dodags branch April 14, 2015 09:11
@cgundogan cgundogan mentioned this pull request Apr 14, 2015
Closed
@OlegHahm OlegHahm modified the milestone: Release 2015.06 Apr 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@OlegHahm OlegHahm

Labels
Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2015.09
Development

Successfully merging this pull request may close these issues.
3 participants
@cgundogan @OlegHahm @BytesGalore