Skip to content

net/rdcli*: restructure and rename RD related modules#10163

Merged
miri64 merged 8 commits intoRIOT-OS:masterfrom
haukepetersen:opt_cord_restructuring
Oct 17, 2018
Merged

net/rdcli*: restructure and rename RD related modules#10163
miri64 merged 8 commits intoRIOT-OS:masterfrom
haukepetersen:opt_cord_restructuring

Conversation

@haukepetersen
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen commented Oct 15, 2018

Contribution description

As discussed in #7428: the Resource Directory endpoint implementations that we merged (i.e. rdcli and rdcli_simple) use some naming conventions that are not in line with the wording used in draft-ietf-core-resource-directory-15.

This PR restructures all existing resource directory code by applying a more clear, and extensible structure, while following the wording used in the draft.

I introduced a high-level module called cord, which includes all the code related to interacting with resource directories. All existing (endpoint) and future (lookup client) code is put as submodules into this high level module. See sys/net/application_layer/cord/doc.txt as included in this PR for more information on the high-level structure.

Sorry for the amount of lines of code in this PR, but I think it does make sense to have all these changes in a single PR. I tried to do the changes in self-contained commits to make reviewing as easy as possible...

Testing procedure

Simply verify that the two example applications (now cord_ep and cord_epsim) are still working as expected.

Also check the generated doxygen.

Issues/PRs references

#7428

@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CoAP Area: Constrained Application Protocol implementations labels Oct 15, 2018
@haukepetersen haukepetersen mentioned this pull request Oct 15, 2018
Merged
@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 15, 2018
@waehlisch waehlisch requested review from miri64 and smlng and removed request for miri64 October 15, 2018 14:15
miri64
miri64 requested changes Oct 16, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

LGTM. Some minor comments though.

examples/cord_ep/README.md Outdated
```
CFLAGS += "-DCORD_EP=\"MyNewEpName\""
```

Copy link
Copy Markdown
Member

@miri64 miri64 Oct 16, 2018

Choose a reason for hiding this comment

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

Git is complaining about this whitespace.

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Oct 17, 2018

Choose a reason for hiding this comment

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

ups

examples/cord_ep/README.md Outdated

This example application demonstrates the usage of RIOT's Resource Directory
(RD) endpoint module, called `cord_ep`. This module supports the registration,
update, and removal procedures as defined in the
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 16, 2018

Choose a reason for hiding this comment

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

Wording: "in the draft-ietf-core-…" sound's clunky (might be an artifact from the previous version where it was "in the […] draft".

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Oct 17, 2018

Choose a reason for hiding this comment

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

fixed

examples/cord_ep/README.md Outdated
Some connection parameters are configured statically during compile time,
namely the lifetime (`CORD_LT`) and the node's endpoint name (`CORD_EP`). You
can change these values at compile time by overriding their defines:
Using command line arguments:
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 16, 2018

Choose a reason for hiding this comment

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

"overriding their defines using command line arguments"

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Oct 17, 2018

Choose a reason for hiding this comment

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

fixed

sys/include/net/cord/epsim.h
/**
* @brief Spawn a new thread that registers the node and updates the
* registration with all responding RDs using the simple registration
* process
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 16, 2018

Choose a reason for hiding this comment

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

The #if defined(MODULE_CORD_EPSIM_STANDALONE) is new and should be documented:

/**
 […]
 * @note   Only available with @ref net_cord_ep_standalone compiled in.
 */

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Oct 17, 2018

Choose a reason for hiding this comment

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

added

@haukepetersen haukepetersen force-pushed the opt_cord_restructuring branch from 391e647 to c17dc9d Compare October 17, 2018 07:30
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 17, 2018

addressed comments and rebased.

miri64
miri64 reviewed Oct 17, 2018
View reviewed changes
sys/include/net/cord/epsim.h Outdated
* process
*
* @note Only available with @ref cord_epsim_standalone compiled in
* @note This function must only be called once (typically during system
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 17, 2018

Choose a reason for hiding this comment

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

I think this should rather be a @warning or @attention even.

Copy link
Copy Markdown
Contributor Author

@haukepetersen haukepetersen Oct 17, 2018

Choose a reason for hiding this comment

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

@warning seems sensible. Will also adapt that in cord/ep_standalone.h.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 17, 2018

addressed last comment.

@miri64 miri64 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 and removed Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 17, 2018
miri64
miri64 reviewed Oct 17, 2018
View reviewed changes
sys/include/net/cord/epsim.h Outdated
* registration with all responding RDs using the simple registration
* process
*
* @note Only available with @ref cord_epsim_standalone compiled in
Copy link
Copy Markdown
Member

@miri64 miri64 Oct 17, 2018

Choose a reason for hiding this comment

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

net_cord_epsim_standalone ;-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 17, 2018

You can squash immediately.

@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 17, 2018

done

@haukepetersen haukepetersen force-pushed the opt_cord_restructuring branch from 15ba5ef to 9e8d626 Compare October 17, 2018 13:03
@haukepetersen haukepetersen force-pushed the opt_cord_restructuring branch from 9e8d626 to 34fa61d Compare October 17, 2018 13:24
@haukepetersen
Copy link
Copy Markdown
Contributor Author

haukepetersen commented Oct 17, 2018

fixed doxygen issue and squashed.

@miri64 miri64 added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Oct 17, 2018
@miri64 miri64 merged commit 266f7ee into RIOT-OS:master Oct 17, 2018
@miri64 miri64 deleted the opt_cord_restructuring branch October 17, 2018 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 approved these changes

@smlng smlng Awaiting requested review from smlng

@bergzand bergzand Awaiting requested review from bergzand

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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@haukepetersen @miri64