Skip to content

Add civetweb HTTP sample #17019

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 30, 2019
Merged

Add civetweb HTTP sample #17019

merged 1 commit into from
Jul 30, 2019

Conversation

tgorochowik
Copy link
Member

@tgorochowik tgorochowik commented Jun 24, 2019 •
edited
Loading

This PR adds support for CivetWeb - an external, POSIX-based HTTP library.

The form of this integration - filling the incompatibilities between CivetWeb and minimal libc - is a result of discussions on TSC and the networking forum, as described in #16683
Note that the additional hacky header only defines (and sometimes even declares) only the absolute minimum to get the sample to work.

Since the CivetWeb support for Zephyr is not yet merged, the west configuration points to our fork, thus - DNM.

It was tested on SAM E70 Xplained.

CC: @laperie @jukkar @PiotrZierhoffer @bel2125

@tgorochowik tgorochowik added the DNM This PR should not be merged (Do Not Merge) label Jun 24, 2019
@tgorochowik tgorochowik mentioned this pull request Jun 24, 2019
Merged
jukkar
jukkar reviewed Jun 25, 2019
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Obviously there are lot of things that would need ported to zephyr but generally this looks ok. Memory consumption seems to be quite high, I wonder if we could make it lower or is this the absolute minimum that we could have.

@tgorochowik
Copy link
Member Author

Yes, actually initially the stacks were quite small but we were getting a BUS fault every now and then (usually when handling a bigger request). To avoid that we just made them very large. The current settings are an overkill for sure.
This can probably be fine-tuned to optimize the memory usage for all the thread stacks.

@tgorochowik tgorochowik removed the DNM This PR should not be merged (Do Not Merge) label Jul 9, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jul 9, 2019 •
edited
Loading

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@tgorochowik tgorochowik changed the title [DNM] Add civetweb HTTP sample Add civetweb HTTP sample Jul 10, 2019
@tgorochowik tgorochowik force-pushed the civetweb-sample branch 2 times, most recently from e3399f7 to 8fd4f40 Compare July 10, 2019 13:31
@PiotrZierhoffer PiotrZierhoffer force-pushed the civetweb-sample branch 4 times, most recently from 431443d to 4dd852e Compare July 22, 2019 06:44
@pfalcon pfalcon mentioned this pull request Jul 22, 2019
Closed
@tgorochowik @PiotrZierhoffer
samples: net: Add civetweb HTTP sample
58f00fa 
This commit adds civetweb as a west module and a sample that uses it.

Signed-off-by: Tomasz Gorochowik <[email protected]>
Signed-off-by: Piotr Zierhoffer <[email protected]>
@ioannisg
Copy link
Member

@pfalcon @tbursztyka could you review this patch, as well?

@pfalcon
Copy link
Collaborator

pfalcon commented Jul 30, 2019

I'm on vacation till next week. We already discussed this matter a lot, so I wouldn't give to -1, and if other folks don't spot small issues, it should be good to go.

tbursztyka
tbursztyka reviewed Jul 30, 2019
View reviewed changes
samples/net/sockets/civetweb/src/libc_extensions.c
return zsock_recv(sock, buf, max_len, flags);
}

int socket(int family, int type, int proto)
Copy link
Collaborator

@tbursztyka tbursztyka Jul 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why re-declaring all these functions since include/net/socket.h does it already when CONFIG_NET_SOCKETS_OFFLOAD is not set. (and when it is, the offloading part provides these anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

It only provides zsock_socket. socket and the like are only provided if CONFIG_NET_SOCKETS_POSIX_NAMES is enabled which cannot be used together with POSIX_APIS in general. So until #16621 is merged we were told to add this stuff to the sample (see #16683 and other issues referenced there).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

@jukkar jukkar merged commit f2bf9a1 into zephyrproject-rtos:master Jul 30, 2019
@erwango erwango mentioned this pull request Dec 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jukkar jukkar jukkar approved these changes

@tbursztyka tbursztyka tbursztyka approved these changes

@nashif nashif Awaiting requested review from nashif

@pfalcon pfalcon Awaiting requested review from pfalcon

Assignees
No one assigned
Labels
area: Modules area: Networking area: Samples Samples
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tgorochowik @zephyrbot @ioannisg @pfalcon @jukkar @tbursztyka