Skip to content

nanocoap: add context pointer to resources#8513

Merged
kb2ma merged 5 commits intoRIOT-OS:masterfrom
kaspar030:nanocoap_add_context_to_resources
Feb 22, 2018
Merged

nanocoap: add context pointer to resources#8513
kb2ma merged 5 commits intoRIOT-OS:masterfrom
kaspar030:nanocoap_add_context_to_resources

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 commented Feb 2, 2018 •
edited
Loading

Contribution description

This PR adds an opaque context pointer to coap resources.
It allows e.g., parameterizing coap resource handlers.

One use case would be a generic "saul coap handler" which would get it's driver information through the newly introduced context parameter. This handler could then easily be re-used.

Example (pseudocode):

const coap_resource_t coap_resources[] = {
    { "/sensors/light", COAP_GET, saul_handler, my_light_sensor_saul_object },
    { "/sensors/sound", COAP_GET, saul_handler, mysound_sensor_saul_object },
};

@kaspar030 kaspar030 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 2, 2018
@kaspar030 kaspar030 requested a review from kb2ma February 2, 2018 18:06
@jnohlgard
Copy link
Copy Markdown
Member

jnohlgard commented Feb 2, 2018

I understand this is useful, but could you describe in the PR description what the intended purpose is? 1-2 sentences is probably enough

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 3, 2018

I understand this is useful, but could you describe in the PR description what the intended purpose is? 1-2 sentences is probably enough

Yes, sorry!

@smlng smlng added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Feb 6, 2018
@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 6, 2018

I need this e.g., for the javascript coap bindings.

In javascript is is possible to dynamically register a coap handler callback. For that, the coap_register_t is allocated, with a generic javascript handler and a context struct containing a reference to the actual javascript handler object and some housekeeping.

kb2ma
kb2ma requested changes Feb 8, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@kb2ma kb2ma left a comment

Choose a reason for hiding this comment

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

Overall, this addition is simple and straightforward. Tested on native for the nanocoap and gcoap examples. I also tried actually reading a context value, and that worked too. Unit tests OK.

Change request: You redefine CoAP resource arrays in several places to add the context pointer. In a few places, you use designated initializers and in a few places you use generic initializers. Please use one form or the other for consistency.

Also see the inline comment about the name of the attribute.

sys/include/net/nanocoap.h
const char *path; /**< URI path of resource */
unsigned methods; /**< OR'ed methods this resource allows */
coap_handler_t handler; /**< ptr to resource handler */
void *context; /**< ptr to user defined context data */
Copy link
Copy Markdown
Member

@kb2ma kb2ma Feb 8, 2018

Choose a reason for hiding this comment

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

I prefer naming this attribute 'data' rather than 'context'. To me, 'context' includes a connotation of an enveloping entity. This pointer is just user-defined data for the resource. I don't feel strongly about this though -- it's just my preference and I'm OK with 'context'.

Copy link
Copy Markdown
Contributor Author

@kaspar030 kaspar030 Feb 19, 2018

Choose a reason for hiding this comment

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

I chose context to be consistent with netdev...

@kaspar030
Copy link
Copy Markdown
Contributor Author

kaspar030 commented Feb 19, 2018 •
edited
Loading

Change request: You redefine CoAP resource arrays in several places to add the context pointer. In a few places, you use designated initializers and in a few places you use generic initializers. Please use one form or the other for consistency.

Hm, do you mind a lot?

I used designated initializers when they fit into 80 characters, as they're IMO much more readable, and if they would've been used before, would have saved me touching them.
I chose not to convert to designated initializers in the resource arrays where they would have gone over 80 characters, as I prefer the short lists (compared to multiple lines per entry).

I hoped to have chosen the most readable option.

kb2ma
kb2ma approved these changes Feb 20, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@kb2ma kb2ma left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I understand your choices on use of designated initializers. I appreciate addition of the one where line length was not an issue because I agree that it is clearer. Coding conventions are silent on use of designated initializers.

@kb2ma kb2ma merged commit d7004eb into RIOT-OS:master Feb 22, 2018
@kaspar030 kaspar030 deleted the nanocoap_add_context_to_resources branch May 11, 2018 21:37
@kb2ma kb2ma mentioned this pull request Jun 22, 2018
Merged
@kb2ma kb2ma mentioned this pull request Mar 4, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kb2ma kb2ma kb2ma approved these changes

Assignees

No one assigned

Labels

Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kaspar030 @jnohlgard @kb2ma @smlng