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

nativenet: Port nativenet for netdev #1732

Merged
merged 3 commits into from
Oct 9, 2014
Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Sep 29, 2014

To test this, run

export BOARD=native
SENDER=0 make -C tests/netdev/ all flash && SENDER=1 make -C tests/netdev/ all flash && make -C tests/netdev/ test

Depends on #1492: (it's merged)

* #1680
|\
… …
| |
| * #1448
| |\
* | | #1733
| * | #1732
| | * #1731
| |/
|/
* (master)

@LudwigKnuepfer LudwigKnuepfer added the State: waiting for other PR State: The PR requires another PR to be merged first label Sep 29, 2014
@miri64 miri64 added Area: network Area: Networking Platform: native Platform: This PR/issue effects the native platform Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Sep 29, 2014
@miri64 miri64 added this to the Release NEXT MAJOR milestone Sep 29, 2014
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 2, 2014
@miri64
Copy link
Member Author

miri64 commented Oct 2, 2014

Rebased to master

@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 2, 2014
@miri64 miri64 force-pushed the netdev-nativenet branch 2 times, most recently from ab486b6 to beab588 Compare October 3, 2014 13:14
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Oct 3, 2014
@miri64
Copy link
Member Author

miri64 commented Oct 6, 2014

@LudwigOrtmann poke?

@LudwigKnuepfer
Copy link
Member

Ayeee

@@ -96,6 +96,10 @@ ifneq (,$(filter ccn_lite,$(USEMODULE)))
USEMODULE += crypto
endif

ifneq (,$(filter nativenet,$(USEMODULE)))
USEMODULE += netdev_base
Copy link
Member

Choose a reason for hiding this comment

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

The change in https://github.com/RIOT-OS/RIOT/pull/1732/files#diff-2b35e3dab26520778e1991f7041d00a5R3 seems to imply it is still possible to use nativenet without netdev_base..?

Copy link
Member Author

Choose a reason for hiding this comment

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

c05d6f1 fixes that

@LudwigKnuepfer
Copy link
Member

I've got wget AND curl installed btw.

./tests/01-interaction.py
Traceback (most recent call last):
  File "/usr/lib/python3.4/site-packages/pexpect/__init__.py", line 972, in read_nonblocking
    s = os.read(self.child_fd, size)
OSError: [Errno 5] Input/output error

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib/python3.4/site-packages/pexpect/__init__.py", line 1535, in expect_loop
    c = self.read_nonblocking(self.maxread, timeout)
  File "/usr/lib/python3.4/site-packages/pexpect/__init__.py", line 977, in read_nonblocking
    raise EOF('End Of File (EOF). Exception style platform.')
pexpect.EOF: End Of File (EOF). Exception style platform.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "./tests/01-interaction.py", line 131, in <module>
    sys.exit(main())
  File "./tests/01-interaction.py", line 25, in main
    receiver.expect(r"RIOT netdev test")
  File "/usr/lib/python3.4/site-packages/pexpect/__init__.py", line 1451, in expect
    timeout, searchwindowsize)
  File "/usr/lib/python3.4/site-packages/pexpect/__init__.py", line 1466, in expect_list
    timeout, searchwindowsize)
  File "/usr/lib/python3.4/site-packages/pexpect/__init__.py", line 1554, in expect_loop
    raise EOF(str(err) + '\n' + str(self))
pexpect.EOF: End Of File (EOF). Exception style platform.
<pexpect.spawn object at 0x7fb1d8ca54a8>
version: 3.3
command: /usr/bin/make
args: ['/usr/bin/make', 'term']
searcher: <pexpect.searcher_re object at 0x7fb1d8caff60>
buffer (last 100 chars): b''
before (last 100 chars): b'udwig/RIOT/tests/netdev/../../Makefile.include:56: *** Neither wget nor curl is installed!.  Stop.\r\n'
after: <class 'pexpect.EOF'>
match: None
match_index: None
exitstatus: 2
flag_eof: True
pid: 9872
child_fd: 3
closed: False
timeout: 5
delimiter: <class 'pexpect.EOF'>
logfile: None
logfile_read: None
logfile_send: None
maxread: 2000
ignorecase: False
searchwindowsize: None
delaybeforesend: 0.05
delayafterclose: 0.1
delayafterterminate: 0.1
Makefile:49: recipe for target 'test' failed
make: *** [test] Error 1

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2014

before (last 100 chars): b'udwig/RIOT/tests/netdev/../../Makefile.include:56: *** Neither wget nor curl is installed!. Stop.\r\n'

it says

@LudwigKnuepfer
Copy link
Member

It's lying.

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2014

I'll look into that tomorrow.

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2014

But just by this error message I would presume it has something to do with a70ee0f. I don't do anything with neither curl nor wget.

@LudwigKnuepfer
Copy link
Member

Maybe you mess up the environment ?

@miri64
Copy link
Member Author

miri64 commented Oct 6, 2014

Can you give me the output with the following patch?

diff --git a/tests/netdev/tests/01-interaction.py b/tests/netdev/tests/01-interaction.py
index 3f2d66b..fb7037a 100755
--- a/tests/netdev/tests/01-interaction.py
+++ b/tests/netdev/tests/01-interaction.py
@@ -8,6 +8,7 @@

 import os, sys, time
 from pexpect import spawn, TIMEOUT, EOF
+import pprint

 class Abort(Exception):
     pass
@@ -17,9 +18,13 @@ board = os.environ.get('BOARD', 'native')
 DEFAULT_TIMEOUT=5

 def main():
+    pp = pprint.PrettyPrinter(indent=4)
+    pp.pprint(dict(os.environ))
     receiver = spawn("make term", timeout=5, env={"SENDER": '0'})
+    receiver.logfile = sys.stdout
     time.sleep(1)
     sender = spawn("make term", timeout=5, env={"SENDER": '1'})
+    sender.logfile = sys.stdout

     try:
         receiver.expect(r"RIOT netdev test")

@LudwigKnuepfer
Copy link
Member

(I did, off the record)

@miri64 miri64 force-pushed the netdev-nativenet branch 3 times, most recently from f7b35a9 to 45d4cf0 Compare October 8, 2014 10:37
@@ -206,7 +215,7 @@ int _native_marshall_ethernet(uint8_t *framebuf, radio_packet_t *packet)
* Linux does this on its own, but it doesn't hurt to do it here.
* As of now only tuntaposx needs this. */
if (data_len < ETHERMIN) {
DEBUG("padding data! (%d -> ", data_len);
DEBUG("padding data!(%d -> ", data_len);
Copy link
Member

Choose a reason for hiding this comment

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

that is part of the coding conventions?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. See dce54c4

@LudwigKnuepfer LudwigKnuepfer added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Oct 8, 2014
#include "nativenet.h"

#ifndef NETDEV_DEFAULT
#define NETDEV_DEFAULT (&nativenet_default_dev)
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering about two things here:

  • why not use it in tests/netdev/main.c?
  • wouldn't it make more sense to set this from the device?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • To the first point: Like I did here?
  • To the second: Maybe, but that's only a matter of opinion IMHO

Copy link
Member Author

Choose a reason for hiding this comment

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

(and here)

Copy link
Member

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.

ad 2: sure, but if there is a possibility to not care about specific devices in the abstraction layer, it should be taken.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway - 2 is not really part of this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

to 1: must have been drunk when I did this. 1 isn't really part of this PR either ;-)

Copy link
Member

Choose a reason for hiding this comment

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

just do it ad 1 ;)

@LudwigKnuepfer
Copy link
Member

Sorry for commenting in commits...

@miri64
Copy link
Member Author

miri64 commented Oct 9, 2014

Fixed it anyways =)

@LudwigKnuepfer
Copy link
Member

Quite elegantly so by use of reduction =)

@LudwigKnuepfer
Copy link
Member

ACK

@LudwigKnuepfer
Copy link
Member

(don't forget to squash)

@miri64
Copy link
Member Author

miri64 commented Oct 9, 2014

done and waiting for Travis

@LudwigKnuepfer
Copy link
Member

Maybe I'll have another look ... ;) (just kidding)

miri64 added a commit that referenced this pull request Oct 9, 2014
nativenet: Port nativenet for netdev
@miri64 miri64 merged commit 56f8de8 into RIOT-OS:master Oct 9, 2014
@miri64 miri64 deleted the netdev-nativenet branch October 9, 2014 08:46
@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 Oct 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Platform: native Platform: This PR/issue effects the native platform 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.

3 participants