HTTP client API #50
HTTP client API #50
Conversation
nashif
added
the
In progress
Very cool, but why it's a part of PR titled "HTTP client API"? People interested in consistent netsamples API will never find it here. People who saw it once, later likely won't be able to find again where it was. I'd suggest to resubmit it as a separate PR. |
samples/net/http_client/README.rst
Outdated
|
The set cannot be used without the netsamples API in second commit. Why would we need to supply it in different PR? |
|
I have a general question about code like this (which applies also to samples/bluetooth/gatt). Is it expected that out of tree applications which want this functionality will be building against the files in samples/net/common, the same way the in-tree apps in samples/ do? |
There was a problem hiding this comment.
This apparently should be removed before merge.
There was a problem hiding this comment.
Enabling debug by default for sample apps is currently done on purpose. Because there is no kconfig for the sample apps debug options, it makes sense to activate the debugging by default for them.
There was a problem hiding this comment.
@jukkar : Ack, so do we need "#if 1" line? If you say "yes", I'm only happy, I love these, and will include them in my patches ;-).
Because it's independent from "HTTP client API", and is a standalone feature. For example, it can be approved independently and be merged, not being blocked by review of completely different features. Of course, I'm not in position to request splitting this particular PR, and a single case is not that important. I'm just expressing hope that switching to Github won't lead to proliferation of cases when unrelated features are submitted in one PR. With Gerrit, that's simply not possible - each commit is a separate review, even if one depends on another. I always thought of that as a limitation of Gerrit, but have to admit that disciplines developers. Multi-commit reviews should be an exception, not a rule. IMHO. |
My guess that as one of the next evolutionary steps we might want to convert it to Kconfig-driven "sub-subsystem". But trying to build OOT app against samples/net/common might be a good first step, to show how ugly it is, and the need for Kconfig-based selection ;-). |
There was a problem hiding this comment.
Why not create a generic client/server API that is not part of the samples and instead available to applications directly without having to include additional code? Samples need to work stand-alone and not by referencing additional code that is not part of Zephyr. What we need there is a simple way to initiate a client or a server and hook into it.
|
@nashif : I believe Jukka does exactly that in this patch. It introduces subsys/net/lib/http/http_client.c , and refactors existing samples to use it. (Is it just me, or people review PRs on github less closer than it was on Gerrit? Perhaps we need to adjust to new look&feel ;-) Oh, and another reason to stuff less commits in every PR ;-) ). |
No he does not, the only code I see in the first patch is adding some "common" code under samples, which is what I do not like, samples will have to reference code that is not part of Zephyr. I prefer to see this common server code implemented as an API, not some common code you include in your Makefile. |
Who'd say!
It doesn't introduce it, it moves bunch of functionality into it, apparently from samples, even bigger bunch hopefully newly added. My biggest interest whether it handles multiple TCP packets, by looking at the serve counterpart at zephyrproject-rtos/net-tools#1, it should. Because older "http client" was funny and supported only single HTTP packet ;-). |
I believe there can't be better argument against adding unrelated commits into one PR ;-). For me, the patch @nashif describes is "last" :-D |
samples/net/http_client/README.rst
Outdated
samples/net/http_client/README.rst
Outdated
samples/net/http_client/README.rst
Outdated
samples/net/http_client/README.rst
Outdated
There was a problem hiding this comment.
all these lines need to be indented 3 spaces to be part of the code-block
Currently the sample-app setup API was only meant for network sample applications as we cannot really know what kind of requirements the out-of-tree apps have. |
Without the sample-app API patch, the http-client application will not work properly. That is why they are submitted together. Besides the sample-app API patch is meant to be "internal" api and only usable for sample apps (at least in this stage), so we can change it at will atm. |
Please, one step at a time. We can create certainly more APIs on top of this HTTP client API and then we can remove some of the samples if needed. |
The "common" sample code API is there so we can get rid of the similar startup functionality that is found in several net sample applications. I though this was something @nashif wanted earlier, but perhaps I misunderstood. At this point the sample app API is very internal code and not meant to be used any out-of-tree application. So there are two things in this patchset:
I could have sent 2) as a separate patch but then the two PRs would be need to be applied in certain order, so it is more convenient to anyone testing this if the patches are groupped in the same set. |
The HTTP client API expects the caller to supply the HTTP context and "response" buffer where the HTTP response is stored. Currently there is only one HTTP context and one such buffer in the sample http-client, so only one HTTP query at the time for that sample app. The API itself does not limit the number of queries. |
That's all good, but my point is that not these criteria should be deciding whether patches should be submitted as one PR or as different. Instead, only one factor should be enough: whether they reasonably can be submitted separately. Of course, there's enough fuzz factor and subjectivity in that, and in this case, we apparently have different opinions on that. Anyway, this isn't much related to this specific patch, instead to a general "migration to github" discussion. So, I'm finishing with that, though my original point remains - having more commits in one PR complicates the review. |
include/net/http.h
Outdated
There was a problem hiding this comment.
If it's "Callback used when reply has been received", why is it called http_request_cb_t?
There was a problem hiding this comment.
Indeed, I will rename it to http_response_cb_t
include/net/http.h
Outdated
There was a problem hiding this comment.
Why do we use enum http_method in some places, and char* in other?
There was a problem hiding this comment.
Leftover from HTTP client application, it wanted to add a space after the method which required a separate string. I will investigate if we can use directly the strings that are available from the http_parser.c.
include/net/http.h
Outdated
There was a problem hiding this comment.
It would be helpful to add a note about format of header_fields.
|
@jukkar :
I see, and it's capable to accumulate response spread over different TCP packets. So, that's good, maybe the problem I saw was in different protocol handling. So, this is definitely a good evolutionary update of HTTP client. Thanks for accepting suggestions for better naming of different pieces. I think that it's still not last time when we may need to tweak the HTTP client, but for now, this is definitely a good improvement, +1. |
I assumed that the main point of adding to samples/ is to provide working reference code, that does something useful, which Zephyr's users can try out and modify to write their own applications. Especially so since there is already a tests/ directory for exercising functionality. If it's more of an internal exercise for the satisfaction of Zephyr's developers, then never mind. If the audience for samples/ includes the users, though, then having these "common" directories presents a problem. People who want to build applications similar to a given sample are either going to build directly against the code in the common directories, which is ugly and something of a layering violation, or they'll copy/paste it, which is also problematic in that updates and fixes to the in-tree versions imply manual maintenance for the users. |
Instead of separate sample application that does everything related to HTTP client connectivity, create a HTTP client library that hides nasty details that are related to sending HTTP methods. After this the sample HTTP client application is very simple and only shows how to use the client HTTP API. Signed-off-by: Jukka Rissanen <[email protected]>
This creates a common API for network sample applications for setting up IP addresses etc. The HTTP client application is modified to use this API. Signed-off-by: Jukka Rissanen <[email protected]>
The samples are meant to be example how to do things. What this "things" means is the question. If you look at the various network samples in samples/net/*, they do these setup etc. things a bit differently. There are multiple ways of setting up network etc, it really depends on what the application is expected to do.
I agree with you. As mentioned in my comment above, lets try to build up some generalized API for applications to use for network setup. |
Cool, thanks, glad to hear it! Sorry, as your initial response had me wondering... My main concern is about this becoming a pattern which grows the way samples/bluetooth/gatt has grown. There are already 500+ SLOC in there, which provide really useful things like IPSS. Our application has a copy of that in https://github.com/Linaro/zephyr-fota-hawkbit/blob/master/src/bt_ipss.c. That diverged a bit, and has had to be maintained for little things like converting to SPDX, the new integer types, etc. that could have taken no effort on our part if we used a common library in the kernel instead. |
Yes, i want this, but done as part of Zephyr and through well documented APIs, not additional source files we include in the sample. The cross referencing of source files across the sample tree has always been a problem, especially with the build system right now. I want basically to be able to copy a sample and build it out of tree without having to deal with additional files, i want to setup client/server using well defined APIs. |
|
@jukkar
|
|
+1 for not setting unrealistic expectations for the usual incremental patches. I recently did a change similar to that - exposed to 3rd-party apps previously internal net shell options: ff3b019 . So, @mbolivar interested in that, me potentially interested in that, @jukkar likely interested in that too, but his time currently better be used on real bugs in the stack. We'll get there.
IMHO, that name is lousy and needs more bikeshedding ;-). Ditto for Kconfig names which will activate it, etc. Again, let's do it outside of this initial patch which already makes a good progress. |
Makes sense to me 👍 |
Well, my immediate plan was to
Anyway, I already started to convert HTTP server to have a similar http-server library as the client one. If the there is too much resistance against about merging this patch, please tell me know so I do not waste any more time for http-server fixes. |
|
You might want to take a look at a review I made a few months ago on the original HTTP patches, it has some comments on how to design a good client API. |
|
@jukkar ok, we have a deal then, can we have JIRAs for all those points so we wont forget them and potentially have them for 1.9? Thanks. |
nashif
removed
the
In progress
…et-callback Replace gpio set callback
…rtos#50) Build ISH kernel using wiki steps. The kernel can be built with provided inputs or with default values. Signed-off-by: Mateusz Redzynia <[email protected]>
7 participants
This one replaces the very low level HTTP client sample application with a simple application that uses more user friendly HTTP client library.
Second patch creates a simple API for network application setup. So instead of each net sample application doing various configurations itself, there is now an API for doing that. Later patches will convert the net samples to use this new API.