net: add CoRE RD lookup client implementation #10222
Conversation
PeterKietzmann
added
Area: network
miri64
added
the
Type: new feature
There was a problem hiding this comment.
Thanks for taking this on!
For now only some small structural findings on the side...
But in general, I think we need to re-think the high level interface a bit to make this module easier to use! In the current state, the interface is IMHO too 'low-level'. So maybe we should think first about what we would want as return data from a lookup function and then structure the user interface around that.
E.g. considering the endpoint lookup: what I as a user actually would want from that function, is an easy to work with list of endpoints fitting the given criteria. So getting simply a link-format string is not exactly easy to work with, right?!
I would imagine we could build the API around some basic principles, like:
- initiate the 'details' of a certain RD server into a struct (e.g. the RDs address, discovery of lookup interface(s), ...)
-> use that struct as argument for the actual lookup functions, e.g.
typedef struct {
sock_udp_ep_t ep;
const char *lookif;
} cord_lc_rd_t;
int cord_lc_init(cord_lc_rd_t *rd, const sock_udp_ep_t *remote, const char *lookif);- split the interface into a set of functions for each specific lookup type (possibly convenience/inline functions mapping to some base function)
- for each lookup type, provide
- a raw function (allowing to specify the content format, e.g. LINK, JSON, CBOR...?!)
- a 'high-level' function returning the parsed results
just a rough draft:
int cord_lc_ep_raw(const cord_lc_rd_t *rd, FILTER, uint8_t content_format, void *buf, size_t buf_size);
// FILTER: how we pass the filter to the function is still to be determined...
// content_format: data format we want to get from the RD
// buf: holding the raw response
// buf_size: number of bytes that we are prepared to receive
int cord_lc_ep(const cord_lc_rd_t *rd, FILTER, cord_lc_ep_t *ep, size_t limit);
// FILTER: see above
// ep: array with endpoint elements, also needs further refinement. Maybe we pass a raw buffer and expect something like a list of EPs that is put into that buffer?!
// limit: maximum number of elements we are prepared to receiveThis basically reflects my state of thoughts towards the lookup client API that I had in my head so far. So bottom line: I think it would be good to come up with an easy to work with API, possibly providing both low- and high-level access to the lookup data.
Does this make sense?
Makefile.dep
Outdated
| ifneq (,$(filter cord_lc,$(USEMODULE))) | ||
| USEMODULE += cord_common | ||
| USEMODULE += core_thread_flags | ||
| USEMODULE += gcoap |
There was a problem hiding this comment.
as this is a duplication with cord_ep, we should factor these dependencies out into the base module cord
There was a problem hiding this comment.
You mean new cord module? Does it make sense to put this into cord_common instead and cord_ep and cord_lc can use cord_common as dependency?
There was a problem hiding this comment.
So we could add this below (and remove the corresponding USEMODULE statements accordingly):
ifneq (,$(filter cord_lc cord_ep,$(USEMODULE)))
ifneq (,$(filter shell_commands,$(USEMODULE)))
USEMODULE += sock_util
endif
endif
Also we might want to move the gcoap 'include' into cord_common, as you suggested.
There was a problem hiding this comment.
I pushed a commit (37bd548) addressing this comment but I'm not sure I understand your last comment correctly. Please correct me if there is anything wrong with it.
There was a problem hiding this comment.
Ok, let me try to illustrate this a little clearer. We might want to do the following:
- move
gcoapdep intocord_common(as it is used by all three,cord_epsim,cord_ep, andcord_lc) - factor out the shared modules that are deps for both
cord_epandcord_lt, something like this:
ifneq (,$(filter cord_lc,$(USEMODULE)))
USEMODULE += clif
endif
ifneq (,$(filter cord_lc cord_ep,$(USEMODULE)))
USEMODULE += core_thread_flags
USEMODULE += cord_common
ifneq (,$(filter shell_commands,$(USEMODULE)))
USEMODULE += sock_util
endif
endif
ifneq (,$(filter cord_common,$(USEMODULE)))
USEMODULE += gcoap
USEMODULE += fmt
USEMODULE += luid
endif
This way there is not duplicate deps declaration for those modules anymore.
sys/include/net/cord/config.h
Outdated
| */ | ||
| #ifndef CORD_LC_RES_BUF_LEN | ||
| #define CORD_LC_RES_BUF_LEN (128) | ||
| #endif |
There was a problem hiding this comment.
IMHO this should not be defined globally, but be specified by the user of the API...
pokgak
force-pushed
the
pr/add-cord-lc
branch
from
7115640 to
2a1401f
Compare
pokgak
changed the title
Agree with you on that.
Both typedef struct {
sock_udp_ep_t ep;
#ifdef MODULE_CORD_EP
const char *regif;
#endif
#ifdef MODULE_CORD_LC
const char *res_lookif;
...
#endif
} cord_rd_t;
int cord_common_rd_init(cord_rd_t *rd, const sock_udp_ep_t *remote);This can then be used in discover function of both
Yeah, splitting the interface to raw and low-level functions makes more sense.
Makes perfect sense! I will work on this next week and hopefully be ready for next round of review. =) |
True, but I would like to keep both submodules more independent of each other. Also, for the current version of As of now, I tend to keep the
Very nice, looking forward to it! |
pokgak
force-pushed
the
pr/add-cord-lc
branch
3 times, most recently
from
9dfc9b1 to
25ac14a
Compare
| /* ignore unrecognized attributes */ | ||
| } | ||
|
|
||
| static size_t _parse_res(const char *source, size_t len, cord_lc_res_t *results, |
There was a problem hiding this comment.
_parse_res and _parse_ep basically do the same thing - parses the result buffer from _lookup_raw and saves the endpoints/resources to respective structs. I tried to combine the two functions together so that there is less redundant code but cannot find a good solution. Suggestions are welcomed.
|
@haukepetersen I pushed the new code, now based on your suggestion and ready for next round of review. |
sys/include/net/cord/lc.h
Outdated
| * @return CORD_LC_TIMEOUT if lookup timed out | ||
| * @return CORD_LC_ERR on any other internal error | ||
| */ | ||
| ssize_t cord_lc_res_raw(const cord_lc_rd_t *rd, uint8_t content_format, |
There was a problem hiding this comment.
An array of clif_param_t parameter structs from #11189 could be used as the input filter.
There was a problem hiding this comment.
Yup, lots of the code in this PR can be replaced by clif e.g. clif_t instead of cord_lc_res_t and cord_lc_ep_t as both of them is just links actually. I think there's a lot more than can replaced.
sys/include/net/cord/lc.h
Outdated
| * @return CORD_LC_TIMEOUT if lookup timed out | ||
| * @return CORD_LC_ERR on any other internal error | ||
| */ | ||
| ssize_t cord_lc_res(const cord_lc_rd_t *rd, cord_lc_res_t *resources, size_t limit); |
There was a problem hiding this comment.
This function is resource intensive. I would consider an iterative function on the collected string from coap_lc_res_raw().
cord_lc_read_res(const char *source, clif_t *link)
where link is the output parameter. However, this function is similar to coap_decode_links() from #11189, so not that useful.
On the other hand, I would not be surprised to see a block based response from the RD. How to incorporate the possibility into the API? Rather than separately call cord_lc_res_raw() first, maybe something like this:
cord_lc_read_res(const cord_lc_rd_t *rd, char *input, unsigned input_len, clif_t *link)
where the cord_lc_rd_t param could be extended to include the block number. That means it could transparently retrieve the next block.
There was a problem hiding this comment.
This function is resource intensive. I would consider an iterative function on the collected string from coap_lc_res_raw().
Sorry, I don't quite understand here. What part of the function, do you mean, is resource intensive? Internally, it currently uses _lookup_raw, which are also used by cord_lc_res_raw, and then parses the result and put it in cord_lc_res_t.
I do however find that using iterative function on the collected string is a good idea. As clif_decode_link only decodes one link per call, cord_lc_read_res can be used as a wrapper to it to decode all the links at once and returns it to the user.
On the other hand, I would not be surprised to see a block based response from the RD.
Block based response would be nice actually. The default GCOAP_PDU_BUF_SIZE is too small even for an RD ep with only 3 resources. Currently, I just set the buffer size bigger but using block option would solve this, I think.
I found #11056 but haven't take a deeper look at it and doesn't really know how to use the API and integrate it here yet, but as you understands it better than me, I'll keep your suggestion in mind ;)
There was a problem hiding this comment.
As clif_decode_link only decodes one link per call, cord_lc_read_res can be used as a wrapper to it to decode all the links at once and returns it to the user.
That's the resource intensive part. ;-) By iterative I meant that it would be less intensive for the user to repeatedly call cord_lc_read_res() with the output from the query, and receive one clif_t link per call. Maybe the cord_lc_rd_t parameter could remember the last character read from the query result, as well as remember the last block number it retrieved.
Let's wait for clif, and I'd be happy to discuss more.
There was a problem hiding this comment.
Aah okay. That's what you mean.
Maybe the cord_lc_rd_t parameter could remember the last character read from the query result, as well as remember the last block number it retrieved.
There are actually page and count parameters that can be used for the lookup. So this should be easier to implement. Just as a note for now.
There was a problem hiding this comment.
Thanks for the link. I actually saw that earlier but thought, "why do we need this when we already have block"? Looking at it more closely, I see that the interface is super simple for a client and eliminates the need for the client to stitch 'sliced' links together from block's fixed size response. On the other hand, if packet space is really limited, block provides certainty in the quantity of data received.
It might actually be worthwhile to ask the core mailing list about the reasoning. If we're concerned about data size, use of CBOR would be valuable here, but I guess that use is orthogonal to how the data is sliced. I also agree with you that we don't need to resolve this issue today.
|
@pokgak @kb2ma @haukepetersen any reason why this is in rest? I think it is worth completing. |
|
Sorry forgot to update the current status. I'm waiting for #11189 to be merged and so that it can be used for parsing link format. My current implementation is a bit hacky IMHO. |
pokgak
force-pushed
the
pr/add-cord-lc
branch
from
25ac14a to
bfff39d
Compare
|
I updated this PR to use the CLIF module (#11189 ) to parse link format response from RD server. This PR is currently ready for review. I'm trying to write some script/Dockerfile so that this can be tested directly without having to setup the test dependencies first. I'll update this later when this is ready. |
|
@haukepetersen @kb2ma are you available to move this forward? |
|
It's going to be a little while before I can look at this. The Wakaama package and @pokgak's integration of DTLS with gcoap are higher priorities right now. If only I could spend more time on RIOT, things would happen faster. ;-) |
|
Updated the OP with steps for testing. |
|
Fixed the static-test error. Rebased and squashed directly. |
|
@haukepetersen are your comments addressed? |
|
Just tried to give this a quick test: seems there is still something not quite in order with the test application: I guess it should not segfault?! :-) Did run this under |
Makefile.dep
Outdated
| ifneq (,$(filter cord_lc,$(USEMODULE))) | ||
| USEMODULE += cord_common | ||
| USEMODULE += core_thread_flags | ||
| USEMODULE += gcoap |
There was a problem hiding this comment.
Ok, let me try to illustrate this a little clearer. We might want to do the following:
- move
gcoapdep intocord_common(as it is used by all three,cord_epsim,cord_ep, andcord_lc) - factor out the shared modules that are deps for both
cord_epandcord_lt, something like this:
ifneq (,$(filter cord_lc,$(USEMODULE)))
USEMODULE += clif
endif
ifneq (,$(filter cord_lc cord_ep,$(USEMODULE)))
USEMODULE += core_thread_flags
USEMODULE += cord_common
ifneq (,$(filter shell_commands,$(USEMODULE)))
USEMODULE += sock_util
endif
endif
ifneq (,$(filter cord_common,$(USEMODULE)))
USEMODULE += gcoap
USEMODULE += fmt
USEMODULE += luid
endif
This way there is not duplicate deps declaration for those modules anymore.
I guess the command can be confusing. |
|
Could you please rebase (and retest)? After rebasing to master, the PR does not build anymore: Error |
|
And when using your branch as is, the example application is still segfaulting: |
|
I cannot reproduce the segfault on the current version (using aiocoap-rd version 0.4b3): |
|
And after rebasing I got cannot connect to the RD, this also happens on master (e8389a5). I'll open an issue for this. |
|
Rebased.
Using a global address (e.g |
|
Rebased to include fix for #14399. Retested again. Log: |
|
@haukepetersen Can you share with me how you setup your testing environment? I'll try to see if I can reproduce the segfault then. |
|
Of course. I just re-tested, the segfault still happens as before. I did exactly the following:
I just did run my test again, with the same result of RIOT segfaulting. But then I updated my Anyway, as the tests run fine now, I'd say we should go ahead with this PR as is. |
|
@pokgak please squash |
|
Squashed.
Agree. That might be something worth looking into in the future. For now we'll leave it as a note here. |
|
|
|
Thanks everyone! |
Contribution description
This PR add CoRE Resource Directory lookup client functionality to RIOT. The code basically done and should work already but I cannot test this yet because problem on my laptop.
This PR adds RD lookup client functionality to RIOT.
Testing
For testing we will need 3 components:
aiocoap-rdexample from the development version of aicoap. Run following to install, taken from the [aiocoap install page]:examples/cord_ep.examples/cord_lc.Steps:
aiocoap-rdRD_SERVER_ADDRESSwith link-local address oftapbr0interface: