-
Notifications
You must be signed in to change notification settings - Fork 2k
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
TinyDTLS integrated to RIOT (without sockets) #5395
Conversation
Awesome! 👍 |
I just felt like referencing this PR #4308 here |
Hi @PeterKietzmann, I used that PR as a reference for the structure of the examples, about all the I was unable to run the examples from that PR, plus that in January, the TinyDTLS code suffered changes, after emigrating from Sourceforge to Eclipse. |
~~Wouldn't it make more sense to a) still have it as a package, so changes from upstream can be pulled in easier (especially for security-related libs I would argue this still makes a lot of sense)? Sorry, I somehow misstook your examples as lib code... |
@@ -0,0 +1 @@ | |||
Subproject commit 6c479ef140b09e83b79b7e26fad8d36c7bebe442 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file, it isn't needed
Hi, @authmillenon Thanks by noticing that my
Could be, though to be honest, I selected the GNRC because was the one in the examples, additionally to the sockets. I'm not sure, which one is intended to be THE main stack in RIOT. |
Hi @authmillenon This is rookie question with Git. I already find my error trying to link to a branch instead to a specific commit in the new Makefile. However, I want to concentrated all my work with TinyDTLS/RIOT in one specific branch from my fork (mostly because I need to work with Linux and contiki versions as well). Is possible to indicated to the new makefile to check a specifically branch ? ( I was experimenting to squash all my commits in one single one in the same branch, but not success) |
GNRC is intended to be the main stack, but we don't want to bind users to that (
This should be possible, just give the branch name for the |
all: git-download | ||
$(MAKE) -C $(PKG_BUILDDIR) | ||
|
||
include $(RIOTBASE)/pkg/pkg.mk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the superfluous newline
after updating my fork with
The line |
of course it checks out a specific commit. A branch is just a reference (in git internal terms: a file named after the branch, containing the object identity (sha-1 sum) of the most recent commit in the branch), so when you checkout a branch you checkout the most recent commit in that branch. What are you trying to do? Regarding your error: You need to adapt your include paths, since the checked out package no resides in another directory: diff --git a/pkg/tinydtls/Makefile.include b/pkg/tinydtls/Makefile.include
index eb1f442..f56e059 100644
--- a/pkg/tinydtls/Makefile.include
+++ b/pkg/tinydtls/Makefile.include
@@ -1 +1 @@
-INCLUDES += -I$(RIOTBASE)/pkg/tinydtls -I$(RIOTBASE)/pkg/tinydtls/tinydtls
+INCLUDES += -I$(BINDIRBASE)/pkg/$(BOARD)/tinydtls Also: please rebase if you use the new package scheme (but I guess you already did that just not pushed ;-)) |
You will realize that a lot of the GNRC API changed ;-) |
Hi @authmillenon, I upgrade my lines with the new GNRC's API (is more friendly!) but when I'm trying to compile fore testing I got the following:
I tried adding the |
Including a header does not do much to include a module ;-). You have to add it to the |
b73a90c
to
b739d21
Compare
Many thanks @authmillenon, I just forget the Ok, let see if I didn't screwed anything after the rebase. |
Setting the CI label to have Travis and Murdock have a look. |
589b969
to
34329dd
Compare
Most of the board are black listed, shouldn't we fix bugs for boards before merging it? |
👍 |
For me is OK as that is a wise action for RIOT. Though, I'll be unable to work closely on them until finishing the thesis and the defense (March next year). If this is action is preferred, then maybe is a good idea to close this PR and open a new one when those bugs are fixed. |
For now we need a rebase. Also: now that sock is in, consider porting to that (you will find it is a lot more comfortable than conn, also see #6005). |
30a5650
to
106d95e
Compare
Sorry by the delay. I hope I had fixed correctly the chaos.
I was planning to do it after my thesis. But, I had though that conn was intended to be used instead of socket. Oh well, minor thing. |
@rfuentess (POSIX) sockets and |
Here's some reference ;-). |
Sorry about the noise :( I now realize that the occurances of |
@rfuentess Please consider rfuentess#2 to address my comments from April. Other than that I say we can merge. |
eba6046
to
285f5ca
Compare
@miri64 merged and rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK & go when travis (murdock, I'm tired, okay?) is happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some adaptations to current master
PKG_NAME=tinydtls | ||
PKG_URL=git://github.com/rfuentess/TinyDTLS.git | ||
# PKG_VERSION=RIOT-OS | ||
PKG_VERSION=f824b5553a865c186a9b41236be03358f0c8feaf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace (as reported by Murdock)
(void) _dtls_kernel_pid; | ||
|
||
/* Only one instance of the server */ | ||
if (server.pid != KERNEL_PID_UNDEF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since #5526 was merged this needs to be server.target.pid
(and this PR needs a rebase to current master)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this PR needs a rebase to current master
Just to avoid the conflicts from the previous week. This means to fetch my local repository with the changes from RIOT-OS and then rebase my branch with those changes, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but just to avoid any confusion here is what you should do
git remote add riot git://github.com/RIOT-OS/RIOT # you might already have done this
git fetch riot
git rebase -i riot/upstream # here remove any commits that do NOT belong to this PR
git push -f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you don't have any conflicts in here this should run rather smoothly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, It was riot/master
, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ähh yes. Sorry.
dtls_init(); | ||
|
||
/* We genarte */ | ||
server.pid = thread_create(_server_stack, sizeof(_server_stack), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use gnrc_netreg_entry_init_pid()
to initialize server
, please.
static void stop_server(void) | ||
{ | ||
/* check if server is running at all */ | ||
if (server.pid == KERNEL_PID_UNDEF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server.target.pid
|
||
/* stop server */ | ||
gnrc_netreg_unregister(GNRC_NETTYPE_UDP, &server); | ||
server.pid = KERNEL_PID_UNDEF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server.target.pid
Ok, fetched riot, rebase with riot/master, added the changes requested, upgraded the client, tested correctly the client and server in native. BUT now the FIT IoT-LAB (greenoble and paris sites) are returning an empty list when Not sure if it is just bad timing with the testbed or something we are forgetting to upgrade on the Makefile. Edit: Seems is just bad timing with FIT IoT-Lab. |
Nothing to do with FIT IoT-Lab, nor timings, nor this PR. #5891 just messed up the dependencies for iotlab-m3 a little so the driver for the network device was not in included. Find fix at #6023. |
285f5ca
to
8a3ade3
Compare
Tested with available hardware. Already pushed. Edit: Push again, after the rebase boards nucleo-f070 and nucleo-f030 do not have enough memory. |
Support for TinyDTLS (0.8.6) is added together an example at 'examples/dtls-echo'.
8a3ade3
to
cf64aba
Compare
Confirmed it to be working. ACK and go as soon as Murdock agrees. |
Wow! Pretty cool! Nice job, @rfuentess! |
Excellent! Congrats! |
This is a tested example using RIOT and TinyDTLS ( V. 0.8.2 from Eclipse instead of sourceforge).
The TinyDTLS's code had been modified to support the GNRC architecture. This is why the clone of the pkg/tinydtls is with my fork of TinyDTLS.
There is too support for simple sockets, yet the examples are not finished, and are not included in this PR.