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: omit dodag id in DAO(-ACK) for global instances #3659

Merged
merged 2 commits into from
Aug 20, 2015
Merged

rpl: omit dodag id in DAO(-ACK) for global instances #3659

merged 2 commits into from
Aug 20, 2015

Conversation

cgundogan
Copy link
Member

This PR introduces minor support for global / local instance ids.
If an instance id 0 <= id <= 127 is specified, the instance is global and thus the dodag_id will be omitted in outgoing DAOs and DAO-ACKs and not expected respectively on the receiving side. (see #3050 (comment))

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 18, 2015
@jnohlgard
Copy link
Member

needs rebase :/

jnohlgard
jnohlgard reviewed Aug 19, 2015
View reviewed changes
sys/net/routing/ng_rpl/ng_rpl_control_messages.c
/* set the K flag to indicate that a ACKs are required */
dao->k_d_flags = ((1 << 6) | (1 << 7));
dao->k_d_flags |= (1 << 7);
Copy link
Member

Choose a reason for hiding this comment

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

in the case of !local_instance the k_d_flags member is never zeroed out.

Copy link
Member

Choose a reason for hiding this comment

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

please replace (1 << 7) with a new constant macro with a describing name

@jnohlgard
Copy link
Member

Wow! 👏

This is one more step towards Contiki interoperability

@cgundogan
Copy link
Member Author

@gebart, addressed your comments and did a rebase. I will put the length checks in a separate PR however, thanks again for the reminder.

@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 19, 2015
@jnohlgard jnohlgard mentioned this pull request Aug 19, 2015
Open
9 tasks
@jnohlgard jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 19, 2015
@jnohlgard
Copy link
Member

OK to squash.
Untested ACK. I won't have the possibility to test this PR during this week, anyone else feel like it?

@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 Aug 19, 2015
@OlegHahm OlegHahm assigned OlegHahm and unassigned jnohlgard Aug 19, 2015
@OlegHahm
Copy link
Member

I can test tomorrow. Please send me a reminder.

@cgundogan
Copy link
Member Author

squashed. anybody can test this by starting rpl with an instance id in the range of 1 <= id <= 127 and look at the DAOs in wireshark. Then do the same with a rpl instance with id >= 128 and look again at the DAOs. In one case there should be the DODAG-ID present, for the other case not.

@cgundogan cgundogan mentioned this pull request Aug 19, 2015
Merged
OlegHahm
OlegHahm reviewed Aug 19, 2015
View reviewed changes
sys/net/gnrc/routing/rpl/gnrc_rpl_control_messages.c
@@ -28,17 +28,20 @@
#define ENABLE_DEBUG (0)
#include "debug.h"

#if ENABLE_DEBUG && defined(MODULE_IPV6_ADDR)
Copy link
Member

Choose a reason for hiding this comment

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

How is this related to the PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

unrelated, but also unnecessary as rpl does not build without MODULE_IPV6_ADDR. Do you want me to open a separate PR for this change?

Copy link
Member

Choose a reason for hiding this comment

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

No, but a separate commit would be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

this comment has been addressed

@cgundogan cgundogan mentioned this pull request Aug 19, 2015
Closed
@cgundogan
Copy link
Member Author

addressed @OlegHahm's comments (squashed)

@cgundogan
Copy link
Member Author

rebased to curr. master

@cgundogan
Copy link
Member Author

I can test tomorrow. Please send me a reminder.

Friendly reminder, @OlegHahm

@OlegHahm
Copy link
Member

If an instance id 0 <= id <= 127

1440077466.212608;m3-9;> rpl root 0 affe::1
1440077466.213585;m3-9;<instance_id> must be a positive number greater than zero

@OlegHahm
Copy link
Member

Seems not to break anything and Wireshark shows what is described. ACK

OlegHahm added a commit that referenced this pull request Aug 20, 2015
@OlegHahm
Merge pull request #3659 from cgundogan/pr/rpl/dao_global_instance
1f7da4e 
rpl: omit dodag id in DAO(-ACK) for global instances
@OlegHahm OlegHahm merged commit 1f7da4e into RIOT-OS:master Aug 20, 2015
@cgundogan cgundogan deleted the pr/rpl/dao_global_instance branch August 20, 2015 16:19
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
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 Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

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