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

gnrc_slip: Fixes #3137

Merged
merged 4 commits into from
Aug 25, 2015
Merged

gnrc_slip: Fixes #3137

merged 4 commits into from
Aug 25, 2015

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jun 1, 2015

I found some bugs in SLIP while testing #2457. This PR fixes them in bulk.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) NSTF labels Jun 1, 2015
@miri64 miri64 added this to the Network Stack Task Force milestone Jun 1, 2015
@haukepetersen
Copy link
Contributor

looks sane to me, but I dont have the time to test it today. Hopefully very soon...

@miri64
Copy link
Member Author

miri64 commented Jun 14, 2015

Had you time to test @haukepetersen?

@miri64
Copy link
Member Author

miri64 commented Jul 1, 2015

Ping?

@haukepetersen
Copy link
Contributor

I checked out this PR, rebased it on master and got the following error when compiling for the stm32f4discovery:

/home/hauke/dev/riot/RIOT/sys/auto_init/netif/auto_init_slip.c:27:25: fatal error: slip_params.h: No such file or directory
 #include "slip_params.h"

Seems like you forgot to add the application folder to the includes?

@miri64
Copy link
Member Author

miri64 commented Jul 2, 2015

@miri64
Copy link
Member Author

miri64 commented Jul 2, 2015

Also: I don't observe this error:

BOARD=stm32f4discovery make -C tests/slip/
make: Entering directory `/home/martine/Repositories/RIOT-OS/RIOT/tests/slip'
Building application "driver_slip" for "stm32f4discovery" with MCU "stm32f4".

"make" -C /home/martine/Repositories/RIOT-OS/RIOT/boards/stm32f4discovery
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/core
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/cpu/stm32f4
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/cpu/cortexm_common
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/cpu/stm32f4/periph
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/drivers
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/auto_init
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/auto_init/netif
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/compat/hwtimer
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_netapi
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_netif
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_netif/hdr
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_netreg
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_pktbuf
/home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_pktbuf/ng_pktbuf.c: In function 'ng_pktbuf_start_write':
/home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_pktbuf/ng_pktbuf.c:137:20: warning: passing argument 1 of 'atomic_dec' from incompatible pointer type
         atomic_dec(&pkt->users);
                    ^
In file included from /home/martine/Repositories/RIOT-OS/RIOT/core/include/mutex.h:25:0,
                 from /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_pktbuf/ng_pktbuf.c:25:
/home/martine/Repositories/RIOT-OS/RIOT/core/include/atomic.h:81:19: note: expected 'struct atomic_int_t *' but argument is of type 'unsigned int *'
 static inline int atomic_dec(atomic_int_t *var)
                   ^
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/crosslayer/ng_pktdump
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/net/link_layer/ng_slip
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/newlib
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/od
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/shell
"make" -C /home/martine/Repositories/RIOT-OS/RIOT/sys/shell/commands
   text    data     bss     dec     hex filename
  25012     120   14456   39588    9aa4 /home/martine/Repositories/RIOT-OS/RIOT/tests/slip/bin/stm32f4discovery/driver_slip.elf
make: Leaving directory `/home/martine/Repositories/RIOT-OS/RIOT/tests/slip'

(the warnings are long fixed btw).

@jnohlgard
Copy link
Member

How do I use the slip interface in practice?

The tunslip program in dist/tools/ is still used as the PC side, just like Contiki?
I guess I need to modify a Makefile or configuration header for the slip interface to be built into the ng_networking example.

@miri64
Copy link
Member Author

miri64 commented Jul 15, 2015

@gebart yes you need to use tunslip6 and modify the Makefile (or your environment) with:

USEMODULE += ng_slip
CFLAGS = -DNG_NETIF_NUMOF=2

@miri64
Copy link
Member Author

miri64 commented Jul 15, 2015

Oh and you need to copy the slip_params.h file from tests/slip and also add the current directory as include path:

CFLAGS += -I$(CURDIR)

@miri64
Copy link
Member Author

miri64 commented Jul 16, 2015

Addressed offline comments

@@ -155,7 +162,8 @@ static void _slip_receive(ng_slip_dev_t *dev, size_t bytes)
ng_pktbuf_hold(pkt, ng_netreg_num(pkt->type, NG_NETREG_DEMUX_CTX_ALL) - 1);

while (sendto != NULL) {
DEBUG("slip: sending pkt %p to PID %u\n", pkt, sendto->pid);
DEBUG("slip: sending pkt %p to PID %" PRIkernel_pid "\n", (void *)pkt,
sendto->pid);
ng_netapi_receive(sendto->pid, pkt);
Copy link
Contributor

Choose a reason for hiding this comment

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

how about ng_netapi_dispatch_receive() 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.

Because this PR is older than this feature ;-)

@haukepetersen
Copy link
Contributor

Changes look good to me, and I can compile for the stm32f[3|4]discovery boards. I further tried to get the slip stuff running with the ng_networking example (just copied over the slip_params.h file and adjusted it accordingly), but no success here. After bootup, the slip interface only has this global IPv6 address ff02::1/128. When adding some unicast address manually, I was not able to ping between two nodes (which are connected via their UARTs...). So not sure what I am missing.

Also I was wondering how slip goes together with the neighbor cache, as the slip interface does not have link layer addressing?

@miri64
Copy link
Member Author

miri64 commented Jul 16, 2015

Rebased to current master and addressed comments.

@miri64
Copy link
Member Author

miri64 commented Jul 16, 2015

After bootup, the slip interface only has this global IPv6 address ff02::1/128. When adding some unicast address manually, I was not able to ping between two nodes (which are connected via their UARTs...). So not sure what I am missing.

Might be an issue with the neighbor discovery. Never tested it with a link-layer layer without addresses.

Also I was wondering how slip goes together with the neighbor cache, as the slip interface does not have link layer addressing?

The neighbor cache should not become involved here. The neighbor cache is involved with Address Resolution, but since SLIP does not has any addressing, there is no Address Resolution needed. It might however be possible, that IPv6 never is able to determine the next hop. As I've said: I never really tested next hop determination with without link-layer addresses ;-).

@miri64
Copy link
Member Author

miri64 commented Jul 16, 2015

Let's just merge this for now and focus on debugging this later (since I'm still not having a suitable board/UART adapter to test this properly myself).

@miri64
Copy link
Member Author

miri64 commented Jul 18, 2015

Tested it myself today. It's the ND. It can't handle address less link-layers. Will fix tomorrow.

@jnohlgard
Copy link
Member

ping @authmillenon

@miri64
Copy link
Member Author

miri64 commented Aug 8, 2015

Waiting for review.

@miri64
Copy link
Member Author

miri64 commented Aug 8, 2015

Wait... Did I fix ND?

@jnohlgard
Copy link
Member

also: Needs rebase

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2015

Rebased. Fix to ND I will do in a seperate PR.

@miri64
Copy link
Member Author

miri64 commented Aug 14, 2015

See #3628

@miri64
Copy link
Member Author

miri64 commented Aug 18, 2015

Rebased to current master.

@miri64
Copy link
Member Author

miri64 commented Aug 18, 2015

And fixed some issues missed on rebase.

@miri64 miri64 changed the title ng_slip: Fixes gnrc_slip: Fixes Aug 18, 2015
@OlegHahm
Copy link
Member

@haukepetersen, to me it looks that this is ready to merge and fix the other problems in separate PRs. Do you agree?

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Aug 25, 2015
@haukepetersen haukepetersen added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 25, 2015
@haukepetersen
Copy link
Contributor

ACK, just kicked Travis...

@miri64
Copy link
Member Author

miri64 commented Aug 25, 2015

Fixed UART_1 issue

@haukepetersen
Copy link
Contributor

re-ACK when squashed.

@miri64
Copy link
Member Author

miri64 commented Aug 25, 2015

Squashed.

miri64 added a commit that referenced this pull request Aug 25, 2015
@miri64 miri64 merged commit 293c532 into RIOT-OS:master Aug 25, 2015
@miri64 miri64 deleted the slip/fix/some-fixes branch August 25, 2015 20:19
@miri64 miri64 added the Area: network Area: Networking label Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants