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: simplify hdr_build functions #5118

Merged
merged 7 commits into from
Mar 23, 2016
Merged

gnrc: simplify hdr_build functions #5118

merged 7 commits into from
Mar 23, 2016

Conversation

OlegHahm
Copy link
Member

Directly using 16 bit port numbers instead of casting uint8_t pointers.

@OlegHahm OlegHahm added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation GNRC labels Mar 20, 2016
@OlegHahm OlegHahm added this to the Release 2016.04 milestone Mar 20, 2016
@kaspar030
Copy link
Contributor

Wow, someone was planning ahead! ;) +1

@cgundogan
Copy link
Member

Just wondering, and maybe unrelated to this PR: do we want to support 8-bit port numbers in the future?

@miri64
Copy link
Member

miri64 commented Mar 21, 2016

Would gladly appreciate if this would be done for ipv6_addr_t gnrc_ipv6_hdr_build(), too ;-).

miri64
miri64 reviewed Mar 21, 2016
View reviewed changes
sys/net/gnrc/netreg/gnrc_netreg.c
{
uint16_t src_port = *((uint16_t *)src);
uint16_t dst_port = *((uint16_t *)dst);
return gnrc_udp_hdr_build(payload, src_port, dst_port);
Copy link
Member

Choose a reason for hiding this comment

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

Error check from https://github.com/RIOT-OS/RIOT/pull/5118/files#diff-a743a0dcf90d0eb32c837f1a047918c1L283 needs to be moved here now.

@OlegHahm
Copy link
Member Author

Just wondering, and maybe unrelated to this PR: do we want to support 8-bit port numbers in the future?

What do you mean by 8-bit port numbers?

@OlegHahm
Copy link
Member Author

Would gladly appreciate if this would be done for ipv6_addr_t gnrc_ipv6_hdr_build(), too ;-).

Will do.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 21, 2016
@OlegHahm
Copy link
Member Author

updated

miri64
miri64 reviewed Mar 21, 2016
View reviewed changes
sys/include/net/gnrc/ipv6/hdr.h
uint8_t *src, uint8_t src_len,
uint8_t *dst, uint8_t dst_len);
gnrc_pktsnip_t *gnrc_ipv6_hdr_build(gnrc_pktsnip_t *payload, uint8_t *src,
uint8_t *dst);
Copy link
Member

Choose a reason for hiding this comment

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

Why not use ipv6_addr_t pointers?

Copy link
Member Author

Choose a reason for hiding this comment

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

true, will update

@cgundogan
Copy link
Member

What do you mean by 8-bit port numbers?

I mean that 16-bit port numbers may be too much for certain IoT use cases. Do we plan to make that configurable (by compile time e.g.) to reduce that to 8-bit instead somewhen in the future?

@kaspar030
Copy link
Contributor

What do you mean by 8-bit port numbers?

I mean that 16-bit port numbers may be too much for certain IoT use cases. Do we plan to make that configurable (by compile time e.g.) to reduce that to 8-bit instead somewhen in the future?

I don't think we should make that part of the protocol compile-time configurable...
Doesn't 6lowpan define a set of "short" UDP ports, but they still start at a value higher than 256?

@OlegHahm
Copy link
Member Author

NHC allows to compress a certain range of UDP ports. Compressing them internally seems to be overkill to me.

@cgundogan
Copy link
Member

IPv6 UDP NHC can compress the ports to 4-bits each if they are in the range of 0xf0b0 to 0xf0bf. But currently all our APIs will use 16-bits for each port (src,dst). I am just wondering if it might make sense to make our (future) APIs configurable for that

@OlegHahm
Copy link
Member Author

I don't think it makes sense to use compressed representation in the API.

@miri64
Copy link
Member

miri64 commented Mar 21, 2016

@cgundogan that's out-of-scope for IPv6 and purely a 6LoWPAN thing. I would keep it there and not put 6LoWPAN specific stuff in our UDP implementation

@OlegHahm
Copy link
Member Author

Addressed @authmillenon's comment.

@OlegHahm
Copy link
Member Author

Thanks for the review - I think this PR now simplifies the code a bit.

@miri64
Copy link
Member

miri64 commented Mar 21, 2016

Please squash (and change title of PR appropriately ;-))

@OlegHahm OlegHahm force-pushed the udp_build_hdr_simplification branch from 72c7833 to c4131f0 Compare March 21, 2016 16:59
@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 21, 2016
@OlegHahm OlegHahm changed the title gnrc udp: simplify gnrc_udp_hdr_build gnrc: simplify hdr_build functions Mar 21, 2016
@OlegHahm
Copy link
Member Author

done

miri64
miri64 reviewed Mar 21, 2016
View reviewed changes
sys/net/gnrc/netreg/gnrc_netreg.c
@@ -143,17 +144,31 @@ gnrc_pktsnip_t *gnrc_netreg_hdr_build(gnrc_nettype_t type, gnrc_pktsnip_t *paylo
#ifdef MODULE_GNRC_IPV6

case GNRC_NETTYPE_IPV6:
return gnrc_ipv6_hdr_build(payload, src, src_len, dst, dst_len);
assert(src_len == sizeof(ipv6_addr_t));
assert(dst_len == sizeof(ipv6_addr_t));
Copy link
Member

Choose a reason for hiding this comment

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

Please document these preconditions in the doxygen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, wondering about how to document this, I begin to wonder if we need this function at all. What is the purpose of this function?

@miri64
Copy link
Member

miri64 commented Mar 21, 2016

One (nitpicky) remark. Otherwise I give my ACK when Murdock is happy.

@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 Mar 21, 2016
@miri64
Copy link
Member

miri64 commented Mar 21, 2016

(you may squash the doc fix immediately).

@OlegHahm
Copy link
Member Author

CI was failing because of missing const specifiers. Unsure if I should squash this last commit.

@OlegHahm
Copy link
Member Author

I couldn't think of a reason where this generic header build function would be needed, hence I removed it.

@miri64
Copy link
Member

miri64 commented Mar 22, 2016

I think the original idea was to be layer agnostic, but that proved to be impossible at least for TCP/IP. Any objections @haukepetersen?

@miri64
Copy link
Member

miri64 commented Mar 23, 2016

Please rebase.

@OlegHahm OlegHahm force-pushed the udp_build_hdr_simplification branch from 0dbb94c to 1a9a8e3 Compare March 23, 2016 13:41
@OlegHahm
Copy link
Member Author

rebased to master

@miri64
Copy link
Member

miri64 commented Mar 23, 2016

ACK and go.

@miri64
Copy link
Member

miri64 commented Mar 23, 2016

(when Murdock is happy).

@OlegHahm OlegHahm force-pushed the udp_build_hdr_simplification branch from 1a9a8e3 to a137b99 Compare March 23, 2016 14:11
@OlegHahm
Copy link
Member Author

Fixed a typo in zep that Murdock reported and squashed immediately.

@OlegHahm OlegHahm force-pushed the udp_build_hdr_simplification branch from a137b99 to cba3ba7 Compare March 23, 2016 14:20
@OlegHahm
Copy link
Member Author

args, fixed the wrong parameter - another try.

@OlegHahm OlegHahm merged commit a65e6aa into RIOT-OS:master Mar 23, 2016
@OlegHahm OlegHahm deleted the udp_build_hdr_simplification branch March 23, 2016 14:32
@miri64 miri64 mentioned this pull request Oct 18, 2016
Closed
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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2016.04
Development

Successfully merging this pull request may close these issues.
4 participants
@OlegHahm @kaspar030 @cgundogan @miri64