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

Workaround for DAO having garbled content after RPL forwarding error #2545

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

arurke
Copy link
Member

@arurke arurke commented Jul 22, 2023

This PR

  1. Proposes a workaround to RPL issue RPL No-path DAO spurred by forwarding errors have garbled contents #2377
  2. Adds a test that showcases RPL No-path DAO spurred by forwarding errors have garbled contents #2377 The test for the issue was added separately in Test cases for RPL issues 2377 and 2285 #2548. This PR simply reverts the logic in the test such that it passes only if the issue is resolved.

In short, #2377 describes how No-Path DAOs that are sent when detecting RPL forwarding error will have garbled content and the receiver will discard it with error-log: icmpv6 bad checksum. The workaround adds a small delay (next tick) to the generation of the DAO, and this resolves the issue. I am no uIP/RPL expert and I have not identified the root cause as to why the content is garbled (see some ideas in #2377). As such, I will have no trouble if we don't merge this and rather leave it for others to work onward from.

For further details see test-description in #2548 and issue #2377.

@pjonsson
Copy link
Contributor

The test case is valuable to get into develop for tracking the behavior, so we'll know if this issue is accidentally fixed by something else.

Can you split out the test cases to a separate PR that can be merged? Just invert the logic for passing/failing testcase, and put a FIXME in a comment above that.

@arurke
Copy link
Member Author

arurke commented Jul 23, 2023

Can you split out the test cases to a separate PR that can be merged? Just invert the logic for passing/failing testcase, and put a FIXME in a comment above that.

🤦‍♂️ This is a great idea @pjonsson. Added #2548

@arurke arurke marked this pull request as draft July 23, 2023 14:18
@simonduq simonduq deleted the branch contiki-ng:develop August 8, 2023 14:27
@simonduq simonduq closed this Aug 8, 2023
@simonduq
Copy link
Member

simonduq commented Aug 8, 2023

Hi!

This was my mistake, very sorry about it, re-opening this PR now.

What happened is the following: I was switching the base branch for this repository https://github.com/wittra/contiki-ng from develop to wittra. But accidentally, I made the change on the wrong repo (this repo) and instead of switching the base branch I renamed it. And somehow github deleted develop and closed all PRs...

Many apologies for this mishap 🙏; I haven't contributed in a while.. but now at least everybody got some notification from me :p

@simonduq simonduq reopened this Aug 8, 2023
@arurke arurke force-pushed the workaround-for-dao-garbled-2377 branch from 14054a2 to 81ad651 Compare October 14, 2023 20:19
@arurke arurke force-pushed the workaround-for-dao-garbled-2377 branch from 81ad651 to 5b09642 Compare October 14, 2023 20:28
@arurke arurke marked this pull request as ready for review October 14, 2023 20:30
@arurke
Copy link
Member Author

arurke commented Oct 14, 2023

I have now rebased and removed the tests that were added via #2548. The logic in the tests have been reverted to verify this PR solves the issue.

As mentioned I believe there is more work to be done on the issue (ref. my explanation above of why calling this a workaround) - at the same time I believe this PR is conservative and is unlikely to do any harm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants