nmattisson / HttpClient

Http Client Library for the Spark Core (also well suited for Arduino and other embedded platforms).
Other
121 stars 150 forks source link

experimental implementation with request "by reference", which makes usa... #2

Closed synox closed 10 years ago

synox commented 10 years ago

...ge easier (see my email i sent you)

nmattisson commented 10 years ago

Hi @synox, and thank you for the pull request!

I think your idea to initialise the status code is great. Probably it's better to make it -1 as that's an approach I've seen used often to detect errors.

With regards to the copying the response body here:

response->body = raw_response.substring(bodyPos+4);

...the response variable is actually already a pointer, so the intention is that the body is stored in the string already allocated by the user from the main application, always using the same memory space for different responses as to not create a memory leak. I don't have a great understanding of the 'String' object though, so if I were to guess the problem lies with Strings not having a fixed memory size. My hunch is that the best solution would be to use char buffer instead of the same max size as the TCP buffer, and not use the String library in the struct. What do you think?

(I'm also new to C++, so I might very well have some flaws in my reasoning).

synox commented 10 years ago

Yes, using String might be tricky. How about returning a pointer to the buffer, at the position of the beginning of the response body.

  response.body = buffer[bodyPos+4];

then it is important that there is a zero at the end of the output body.

nmattisson commented 10 years ago

Yes, there is already a check that adds a '\0' at the end of the buffer in case of an overflow — but if the body is less than the buffer size and doesn't contain the zero then there could be an issue. I'd guess that the TCP client would run into problems too in that scenario though, so it might be unnecessary to check for.

synox commented 10 years ago

and what do you think about the init() function, where you can reuse the client with another hostname later?

nmattisson commented 10 years ago

Hi @synox, I made an update that I think addresses your problems: https://github.com/nmattisson/HttpClient/commit/b91698493e7ee0c893e8ab800f755f387b245f86

Instead of adding an init function, I moved the port/path/host to the request struct. It accomplishes the same thing, but I think it's a cleaner implementation.

Also, all the function calls are now by reference and I think it could solve your memory issues. It seems to work decently for me anyways. :)

Let me know if this works!

synox commented 10 years ago

great, thanks for the update. I am testing...

BTW, you should not change the application.h, but just

include "HttpClient.h"

in your application.cpp

synox commented 10 years ago

With timeapi it seems to work fine. But I just can't load data with the library from the following host, i don't know why. Can you please try it as well?

http://query.yahooapis.com/v1/public/yql?

nmattisson commented 10 years ago

Updated the header files too.

I also have no luck with query.yahooapis.com, but the problem seems to be on the TCPClient level or lower, even without HttpClient I'm running in to the same error. Even when I try it in the browser I have issues, it sends me to some dev site instead of a proper response.

The alternative on weather.yahooapis.com seems to work fine though, so I'd use that.

I'll close this one now. Thank you for your help!