Skip to content

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented Jul 1, 2025

Contribution description

The gnrc_rpl_recv_DIO function in gnrc_rpl_control_messages is > 150 LOC that are quite hard to understand, especially as someone that is new to RIOT.

This PR refactors the function into smaller sub-functions and extracts logic that is common between the two main branches (creating a new DODAG and updating an existing DODAG).

I split the refactoring into separate commits that are easier to review one-by-one; can squash them together at the end.

I am not super happy with the resulting code, but I think it is a small improvement.
Further feedback on how to improve it would be appreciated.

Open TODOs

  • Document new sub-functions?

Testing procedure

This PR shouldn't change any logic apart from the order in which individual DODAG properties are updated (see a8bc3ad).

I tested it manually:

  • examples/networking/gnrc_networking + added USEMODULE += nimble_rpble
  • 9 nodes (nrf52dk), 1 node A set as root (ifconfig 8 add 2001:db8:1::1/64 && rpl root 1 2001:db8:1::1)
  • => RPL tree was created as expected with A as root

Issues/PRs references

Preparation for future work on #21574.

@github-actions github-actions bot added Area: network Area: Networking Area: sys Area: System labels Jul 1, 2025
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2025
@riot-ci
Copy link

riot-ci commented Jul 1, 2025

Murdock results

✔️ PASSED

a461e5c gnrc/rpl/control_messages: doc gnrc_rpl_recv_DIO

Success Failures Total Runtime
10515 0 10516 12m:20s

Artifacts

@elenaf9 elenaf9 marked this pull request as ready for review July 2, 2025 16:42
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Thank you for taking up the task of breathing some new life into RPL! A working mesh protocol in RIOT would be highly appreciated.

@elenaf9
Copy link
Contributor Author

elenaf9 commented Aug 11, 2025

Friendly ping :)

I have a few other changes in RPL that I'd like to PR but they depend on this branch.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good to me - sorry for the delay.
I suppose you ran e.g. tests/net/gnrc_rpl (unfortunately we disabled that on CI 😕)

@elenaf9
Copy link
Contributor Author

elenaf9 commented Aug 26, 2025

Last commits just add docs, and fix one copy-paste error I noticed while self reviewing.

One more thing I noticed just now is that there is actually a minor logic change in this PR:

We now don't remove the whole RPL instance anymore if our preferred parent sent a DIO message with invalid options. Instead we just early return after the parent was updated (which doesn't change any dodag opts).
I could change this back by returning different error codes from _update_dodag_from_DIO and handling them appropriately, but I was wondering if we even want to remove the whole dodag if there is a (potentially one-time) error in the opts?


I suppose you ran e.g. tests/net/gnrc_rpl (unfortunately we disabled that on CI 😕)

Yes, I ran all RPL tests, and did some manual testing described in the PR description.


Edit: Should I squash now? (will fix the typos while squashing)

@elenaf9 elenaf9 force-pushed the gnrc/rpl/refactor-ctrl-messages branch from 7ca70d5 to 51f2647 Compare September 5, 2025 12:35
@github-actions github-actions bot added the Area: doc Area: Documentation label Sep 5, 2025
@elenaf9 elenaf9 force-pushed the gnrc/rpl/refactor-ctrl-messages branch from 51f2647 to 0508446 Compare September 5, 2025 12:36
@github-actions github-actions bot removed the Area: doc Area: Documentation label Sep 5, 2025
@elenaf9 elenaf9 force-pushed the gnrc/rpl/refactor-ctrl-messages branch from 0508446 to a461e5c Compare September 5, 2025 13:29
Extract commit logic from `_recv_DIO_for_{existing,new}_dodag` into new
function `_update_dodag_from_DIO`. The function updates the dodag and
parent based on the DIO data.
@benpicco benpicco enabled auto-merge September 5, 2025 13:35
@benpicco benpicco added this pull request to the merge queue Sep 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: sys Area: System 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
Development

Successfully merging this pull request may close these issues.

4 participants