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

gnrc_ipv6_ext_frag: Initial import of IPv6 fragmentation #11623

Merged
merged 3 commits into from
Sep 17, 2019
Merged

gnrc_ipv6_ext_frag: Initial import of IPv6 fragmentation #11623

merged 3 commits into from
Sep 17, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jun 4, 2019 •
edited
Loading

Contribution description

This provides IPv6 fragmentation for GNRC. Reassembly was introduced in #11596 but that PR is independent from this one (though it touches similar files).

Testing procedure

Will provide tests, but

BOARD=native USEMODULE=gnrc_ipv6_ext_frag make -C tests/gnrc_udp all term

and sending large (> 1500 bytes) UDP packets to the host's TAP bridge works (ping does not work as reassembly is provided in a separate PR: #11596).

Issues/PRs references

Addresses #5371 in part.

Depends on #11593 and #11622.

Touches similar files as #11596 but can be merged independent.

@miri64 miri64 added Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 4, 2019
@miri64 miri64 mentioned this pull request Jun 4, 2019
Merged
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 4, 2019
@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-frag branch from 3efab1a to cb5861f Compare June 4, 2019 15:16
@miri64
Copy link
Member Author

miri64 commented Jun 4, 2019

Rebased to current master. This PR now doesn't have any dependencies anymore (but still remains WIP)

@miri64 miri64 added this to the Release 2019.07 milestone Jun 4, 2019
@miri64 miri64 mentioned this pull request Jun 4, 2019
Closed
@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-frag branch from 9cdeccd to d394359 Compare June 4, 2019 18:00
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Jun 4, 2019
@miri64
Copy link
Member Author

miri64 commented Jun 4, 2019

There is one test I'd like to add (test if forwarding a large packet does not cause refragmentation), but that can also be done as a follow-up if time presses => removing WIP.

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-frag branch 3 times, most recently from 3051fd9 to 3d54070 Compare June 4, 2019 21:04
@MrKevinWeiss
Copy link
Contributor

release status ping, should I change the milestone?

@miri64
Copy link
Member Author

miri64 commented Jun 13, 2019

Isn't the (soft) feature freeze still a few weeks ahead?

@MrKevinWeiss
Copy link
Contributor

Yup, the 28th. Just a ping for those who mentioned they wanted this in.

@MrKevinWeiss
Copy link
Contributor

@cgundogan ping 👿

@miri64
Copy link
Member Author

miri64 commented Jun 25, 2019

Since I still want to implement a test case for this, I'd prefer to have reassembly (#11596) be reviewed and merged first. It also makes more sense IMHO to first support the receiving end.

@miri64
Copy link
Member Author

miri64 commented Jul 24, 2019

Since I still want to implement a test case for this, I'd prefer to have reassembly (#11596) be reviewed and merged first. It also makes more sense IMHO to first support the receiving end.

This is now finally done.

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-frag branch from 4ee0591 to 7e0024e Compare July 25, 2019 08:23
@miri64
Copy link
Member Author

miri64 commented Jul 25, 2019

Rebased to current master to adapt for #11668.

@benpicco
Copy link
Contributor

When you rebase please just squash those fixups directly.
I haven't seen the state before the fixup, so they just clutter reviewing.

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-frag branch from 23722e3 to f6d51b8 Compare September 16, 2019 20:04
@miri64
Copy link
Member Author

miri64 commented Sep 16, 2019

Rebased and squashed (I hope I disentangled this knot correctly... the tests are passing on native) ;-)

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 16, 2019
benpicco
benpicco reviewed Sep 16, 2019
View reviewed changes
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.

At a first glance this looks good too - I would enjoy more documentation though.
I don't know much about IPv6 fragmentation, so maybe by the time I understand it, it's well documented 😉

@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Addressed review comments

@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Adapted tests, as ethos is perfectly fine sending huge fragmented packets, but still looses packets when receiving.

@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Added boards to BOARD_INSUFFICIENT_MEMORY and BOARD_BLACKLIST required to the enlarged test and the (re-)inclusion of ethos into the test.

@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Murdock is only complaining about fixup commits (and their summary lengths).

benpicco
benpicco approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think I see what's going on now - but nothing more to comment on. Please squash.

Tests run on native and ek-lm4f120xl with ethos. I would like to try this with real 802.15.4 hardware, but I'm not in the office again before next week and don't really expect any surprises other than driver bugs.
Reassembly is the more critical path anyway.

If the module is not used, nothing changes, so no potential for regressions.

Can you please create an issue for the ethos bug, so we don't lose track of that? If you do, reference it in a comment for the unit test also, just squash directly.

@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Squashed

@miri64 miri64 force-pushed the gnrc_ipv6_ext/feat/ipv6-frag branch from 48463c2 to e62bb9c Compare September 17, 2019 16:55
@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Can you please create an issue for the ethos bug, so we don't lose track of that? If you do, reference it in a comment for the unit test also,

Will do in a moment ;-)

@miri64 miri64 merged commit 5631b69 into RIOT-OS:master Sep 17, 2019
@miri64 miri64 deleted the gnrc_ipv6_ext/feat/ipv6-frag branch September 17, 2019 18:10
@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Thanks for the reviews. The requested issue you can find at #12264.

@benpicco
Copy link
Contributor

Good to have this feature in one piece for the 2019.10 release!

@miri64
Copy link
Member Author

miri64 commented Sep 17, 2019

Most definitely! :-)

@miri64 miri64 added this to the Release 2019.10 milestone Sep 17, 2019
@miri64 miri64 mentioned this pull request Oct 4, 2019
Closed
@miri64 miri64 mentioned this pull request Nov 29, 2019
Closed
@miri64 miri64 mentioned this pull request Jan 31, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@jia200x jia200x Awaiting requested review from jia200x

@cgundogan cgundogan Awaiting requested review from cgundogan

@smlng smlng Awaiting requested review from smlng

@bergzand bergzand Awaiting requested review from bergzand

Assignees
No one assigned
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.
3 participants
@miri64 @MrKevinWeiss @benpicco