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

COAP unusable #658

Closed
djwdjw321 opened this issue Sep 19, 2015 · 15 comments
Closed

COAP unusable #658

djwdjw321 opened this issue Sep 19, 2015 · 15 comments

Comments

@djwdjw321
Copy link

The primary problem is that there is no documentation but there also seem to be a number of bugs. I looked through the code but I am not expert enough in Lua/C interfaces to debug the code myself. As this is a very significant protocol for IOT for small devices like this, I am stalled until it is fixed.

The COAP server will only respond with numbers. It may have something to do with a mixup between "payload" and "content", but I didn't have time to analyze the code in detail. Without the ability to respond with text, especially JSON, it really cannot be used.

It also does not seem to support the JSON data type yet although I could use a text data type for now.

The client code doesn't seem to handle responses (but this may just be a doc issue).

There may be more but that is as far as I was able to get. I am happy to help get this fixed but it needs an expert on the current code.

Was the code written from scratch or adapted from another COAP implementation?

@marcelstoer
Copy link
Member

marcelstoer commented Sep 19, 2015

The missing documentation is (not yet) addressed in #472 and #486.

this is a very significant protocol for IOT for small devices

That depends on who you ask. According to my own stats CoAP isn't significant for the ESP8266 community.

it needs an expert on the current code

Primarily this needs a reproducible test case from you to bolster the claim that the CoAP module be buggy. It may very well be that it's buggy but how would one know without a test case? Normally I'd also suggest you look at all open issues related to CoAP but in this specific case there's too much noise.

And if you think there are multiple bugs you need multiple isolated test cases - and multiple individually reported issues here.

Was the code written from scratch or adapted from another COAP implementation?

Looking at the license might give you an idea where this modules comes from (I believe). You need to understand that many of the modules were contributed "by the community" and may not necessarily be maintained by the NodeMCU core team.

@djwdjw321
Copy link
Author

Thanks, the string issue is easy to reproduce using the fragment code. Just set myvar to anything other than a number. The response question is also easy to see but may just be a matter of documentation. COAP may not have a high demand now but one of the things I wan't to do is to demonstrate that ESP8266 + nodemcu + UDP/COAP + JSON is a great combination for simpler IOT objects. If the string issue can be fixed, I can make progress. I have a tight deadline and I just need to know if I need to change my plans or not.

@ashubhatt
Copy link

Found a work around solution?

@djwdjw321
Copy link
Author

djwdjw321 commented Sep 23, 2015

No. I am looking at the code to see if I can identify the issue but I am not an expert on interfacing Lua with C and there are a lot of moving parts. I am hoping the original developer takes a look soon. Nodemcu + COAP is a great combination for battery powered IOT sensors due to the fast connect time of UDP. MQTT is slower as it uses TCP/IP and although its pub/sub model has some advantages, it doesn't meet my needs. So, I may have to wait.

@loles
Copy link

loles commented Nov 7, 2015

@loles
Copy link

loles commented Nov 7, 2015

This should allow to send also strings. Sending JSON will require more changes. I didn't test it yet. First I need to learn how to build custom image :)

diff --git a/app/coap/endpoints.c b/app/coap/endpoints.c
index a54b123..17c7a3f 100644
--- a/app/coap/endpoints.c
+++ b/app/coap/endpoints.c
@@ -64,8 +64,8 @@ static int handle_get_variable(const coap_endpoint_t *ep, coap_rw_buffer_t *scra
                     {
                         n = lua_gettop(h->L);
                         lua_getglobal(h->L, h->name);
-                        if (!lua_isnumber(h->L, -1)) {
-                            NODE_DBG ("should be a number.\n");
+                        if (!lua_isnumber(h->L, -1) && !lua_isstring(h->L, -1)) {
+                            NODE_DBG ("should be a number or string.\n");
                             lua_settop(h->L, n);
                             return coap_make_response(scratch, outpkt, NULL, 0, id_hi, id_lo, &inpkt->tok, COAP_RSPCODE_NOT_FOUND, COAP_CONTENTTYPE_NONE);
                         } else {

@djwdjw321
Copy link
Author

djwdjw321 commented Nov 7, 2015

I took the "else" part of the statement as covering strings. I will look closer. Thanks.
I really want to use JSON with CoAP.

@TerryE
Copy link
Collaborator

TerryE commented Nov 7, 2015

@loles, Łukasz.

This should allow to send also strings. Sending JSON will require more changes. I didn't test it yet. First I need to learn how to build custom image :)

It makes more sense for you to build and test the patch locally before pusing it to your own public fork. See Contributing to NodeMCU and esp8266.com -- How to set up manually the GCC toolchain and SDK which explains how to build your own dev VM and install the toolchain so you can do local builds of the firmware.

@djwdjw321
Copy link
Author

If strings can be sent, JSON can be sent but to fully comply with the CoAP RFC, the media type ( in the content-format option sent with the response: see section 12.3 of the RFC) should be set to 0 for text and 50 for JSON. If the receiving program knows to expect JSON this is not critical.

The key is to allow variable length payloads containing UTF-8 encoded characters to be sent in a response. Although I haven't been able to fully analyze the code yet, there are three response type defined in coap.h: 0 for text, 40 for linkformat and -1 described as "a bodge to allow us not to send an options block". The response code is set to 0 when a number is returned and -1 when text is supposed to be returned instead of 0 (or 50 for JSON). That might be the problem or it may be that text is not actually being returned. I don't know yet.

Dave White

@lukaszo
Copy link
Contributor

lukaszo commented Nov 9, 2015

Ok. I created a fix. It works for me, now you can set content type with: cs:var("all", coap.JSON)

I didn't create PR yet because coap doesn't work for me on dev branch so I tested it on master. Code in both branches for coap is the same :/ I tried to enable debug with define DEVELOP_VERSION but then nodemcu doesn't work, my esp is rebooted by watchdog :(

PS. I used company account earlier instead of private.

jmattsson pushed a commit that referenced this issue Nov 17, 2015
Allow to set content type in COAP (fix #658)
@rahulpowar
Copy link

@lukaszo Note that the content type is hardcoded to 0 for .well-known/core due to the implementation here https://github.com/nodemcu/nodemcu-firmware/blob/dev/app/coap/endpoints.c#L210

@lukaszo
Copy link
Contributor

lukaszo commented Dec 29, 2015

@rahulpowar Yes, I know. I didn't want to touch it because it's used in a little strange way in build_well_known_rsp In feature it should be adjusted

@marcelstoer
Copy link
Member

@lukaszo @rahulpowar is one of you going to create a PR with your changes to improve our CoAP module?

@lukaszo
Copy link
Contributor

lukaszo commented Jul 14, 2016

@marcelstoer Fix is already merged 3a5e845

I don't know why github is not showing Pull request here :/

@marcelstoer
Copy link
Member

So, the "no strings supported" and "not able to set content type" issues are fixed, thanks Łukasz. We even have the JSON example in the docs: http://nodemcu.readthedocs.io/en/latest/en/modules/coap/#example_1.

What still remains to be fixed/added is response processing on the client but that should be handled in a dedicated issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants