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

Decouple network stack from transceiver #644

Merged
merged 19 commits into from
Mar 1, 2014

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Feb 7, 2014

Follow-Up to #460

Depends on #460 and #557.

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2014

Not tested functionality yet, but it builds on native.

@OlegHahm
Copy link
Member

OlegHahm commented Feb 7, 2014

Needs rebase already.

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2014

Yes, because of #648 I guess ;-)

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2014

Done + Rebased all depending PRs (#460 and #557).

@miri64 miri64 mentioned this pull request Feb 7, 2014
@@ -100,10 +103,10 @@ void rpl_udp_init(char *str)
ipv6_addr_init(&std_addr, 0xABCD, 0xEF12, 0, 0, 0x1034, 0x00FF, 0xFE00, id);
ipv6_addr_init_prefix(&prefix, &std_addr, 64);
plist_add(&prefix, 64, NDP_OPT_PI_VLIFETIME_INFINITE, 0, 1, ICMPV6_NDP_OPT_PI_FLAG_AUTONOM);
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be replaced by ndp_add_prefix_info() (?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorry tested the last change only on the 6LoWPAN plugtests

@miri64
Copy link
Member Author

miri64 commented Feb 7, 2014

For those wondering why I removed the ip command from the applications: ifconfig [<interface>] from net_if shell commands does this + more info

@OlegHahm
Copy link
Member

OlegHahm commented Feb 7, 2014

needs rebase again.

@mehlis
Copy link
Contributor

mehlis commented Feb 12, 2014

probably the example/rpl_udp needs changes too:

$ valgrind ./bin/native/rpl_udp.elf tap0
==5912== Memcheck, a memory error detector
==5912== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==5912== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==5912== Command: ./bin/native/rpl_udp.elf tap0
==5912== 
RIOT native uart0 initialized.
RIOT native interrupts/signals initialized.
LED_GREEN_OFF
LED_RED_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

kernel_init(): This is RIOT! (Version: 2013.08-1210-gf168)
Scheduler...[OK]
kernel_init(): jumping into first task...
Auto init hwtimer module.
Auto init vtimer module.
Auto init uart0 module.
UART0 thread started.
uart0_init() [OK]
Auto init net_if module.
Set radio address on interface 0 to 1
Change this value at compile time with macro CONF_RADIO_ADDR
Set PAN ID on interface 0 to 0xabcd
Change this value at compile time with macro CONF_PAN_ID
Interface 0 initialized
Auto init 6LoWPAN module.
Auto init transport layer [destiny] module.
Initializing transport layer packages. Size of socket_type: 177
RPL router v1.1
> set 1
set 1
Set node ID to 1
> init r
init r
INFO: Initialize as root on address 1
lowpan.c, 1699: sixlowpan_lowpan_init(): add link local address to interface 0: fe80::abcd:ff:fe00:1
lowpan.c, 1712: sixlowpan_lowpan_init(): add solicited nodes multicast address to of link layer address interface 0: ff02::1:0:ff00:1
==5912== Conditional jump or move depends on uninitialised value(s)
==5912==    at 0x400B7AF: bcmp (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5912==    by 0x80565E1: ipv6_net_if_addr_match (ip.c:508)
==5912==    by 0x8056284: ipv6_net_if_add_addr (ip.c:445)
==5912==    by 0x805ADF5: sixlowpan_lowpan_init_interface (lowpan.c:1715)
==5912==    by 0x8062F5B: rpl_init (rpl.c:211)
==5912==    by 0x8050E35: rpl_udp_init (rpl.c:74)
==5912==    by 0x8051B8C: handle_input_line (shell.c:104)
==5912==    by 0x80519C2: shell_run (shell.c:162)
==5912==    by 0x8050305: main (main.c:54)
==5912== 
==5912== Conditional jump or move depends on uninitialised value(s)
==5912==    at 0x80565E7: ipv6_net_if_addr_match (ip.c:508)
==5912==    by 0x8056284: ipv6_net_if_add_addr (ip.c:445)
==5912==    by 0x805ADF5: sixlowpan_lowpan_init_interface (lowpan.c:1715)
==5912==    by 0x8062F5B: rpl_init (rpl.c:211)
==5912==    by 0x8050E35: rpl_udp_init (rpl.c:74)
==5912==    by 0x8051B8C: handle_input_line (shell.c:104)
==5912==    by 0x80519C2: shell_run (shell.c:162)
==5912==    by 0x8050305: main (main.c:54)
==5912== 
lowpan.c, 1725: sixlowpan_lowpan_init(): add all nodes multicast address to interface 0: ff02::1
lowpan.c, 1738: sixlowpan_lowpan_init(): add loopback address to interface 0: ::1
6LoWPAN and RPL initialized.
Channel set to 10
Destiny initialized
> ==5912== Invalid read of size 2
==5912==    at 0x80552A0: ipv6_send_packet (ip.c:70)
==5912==    by 0x80636E6: rpl_send (rpl.c:949)
==5912==    by 0x80634C3: send_DIO (rpl.c:328)
==5912==    by 0x80625B8: trickle_timer_over (trickle.c:134)
==5912==    by 0x4A121CEA: makecontext (in /usr/lib/libc-2.17.so)
==5912==    by 0x8082E03: ???
==5912==  Address 0x4 is not stack'd, malloc'd or (recently) free'd
==5912== 
==5912== 
==5912== Process terminating with default action of signal 11 (SIGSEGV)
==5912==  Access not within mapped region at address 0x4
==5912==    at 0x80552A0: ipv6_send_packet (ip.c:70)
==5912==    by 0x80636E6: rpl_send (rpl.c:949)
==5912==    by 0x80634C3: send_DIO (rpl.c:328)
==5912==    by 0x80625B8: trickle_timer_over (trickle.c:134)
==5912==    by 0x4A121CEA: makecontext (in /usr/lib/libc-2.17.so)
==5912==    by 0x8082E03: ???
==5912==  If you believe this happened as a result of a stack
==5912==  overflow in your program's main thread (unlikely but
==5912==  possible), you can try to increase the size of the
==5912==  main thread stack using the --main-stacksize= flag.
==5912==  The main thread stack size used in this run was 8388608.
==5912== 
==5912== HEAP SUMMARY:
==5912==     in use at exit: 0 bytes in 0 blocks
==5912==   total heap usage: 27 allocs, 27 frees, 2,737 bytes allocated
==5912== 
==5912== All heap blocks were freed -- no leaks are possible
==5912== 
==5912== For counts of detected and suppressed errors, rerun with: -v
==5912== Use --track-origins=yes to see where uninitialised values come from
==5912== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

@miri64
Copy link
Member Author

miri64 commented Feb 13, 2014

@mehlis nope, thats the network stack (and I think some false positives, because some of this "uninitialised variables" are definitely initialized at some point).

@miri64
Copy link
Member Author

miri64 commented Feb 13, 2014

@miri64
Copy link
Member Author

miri64 commented Feb 18, 2014

LoWPAN layer should be working now. Tested (but not valgrinded) with des-testbed/RIOT-projects#97. All other apps will follow.
Only the tests with short addresses will work on native, since native does not support sending and receiving of packets with long addresses (yet), though at least the sending packages are shown in wireshark (keep in mind that fragmented, uncompressed packages are either generated wrong by us or parsed wrong by the 6LoWPAN dissector; I actually guess the last one is right, since otherwise distinguishing uncompressed, fragmented packages and compressed, fragmented packages will be really, really hard except I miss something).
ND as it was should work and tests will follow this night.

@miri64
Copy link
Member Author

miri64 commented Feb 23, 2014

No longer WIP but might need some follow-up PRs for neighbor discovery.

@miri64
Copy link
Member Author

miri64 commented Feb 23, 2014

(neighbor discovery never worked right before anyway and it was not target of this PR)

@mehlis
Copy link
Contributor

mehlis commented Feb 24, 2014

@authmillenon PR needs rebase

return SIXLOWERROR_SUCCESS;
if (prefix_len > 128) {
prefix_len = 128;
}
Copy link
Member

Choose a reason for hiding this comment

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

prefix_len & 0x80 - but probably the compiler will optimize this anyway.

But this could benefit from documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you thing that your proposed code would be more efficient? I doubt it very much.

Copy link
Member

Choose a reason for hiding this comment

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

Without any optimization it's a branch less, but I doubt any gained efficiency, too. Mostly I find my proposal neater.

@OlegHahm
Copy link
Member

OlegHahm commented Mar 1, 2014

Review completed.

@miri64
Copy link
Member Author

miri64 commented Mar 1, 2014

Critiques applied and/or discussed.

@OlegHahm
Copy link
Member

OlegHahm commented Mar 1, 2014

ACK - from my perspective we are ready to merge this.

miri64 added a commit that referenced this pull request Mar 1, 2014
Decouple network stack from transceiver
@miri64 miri64 merged commit 79a16df into RIOT-OS:master Mar 1, 2014
@miri64 miri64 deleted the decouple_network_stack branch March 1, 2014 14:46
@OlegHahm
Copy link
Member

OlegHahm commented Mar 1, 2014

Hooray!

@mehlis
Copy link
Contributor

mehlis commented Mar 1, 2014

whooohoooo, let's focus on #799 which is the next big one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-award-nominee Deprecated. Will be removed soon. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants