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

sixlowpan: iphc: fix dependencies for udp and nhc #4540

Merged
merged 1 commit into from
Dec 23, 2015
Merged

sixlowpan: iphc: fix dependencies for udp and nhc #4540

merged 1 commit into from
Dec 23, 2015

Conversation

cgundogan
Copy link
Member

This PR tries to fix the dependency issue that we have with udp <-> nhc <-> border routers (#4462 (comment)).

Not sure if I like this solution, nevertheless, I put this on the table for review.

@authmillenon @jfischer-phytec-iot please review

@cgundogan cgundogan added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Dec 22, 2015
@cgundogan cgundogan added this to the Release 2015.12 milestone Dec 22, 2015
@cgundogan cgundogan added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Dec 22, 2015
miri64
miri64 reviewed Dec 22, 2015
View reviewed changes
sys/include/net/gnrc/nettype.h
@@ -77,7 +77,8 @@ typedef enum {
#ifdef MODULE_GNRC_TCP
GNRC_NETTYPE_TCP, /**< Protocol is TCP */
#endif
#ifdef MODULE_GNRC_UDP
#if defined(MODULE_GNRC_UDP) || \
(defined(MODULE_GNRC_SIXLOWPAN_IPHC_NHC) && defined(MODULE_GNRC_SIXLOWPAN_ND_BORDER_ROUTER))
Copy link
Member

Choose a reason for hiding this comment

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

Make it just MODULE_GNRC_SIXLOWPAN_IPHC_NHC. There is no need for the relation to gnrc_udp since no functions from that module is used. If we want to make UDP support for NHC optional we should introduce a pseudo module gnrc_sixlowpan_iphc_nhc_udp

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 have to test here for udp || iphc_nhc, for scenarios where we want to use udp but not iphc_nhc (this is nettype.h)

Copy link
Member

Choose a reason for hiding this comment

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

Make set to UNDEF if UDP is not available ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Make set to UNDEF if UDP is not available ;)

I do not understand - it's too late for my brain. Could you elaborate?

Copy link
Member

@miri64 miri64 Dec 22, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

See cgundogan#9

@cgundogan cgundogan force-pushed the pr/sixlowpan/udp_dependencies branch from ffe430e to 7f57951 Compare December 23, 2015 09:23
@cgundogan
Copy link
Member Author

addressed comments

@cgundogan cgundogan added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 23, 2015
miri64
miri64 reviewed Dec 23, 2015
View reviewed changes
sys/include/net/gnrc/nettype.h
@@ -77,7 +77,7 @@ typedef enum {
#ifdef MODULE_GNRC_TCP
GNRC_NETTYPE_TCP, /**< Protocol is TCP */
#endif
#ifdef MODULE_GNRC_UDP
#if defined(MODULE_GNRC_UDP) || defined(MODULE_GNRC_SIXLOWPAN_IPHC_NHC)
Copy link
Member

Choose a reason for hiding this comment

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

Mh, that could have implications to netreg size

Copy link
Member

Choose a reason for hiding this comment

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

(see cgundogan#9)

@cgundogan
Copy link
Member Author

merged cgundogan#9 as proposed by @authmillenon

@miri64
Copy link
Member

miri64 commented Dec 23, 2015

Please squash down to one commit. Will try to test as soon as I have a border router scenario running.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 23, 2015
@cgundogan cgundogan force-pushed the pr/sixlowpan/udp_dependencies branch from 32e8817 to 7d62851 Compare December 23, 2015 12:58
@cgundogan
Copy link
Member Author

squashed

@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 Dec 23, 2015
@miri64
Copy link
Member

miri64 commented Dec 23, 2015

As soon as this is backported I make a new RC

@cgundogan
Copy link
Member Author

Please squash down to one commit. Will try to test as soon as I have a border router scenario running.

As soon as this is backported I make a new RC

I will create the backport as soon as you ACK

@miri64
Copy link
Member

miri64 commented Dec 23, 2015

I can't test, because for some reason fib does not resolve default route on my machine :(. I'll do an untested ACK and go, since the fix seems alright.

miri64 added a commit that referenced this pull request Dec 23, 2015
@miri64
Merge pull request #4540 from cgundogan/pr/sixlowpan/udp_dependencies
3692334 
sixlowpan: iphc: fix dependencies for udp and nhc
@miri64 miri64 merged commit 3692334 into RIOT-OS:master Dec 23, 2015
@cgundogan cgundogan deleted the pr/sixlowpan/udp_dependencies branch December 23, 2015 14:42
@cgundogan cgundogan mentioned this pull request Dec 23, 2015
Merged
@cgundogan cgundogan removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Dec 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2015.12
Development

Successfully merging this pull request may close these issues.
2 participants
@cgundogan @miri64