Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ng_sixlowpan: initial import #2614

Merged
merged 2 commits into from
Apr 23, 2015
Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 16, 2015

Depends on #2433 and all its dependencies. (merged)

This only implements handling and sending of uncompressed, unfragmented IPv6 packets. The rest will come as a follow-up:

@miri64 miri64 added Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: waiting for other PR State: The PR requires another PR to be merged first NSTF labels Mar 16, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone Mar 16, 2015
@miri64 miri64 mentioned this pull request Mar 16, 2015
36 tasks
@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch 2 times, most recently from 6ce4e84 to dc7214c Compare March 19, 2015 15:00
@miri64
Copy link
Member Author

miri64 commented Mar 19, 2015

Rebased to current #2555

@miri64
Copy link
Member Author

miri64 commented Mar 25, 2015

Rebased to current #2555

@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch from 7fd337b to 33f0a83 Compare March 30, 2015 01:00
@miri64
Copy link
Member Author

miri64 commented Mar 30, 2015

Rebased to current #2555

@OlegHahm OlegHahm force-pushed the master branch 3 times, most recently from 9f184dd to 45554bf Compare March 31, 2015 13:01
@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch from 33f0a83 to e4f8f30 Compare April 1, 2015 16:07
@miri64
Copy link
Member Author

miri64 commented Apr 1, 2015

Rebased to current #2555

@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch from e4f8f30 to dd4e87f Compare April 7, 2015 13:39
@miri64
Copy link
Member Author

miri64 commented Apr 7, 2015

Rebased to current #2555

@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch from dd4e87f to 77395f0 Compare April 7, 2015 17:55
@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch from 77395f0 to 2a3ad06 Compare April 8, 2015 10:25
@miri64
Copy link
Member Author

miri64 commented Apr 8, 2015

Rebased to current #2555

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 22, 2015
ifneq (,$(filter ng_sixlowpan,$(USEMODULE)))
USEMODULE += ng_sixlowpan_netif
USEMODULE += ng_netbase
endif
Copy link
Member

Choose a reason for hiding this comment

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

Conceptional: Shouldn't be IPv6 also a dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Conceptionally, yes. To work: no. If we ever want to introduce stand-alone tests for every layer we would not need IPv6 here.

@OlegHahm
Copy link
Member

Sorry for my somewhat nit-picky style comments. I just note down whatever I find, but it's up to you if you want to address it directly or postpone it to a later PR. Just let me know.


((ng_netif_hdr_t *)pkt->data)->if_pid = iface;

if ((if_entry != NULL) && (if_entry->flags & NG_IPV6_NETIF_FLAGS_SIXLOWPAN)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we not exit early if if_entry is NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no exit on if_entry == NULL, it is just send to iface (which is assumed to be a valid PID at this point).

Copy link
Member

Choose a reason for hiding this comment

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

I thought NULL indicates that the PID is probably not valid.

Copy link
Member

Choose a reason for hiding this comment

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

Can you still explain this? What does NULL as a return value for ng_ipv6_netif_get(iface); indicate here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That there is no IPv6 interface. This is not a case that should happen here so maybe we can even remove the NULL test

@miri64
Copy link
Member Author

miri64 commented Apr 23, 2015

Addressed comments.

@OlegHahm
Copy link
Member

Everything's addressed. The rest looks good. ACK. Please squash.

@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 Apr 23, 2015
@miri64 miri64 force-pushed the ng_sixlowpan/feat/initial branch from d4cc6e9 to 5412e0e Compare April 23, 2015 09:30
@miri64 miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Apr 23, 2015
@miri64
Copy link
Member Author

miri64 commented Apr 23, 2015

Done.

@miri64
Copy link
Member Author

miri64 commented Apr 23, 2015

Can't restart the failed job. So merge.

miri64 added a commit that referenced this pull request Apr 23, 2015
@miri64 miri64 merged commit 1d74465 into RIOT-OS:master Apr 23, 2015
@miri64 miri64 deleted the ng_sixlowpan/feat/initial branch April 23, 2015 14:17
ng_sixlowpan_netif_t *iface;
/* cppcheck: datagram_size will be read by frag */
/* cppcheck-suppress unreadVariable */
size_t payload_len, datagram_size;
Copy link
Member

Choose a reason for hiding this comment

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

The suppression doesn't seem to work for me I got -Wunused-but-set-variable warning for datagram_size here, anyone else?

gcc: gcc version 4.9.3 20141119 (release) [ARM/embedded-4_9-branch revision 218278] (GNU Tools for ARM Embedded Processors)

Copy link
Member

Choose a reason for hiding this comment

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

It's a suppression for cppchech, not for gcc.

Copy link
Member

Choose a reason for hiding this comment

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

ahh, my bad thx

Copy link
Member

Choose a reason for hiding this comment

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

The correct way to suppress the GCC warning is to cast to void, at least for function arguments, I guess it should work for local variables as well.

    (void) datagram_size;

edit: However, I do not understand what the purpose of the variable is when it is only set and never used, even as by reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gebart I know that. The variable is supposed to be used in #2781 and I did not introduce Toi many lines I would revert there anyways.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants