Add HTTPServer example #17

kegilbert · 2019-01-07T23:46:05Z

Updated version of: https://github.com/janjongboom/mbed-os-example-http-server/

Placing this example in the docs only repo as per previous discussions with @cmonr until the examples standardization work is further along.

Wanted some feedback from Jan and someone on the networking group (tagged Seppo for now). Please feel free to tag anyone else who may be interested. We don't have CI yet in this repo but I have run the code through astyle and built it with GCC_ARM, IAR, and ARM compilers.

@AnotherButler, this was one of the new examples we had discussed with @iriark01 and @SenRamakri

janjongboom · 2019-01-08T08:56:35Z

@kegilbert Great idea, happy to see this included.

Why are the docs still mentioning easy connect whilst the code is using the default interface pattern.
Same in mbed_app.json, there's a network type selection screen.

Note that I've added Greentea tests to Mbed HTTP recently. Could we included them in the CI as well for this repo?

janjongboom · 2019-01-08T08:57:20Z

One of the things I would consider could use improving is to use the asynchronous socket interface for everything. I think we could get rid of the threads.

Also, I'd suggest to put the stats on a web page, instead of on the console. That'd be more fun.

SeppoTakalo · 2019-01-08T09:46:19Z

Is the aim to publish this as a official example?

I would prefer this not to be part of official Handbook, but instead published somewhere in mbed.org, or do we have some kind of Cookbook.
Reason for that is, there is no team to actually maintain and keep this up to date, so we should not announce it as such.

SeppoTakalo

I don't like the approach where the code lives inside header files.

Can you move the HTTPresponseparser and server to .cpp files?

SeppoTakalo · 2019-01-08T09:50:19Z

HTTPServer/main.cpp

+        printf("Server could not be started... %d\n", res);
+    }
+
+    wait(osWaitForever);


Why this is here?

If we rely on event-loop to run, and main() has nothing else to do, can we just same some RAM and run event-loop from main thread?

janjongboom · 2019-01-08T09:51:40Z

@SeppoTakalo It lives at https://os.mbed.com/teams/sandbox/code/http-webserver-example/ currently.

SeppoTakalo · 2019-01-08T13:57:15Z

I think that is the proper location for it.

We should have clear distinction between what examples are part of API handbook, and what are just examples build on top of Mbed OS 5.

We don't have HTTP library, and therefore this example should not end up in the handbook.

But we should create some kind of Cookbook (did we already have it at some point?) that demonstrates all kind of cool things that you could build on top of Mbed OS.

kegilbert · 2019-01-08T17:50:15Z

@SeppoTakalo Good point, there's ongoing work this release to expand and unify our examples. We don't really have many examples now that build interesting user applications off of Mbed OS, and as such we don't have a proper location yet for them to live.

Moving forward the plan is to add a section to the Handbook Tutorial page that includes more varied examples. These examples would live in a consolidated Github repo allowing CI, OS release updates, and review processes to become standardized. I would like some of the examples to use libraries outside of Mbed OS as a way to showcase how to integrate other applications and allow some really interesting projects. This does add more maintenance problems though and am open to suggestions. We'll be having some internal discussions soon, I'll be sure to include both of you in the process.

cmonr · 2019-01-09T19:32:01Z

@ARMmbed/mbed-docs Thoughts?

iriark01 · 2019-01-09T20:42:27Z

@SeppoTakalo it should be in the handbook (the cookbook is no longer supported), just not in the API references (that's for code snippets). We are absolutely looking for solid, interesting and useful examples to include in the docs.

cmonr · 2019-01-26T02:07:25Z

@iriark01 @SeppoTakalo @kegilbert Thoughts on moving this forward, or should this be closed?

iriark01 · 2019-01-26T08:19:54Z

Sorry, I was waiting for @SeppoTakalo to reply.

iriark01 · 2019-01-28T07:50:31Z

Also, @AnotherButler is collecting these

SeppoTakalo

OK,

If this is going to go into the handbook, and we don't have the cookbook anymore, then I need to request some changes to it.

Documentation is outdated, it refers to easy-connect, but does not use it. It also mentions outdated ESP8266 patches.
mbed_app.json defines parameters that are not used in this app. Those were used by easy-connect.
Implementation needs to go into .cpp files. We should not have all the code in header files.
Other small changes suggested.

Other than those, this is a good example.
Who is maintaining tha HTTP library that this depends on?

SeppoTakalo · 2019-01-28T07:53:42Z

HTTPServer/http_server.h

+                if (recv_ret < -3000 || recv_ret == 0) {
+                    return;
+                } else {
+                    continue;


What is this sequence supposed to do?
Any negative value should be handled as a error. You should not continue reading the socket anymore.

https://github.com/ARMmbed/mbed-os/blob/master/features/netsocket/Socket.h#L108..L113

SeppoTakalo · 2019-01-28T07:54:46Z

HTTPServer/LICENSE

@@ -0,0 +1,191 @@
+Apache License


Do we need full license text? Other examples do not have it, they just refer to Apache 2.0 in the source files.

SeppoTakalo · 2019-01-28T07:55:23Z

HTTPServer/README.md

+
+## To build
+
+1. Open ``mbed_app.json`` and change the `network-interface` option to your connectivity method ([more info](https://github.com/ARMmbed/easy-connect)).


I would not prefer to use easy-connect. It is not something we maintain.

SeppoTakalo · 2019-01-28T07:57:49Z

HTTPServer/mbed_app.json

+            "mbed-http.http-buffer-size": 2048,
+            "platform.stdio-baud-rate": 115200,
+            "platform.thread-stats-enabled": 1,
+            "nsapi.socket-stats-enable": 1


I would convert this example to use the default network interface and limit this file to only

{ "target_overrides": { "*": { "mbed-http.http-buffer-size": 2048, "platform.stdio-baud-rate": 115200, "platform.thread-stats-enabled": 1, "nsapi.socket-stats-enable": 1 } } }

SeppoTakalo · 2019-01-28T07:59:53Z

HTTPServer/main.cpp

+
+    // Connect to the network (see mbed_app.json for the connectivity method used)
+    NetworkInterface *network = NetworkInterface::get_default_instance();
+    if (!network) {


NetworkInterface::get_default_instance() will give you the interface, but it does not connect.

You need to also add network->connect()

The HttpServer class constructor connects with the provided interface.

SeppoTakalo · 2019-01-28T08:02:44Z

HTTPServer/README.md

+
+* K64F with Ethernet.
+* NUCLEO_F411RE with ESP8266.
+    * For ESP8266, you need [this patch](https://github.com/ARMmbed/esp8266-driver/pull/41).


This is outdated, the external repository is not used anymore.

This is still valid.

SeppoTakalo · 2019-01-28T08:03:19Z

HTTPServer/http_response_builder.h

+#include "http_parser.h"
+#include "http_parsed_url.h"
+
+static const char *get_http_status_string(uint16_t status_code)


Static functions in header files?

kegilbert · 2019-01-28T17:44:23Z

Apologies for the delay! I've done some work to remove references to easy-connect and some minor cleanup. I'm a bit tied up at the moment with the example consolidation project but I'll get back to this ASAP to resolve Seppo's other comments.

Thanks for the review!

cmonr · 2019-02-14T02:53:12Z

@kegilbert Ping

kegilbert · 2019-02-14T18:06:39Z

Sorry about that, flu and example consolidation sort of knocked me out for a good while. Hoping to polish this off end of this week/beginning of next week.

kegilbert · 2019-02-21T01:20:08Z

Large scale refactor finished and ran through astyle. Still need to clean up some comments but functionally mostly there.

@janjongboom I poked around a bit with getting the thread stats to display on the webpage but my Javascript fu was a bit lacking. I wasn't able to get the XMLHttpRequest onstagechange to work correctly (think the object was being killed as it fell out of scope before the response could be sent). I can poke around a bit more later if you think it'd be worth it.

janjongboom · 2019-02-25T11:28:32Z

@kegilbert Reviewed, and added some changes here: kegilbert#1

Make http-server look pretty

kegilbert · 2019-02-27T00:26:52Z

@janjongboom Merged and test with three concurrent connections. Looks pretty awesome! My only concern is the addition of another external library, who maintains rapidjson?

janjongboom · 2019-02-27T16:41:59Z

@kegilbert Tencent. It's not an Mbed specific library.

kegilbert · 2019-03-29T21:52:15Z

@SeppoTakalo @janjongboom Any further thoughts/feedback? Going forward into 5.13 this will be tested along with every other example. Is the Tencent RapidJSON library a stable enough product do you think to not impact CI?

SeppoTakalo

Small changes requested.
I think this is now leaking memory.

SeppoTakalo · 2019-04-01T07:47:50Z

HTTPServer/http_server.cpp

+            m->thread = t;
+            socket_threads.push_back(m);
+        } else {
+            delete clt_sock;


Delete nullptr?
This else-block does not have any effect.

SeppoTakalo · 2019-04-01T07:48:01Z

HTTPServer/README.md

+
+* K64F with Ethernet.
+* NUCLEO_F411RE with ESP8266.
+    * For ESP8266, you need [this patch](https://github.com/ARMmbed/esp8266-driver/pull/41).


This is still valid.

SeppoTakalo · 2019-04-01T07:56:41Z

HTTPServer/http_server.cpp

+
+void HttpServer::receive_data()
+{
+    TCPSocket *socket = sockets.back();


Did you mean to use .pop_back(), because now it looks like the sockets vector is growing constantly as I cannot find any place where sockets would be removed from there.

Add HTTPServer example

8bb9f3c

kegilbert requested review from janjongboom and SeppoTakalo January 7, 2019 23:46

SeppoTakalo suggested changes Jan 8, 2019

View reviewed changes

SeppoTakalo suggested changes Jan 28, 2019

View reviewed changes

Large scale refactor

0c44933

janjongboom added 5 commits February 25, 2019 12:25

Add default content type for responses

25796f4

Don't call connect from the http_server

6b4246c

Clean up the mbed_app.json from references to easy-connect

c24af71

Update to Mbed OS 5.11

f68c441

Add JSON reply with thread stats and show them on the web page

a3e2e32

kegilbert and others added 2 commits February 26, 2019 18:04

Merge pull request #1 from janjongboom/httpserver-pretty

db9714b

Make http-server look pretty

Increase server_thread default stack size

0e1f40b

kegilbert requested a review from SeppoTakalo February 27, 2019 00:26

janjongboom mentioned this pull request Mar 20, 2019

Mainline Mbed HTTP ARMmbed/mbed-os#10169

Closed

SeppoTakalo suggested changes Apr 1, 2019

View reviewed changes


		## To build

		1. Open ``mbed_app.json`` and change the `network-interface` option to your connectivity method ([more info](https://github.com/ARMmbed/easy-connect)).

Add HTTPServer example #17

Are you sure you want to change the base?

Add HTTPServer example #17

Uh oh!

Conversation

kegilbert commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janjongboom commented Jan 8, 2019

Uh oh!

janjongboom commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SeppoTakalo commented Jan 8, 2019

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janjongboom commented Jan 8, 2019

Uh oh!

SeppoTakalo commented Jan 8, 2019

Uh oh!

kegilbert commented Jan 8, 2019

Uh oh!

cmonr commented Jan 9, 2019

Uh oh!

iriark01 commented Jan 9, 2019

Uh oh!

cmonr commented Jan 26, 2019

Uh oh!

iriark01 commented Jan 26, 2019

Uh oh!

iriark01 commented Jan 28, 2019

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kegilbert commented Jan 28, 2019

Uh oh!

cmonr commented Feb 14, 2019

Uh oh!

kegilbert commented Feb 14, 2019

Uh oh!

kegilbert commented Feb 21, 2019

Uh oh!

janjongboom commented Feb 25, 2019

Uh oh!

kegilbert commented Feb 27, 2019

Uh oh!

janjongboom commented Feb 27, 2019

Uh oh!

kegilbert commented Mar 29, 2019

Uh oh!

SeppoTakalo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kegilbert commented Jan 7, 2019 •

edited

Loading

janjongboom commented Jan 8, 2019 •

edited

Loading