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

rpl: remove unnecessary check for multiple instances #2606

Merged

Conversation

cgundogan
Copy link
Member

If another instance id is received, the current implementation returns, because it does not support multiple instances. The check however is unnecessary in this case and will be removed anyway when I introduce multiple instances support.
Putting this in smaller PRs will reduce complexity.

@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
@BytesGalore BytesGalore mentioned this pull request Mar 17, 2015
21 tasks
@@ -539,19 +532,6 @@ void rpl_recv_DIO(void)
return;
}
}
else if (my_inst == NULL) {
DEBUGF("Not joined an instance yet\n");
}
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this debug line useful anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see the importance of this debug message. If the rpl implementation did not join any instance yet, it will do it now with the information specified in this incoming DIO

Copy link
Member

Choose a reason for hiding this comment

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

I thought it could be good to keep an overview if this node will join the dodag because it hasn't joined any dodag or because it prefers the dodag its DIO it is currently receiving.

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 information cannot be retrieved here, because this part of the code handles the instances and not the dodags. The implementation will create any instance it encounters, if it did exist before, it will skip the creation process. This is all to this section of the code, therefore it was kind of too complex before..

@OlegHahm
Copy link
Member

I don't get why this check is unnecessary in the first place.

@cgundogan
Copy link
Member Author

The check is too complicated and does not allow more then one instance. With the proposed changes it will check, if the implementation joined the current DIO's instance. If not, it will create this instance and join the dodag (later in the code). Otherwise, it already joined the isntance and continues.

@OlegHahm
Copy link
Member

But isn't this dependent on another patch then?

@cgundogan
Copy link
Member Author

not really dependent.. there is in the momentan no code which utilizes multiple instance ids. So it will not hurt to remove the restrictions that block the creation of other than the first instance. And even if there were two different instance ids floating around, the current implemenation does not take this into account, because it uses rpl_get_my_dodag() throughout the code.

@BytesGalore
Copy link
Member

I think if the node uses/takes advantage of more than one RPL instance the current check can be adopted to determine if the node is already using it.
@OlegHahm the check you mentioned for determine if the node joined due to a more preferable DODAG could be eventually done here [1].

[1] https://github.com/RIOT-OS/RIOT/blob/master/sys/net/routing/rpl/rpl_control_messages.c#L669

@OlegHahm OlegHahm force-pushed the master branch 3 times, most recently from 9f184dd to 45554bf Compare March 31, 2015 13:01
@OlegHahm
Copy link
Member

OlegHahm commented Apr 9, 2015

So, what will happen if there's another instance ID and the node calls the join function?

@BytesGalore
Copy link
Member

@OlegHahm this won't happen because the node can only be in one RPL instance

@OlegHahm
Copy link
Member

OlegHahm commented Apr 9, 2015

Ah, because it is checked later in this function, right?

@BytesGalore
Copy link
Member

Ok, strike my comment I just starred on wrong codelines.
When there is a new instance ID, the node will join the DODAG, e.g here [1], which will create a new DODAG here [2]. (if I'm not starred on wrong lines again 👓 )

[1] https://github.com/cgundogan/RIOT/blob/rpl_remove_multiple_instance_check/sys/net/routing/rpl/rpl_control_messages.c#L672
[2] https://github.com/RIOT-OS/RIOT/blob/master/sys/net/routing/rpl/rpl_dodag.c#L90

@OlegHahm
Copy link
Member

Ok, I think I finally understood this. ACK and go.

OlegHahm added a commit that referenced this pull request Apr 22, 2015
…check

rpl: remove unnecessary check for multiple instances
@OlegHahm OlegHahm merged commit 126eba7 into RIOT-OS:master Apr 22, 2015
@cgundogan cgundogan deleted the rpl_remove_multiple_instance_check branch July 31, 2015 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants