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

Changes from Iotivity for draft-ietf-core-coap-tcp-tls and cross-platform support #47

Open
wants to merge 49 commits into
base: develop
Choose a base branch
from

Conversation

dthaler
Copy link

@dthaler dthaler commented Sep 17, 2016

These changes were already in the Iotivity code base, and we're trying to unfork libcoap and have iotivity just use the regular libcoap library. These changes include support for draft-ietf-core-coap-tcp-tls and some cross-platform fixes (e.g., to also run on Windows).

Change-Id: Idddee819530e8eb497a8abf72aa1f94a7b30d40b
Signed-off-by: Dave Thaler <[email protected]>
Change-Id: Idddee819530e8eb497a8abf72aa1f94a7b30d40b
Signed-off-by: Dave Thaler <[email protected]>
…elop

Change-Id: I92dee387c3d58b2d348366e923be007efe833b3e
…elop

Change-Id: I92dee387c3d58b2d348366e923be007efe833b3e
…elop

Change-Id: Ib573dd192fdb84622e9bd24b2e5932dad1dd9501
…elop

Change-Id: Ib573dd192fdb84622e9bd24b2e5932dad1dd9501
Change-Id: I41875348e9667a999bbb223bca3d99d4201fc8fb
Signed-off-by: Dave Thaler <[email protected]>
Change-Id: I41875348e9667a999bbb223bca3d99d4201fc8fb
Signed-off-by: Dave Thaler <[email protected]>
…elop

Change-Id: I02a8a520b3743f13b5ab957773c012587263fc83
@obgm
Copy link
Owner

obgm commented Sep 20, 2016

Thanks, this contribution is highly appreciated. Unfortunately, Travis reports this PR to break the Linux port. It would be great if you could take a look into that issue.

Signed-off-by: Dave Thaler <[email protected]>
@dthaler
Copy link
Author

dthaler commented Sep 22, 2016

Just pushed a fix for the bug due to a bad merge

@obgm
Copy link
Owner

obgm commented Sep 23, 2016

My apologies -- I have pushed an old patch to src/block.c which now breaks this PR.

src/net.c Outdated
@@ -554,7 +554,7 @@ coap_send_ack(coap_context_t *context,
return result;
}

#if defined(WITH_POSIX) || defined(WITH_CONTIKI)
#if !defined(WITH_LWIP)
Copy link
Contributor

@tijuca tijuca Sep 23, 2016

Choose a reason for hiding this comment

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

Please don't use negative logic here and let that check as it is, it's hard to figure out for some not well knowledge people what !WITH_LWIP means!

Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
@rzr
Copy link

rzr commented Nov 7, 2016

Is this task tracked on iotivity tracker ?

I see there is a conflict to be resolved

@dthaler
Copy link
Author

dthaler commented Nov 17, 2016

Yes https://jira.iotivity.org/browse/IOT-1072 tracks the iotivity task. IoTivity already uses this branch temporarily on Windows and other OS's will start snapping to it after merge. I will rebase to fix the conflict.

Signed-off-by: Dave Thaler <[email protected]>
@dthaler
Copy link
Author

dthaler commented Jan 20, 2017

I believe all feedback so far has been addressed. Would like to see this PR merged so iotivity can start depending on the upstream branch. Let me know if there are any more blocking issues.

@tijuca
Copy link
Contributor

tijuca commented Jan 22, 2017

Hello Dave,

first thanks for contributing! Some comments below.

Beside the build issues I'd like to see better commit messages, please see and note https://github.com/obgm/libcoap/blob/develop/CONTRIBUTE#L161
Please describe why the commit is made in the way you have done and make explanation what the commit is about (for example c1bda6e 749514f fd5c9ec). It's a bit tedious to go through every source change to understand what's changed and why. That's obviously not needed for typo fixes, but a good commit message helps to see what was changed.

Please don't mix extensions of specific changes to the header files or small adoptions within the header files with improving of the API documentation in other parts, split them of into more easier to understand changes.
For example c11591d is much to big. But other commits will need also be split of into smaller peaces.

Also try to decrease merge commit from upstream much as possible, normally there shouldn't be even one as you should use git rebase instead of git merge to pull upstream changes. I'm currently not able to make a review due the huge changes that I not really understand what and why the source is changed.

Please don't be disappointed, such big changes need time to get integrated. Maybe you can split the PR into more than one PR if it turns out there are some thematic changes. If I read your first post this PR is including two things, tcp-tls and cross-platform support for Windows. For the latter it would be good to have build support than.

Regards
Carsten

@dthaler
Copy link
Author

dthaler commented Feb 10, 2017

FYI, The changes in this PR have been done in IoTivity's fork of this repo across the course of a couple years by a bunch of people. I am merely pushing them here, I am not authoritative for all the original changes, just trying to get rid of a forked copy and make contributions in the future to a single place rather than IoTivity folks continuing to make changes to their own fork. I will do what I can, but I may not be able to answer all questions myself. Pushing back just means that tons of devices (like all the devices Samsung ships) will continue to use a separate fork not the master GitHub version.

@rzr
Copy link

rzr commented Feb 10, 2017

Hi as Samsung OSG developer I believe it's better for iotivity to be the most aligned possible with upstream, thanks @dthaler for your efforts.

@tijuca
Copy link
Contributor

tijuca commented Feb 10, 2017

Dave, thanks for a little bit explanation about the details of this PR.
I agree and encourage the best for libcoap and the other projects is obviously to land pull requests early in libcoap and the uses code is not differing much between projects. It's like in the kernel, commit early and do also a pull request early.

I think Olaf is of course willing to pull your changes in this PR. But we need to puzzle the various things out into parts that are logical things. That for one simple reason, not only we need to understand what the changes are about. So how to get further here?

I'd suggest to do the easy parts first, for example changes that are made by you or direct working people.

Olaf has released the recent state of libcoap as version 4.1.2 so we can start now mostly lazy with new changes.

@malinengineer malinengineer mentioned this pull request Feb 15, 2017
danmihai1 and others added 7 commits March 8, 2017 12:04
Apparently caused by an incorrect merge.

Signed-off-by: Dan Mihai <[email protected]>
Fix pdu length truncation
This commit makes sure libcoap doesn't generate any W4
warnings when building with a Visual Studion 2013 and 2015
compiler.
Making sure that "if" statements have opening brackets
on the same line to fit the with the rest of the code.
@AnderssonPeter
Copy link

Any news on this?
I would love to have Windows support!

@obgm
Copy link
Owner

obgm commented Apr 13, 2017

It certainly is high on my priority list but sorting stuff out will take some time.
Maybe it is feasible to cherry-pick some of the Windows-specific compatibility code. That part seems pretty straight-forward to me.

This change contains only packet format changes in the library
to support coap+ws and coaps+ws.

Signed-off-by: G S Senthil Kumar <[email protected]>
jcmichelou added a commit to spinetix/libcoap that referenced this pull request May 6, 2017
…p on windows.

Merge changes from PR obgm#47 related to windows platform support.
Add full windows platform support including correct error checking and working regression tests and examples.
Remove need to build applications using libcoap with -DWITH_POSIX and to include coap_config.h. This applies to the examples as well.
jcmichelou added a commit to spinetix/libcoap that referenced this pull request May 6, 2017
jcmichelou added a commit to spinetix/libcoap that referenced this pull request May 25, 2017
…p on windows.

Merge changes from PR obgm#47 related to windows platform support.
Add full windows platform support including correct error checking and working regression tests and examples.
Remove need to build applications using libcoap with -DWITH_POSIX and to include coap_config.h. This applies to the examples as well.

# Conflicts:
#	examples/client.c
#	examples/coap-rd.c
#	examples/coap-server.c
#	include/coap/address.h
#	include/coap/coap_io.h
#	include/coap/net.h
#	include/coap/prng.h
#	src/address.c
#	src/block.c
#	src/net.c
#	src/pdu.c
#	src/platform/posix/coap_io.c
#	src/resource.c
jcmichelou added a commit to spinetix/libcoap that referenced this pull request May 25, 2017
@obgm
Copy link
Owner

obgm commented Jun 4, 2018

@dthaler I am having trouble understanding what these changes are about. For example, commit 5f2d238 introduces a function coap_add_token_to_empty_message(). Neither the commit log nor the doxygen comment for the function prototype in commit bd60461 explain what this function is about and, more important, why it is needed in addition to coap_add_token().

mwichmann and others added 2 commits September 13, 2018 08:29
The function coap_pdu_parse2() as changed by iotivity patches has a
block of code bracketed by

  #if defined(WITH_TCP) || defined(WITH_WS)

the problem is this block contains a reference to a structure member
that is defined only if WITH_WS is defined, and it is possible to build
this code in iotivity in the presence of WITH_TCP and the absence of
WITH_WS. Whether that should ever happen is a separate question; the
change keeps these two conditions separate so this can be used in the
iotivity CI system which does build some targets in the way noted.

Signed-off-by: Mats Wichmann <[email protected]>
Fix for where WITH_TCP is defined, WITH_WS is not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants