Skip to content

net/gcoap: add macro to delay initialization of gcoap #12798

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
Nov 28, 2019
Merged

net/gcoap: add macro to delay initialization of gcoap #12798

merged 1 commit into from
Nov 28, 2019

Conversation

kb2ma
Copy link
Member

@kb2ma kb2ma commented Nov 24, 2019

Contribution description

Sometimes we do not wish gcoap to start at auto-initialization time. For example, #12104 must initialize its credman tool for DTLS before the gcoap thread starts. We wish to facilitate this use case without breaking existing applications. This PR adds a macro, GCOAP_INIT_DELAY, to accomplish this goal. When set to 1, gcoap must be started explicitly.

Testing procedure

In the gcoap example main(), insert the snippet below before the call to gcoap_cli_init().

   if (IS_ACTIVE(GCOAP_INIT_DELAY)) {
        kernel_pid_t pid = gcoap_init();
        printf("gcoap init: %u\n", pid);
    }

Test with and without defining the macro at compile time with something like:

$ CFLAGS="${CFLAGS} -DGCOAP_INIT_DELAY=1" make clean all

When defined, you should see the following at startup, and be able to send/receive gcoap messages as usual.

main(): This is RIOT! (Version: 2020.01-devel-1032-g41e2-gcoap/init_delay)
gcoap init: 6
gcoap example app
All up, running the shell now

Issues/PRs references

Alternative to #12529.

@kb2ma kb2ma added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: CoAP Area: Constrained Application Protocol implementations labels Nov 24, 2019
@kb2ma kb2ma added this to the Release 2020.01 milestone Nov 24, 2019
@kb2ma kb2ma mentioned this pull request Nov 24, 2019
Closed
kb2ma added a commit to kb2ma/RIOT that referenced this pull request Nov 24, 2019
@kb2ma
fixup! use GCOAP_INIT_DELAY from RIOT-OS#12798
e00571a
@kb2ma
Copy link
Member Author

kb2ma commented Nov 25, 2019

Thanks for the thumbs up, @pokgak. Unfortunately, I can't add you as a reviewer (yet), but I know this PR is important for gcoap support of DTLS. If you'd like to review it, I bet we could talk @leandrolanzieri or @miri64 into giving a proxy-ACK. ;-)

pokgak
pokgak suggested changes Nov 25, 2019
View reviewed changes
Copy link
Contributor

@pokgak pokgak left a comment

Choose a reason for hiding this comment

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

This is definitely simpler than the lazy init approach. I like how, with CFLAG, you can document what the flag does compared to pseudomodules where there is normally no documentation about it.

How can this CFLAG be included as a dependency to another module? I haven't seen any CFLAGS in Makefile.dep so not sure if there are any rules regarding CFLAGS in that file or not.

miri64
miri64 reviewed Nov 25, 2019
View reviewed changes
leandrolanzieri
leandrolanzieri reviewed Nov 26, 2019
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

I certainly find this solution simpler than lazy init mode.

Although, I wonder if we should add some inline function to check if the server is already running or not. I think, for example, about the gcoap_cli, when you call coap info it will tell you that the server is listening on GCOAP_PORT even when gcoap is not initialized yet. There may be in the future some other modules that depend on gcoap server to be running on init?

Otherwise I find this macro to be a good solution, and agree with @miri64 's proposal for the renaming.

@kb2ma
Copy link
Member Author

kb2ma commented Nov 27, 2019

How can this CFLAG be included as a dependency to another module? I haven't seen any CFLAGS in Makefile.dep so not sure if there are any rules regarding CFLAGS in that file or not.

sys/Makefile.dep includes addition to CFLAGS.

@kb2ma
Copy link
Member Author

kb2ma commented Nov 27, 2019

Although, I wonder if we should add some inline function to check if the server is already running or not.

@pokgak, would a function like bool gcoap_is_running() be useful for DTLS integration? My original idea was that initialization was only delayed, and not deferred until some arbitrary time. In other words, I didn't think there would be much question on when to call gcoap_init().

Also, we may create some more generic facility on the state of gcoap, which I kind of started with gcoap_op_state(). It might make more sense to expand on that function rather than create a standalone function. I guess this is all a way of saying I'd rather wait until we need the capability to decide how to implement it.

@kb2ma
Copy link
Member Author

kb2ma commented Nov 27, 2019

Comments addressed.

leandrolanzieri
leandrolanzieri approved these changes Nov 27, 2019
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Changes look good. I tested this and works as expected. ACK.

@leandrolanzieri
Copy link
Contributor

@kb2ma please squash

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 27, 2019
@kb2ma kb2ma added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 28, 2019
@kb2ma
Copy link
Member Author

kb2ma commented Nov 28, 2019

Squashed and tests pass.

@leandrolanzieri leandrolanzieri merged commit 2733ef4 into RIOT-OS:master Nov 28, 2019
@pokgak pokgak mentioned this pull request Dec 10, 2019
Open
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

+1 more reviewer

@pokgak pokgak pokgak requested changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
4 participants
@kb2ma @leandrolanzieri @miri64 @pokgak