[riot-notifications] [RIOT-OS/RIOT] net: add CoRE RD lookup client implementation (#10222)

Hauke Petersen notifications at github.com
Mon May 18 17:06:26 CEST 2020

@haukepetersen requested changes on this pull request.

I just see this one little optimization potential for the dependency declarations, and of course the segfaulting... Else I am ok with proceeding.

> @@ -773,6 +773,15 @@ ifneq (,$(filter cord_ep,$(USEMODULE)))
+ifneq (,$(filter cord_lc,$(USEMODULE)))
+  USEMODULE += cord_common
+  USEMODULE += core_thread_flags
+  USEMODULE += gcoap

Ok, let me try to illustrate this a little clearer. We might want to do the following:
- move `gcoap` dep into `cord_common` (as it is used by all three, `cord_epsim`, `cord_ep`, and `cord_lc`)
- factor out the shared modules that are deps for both `cord_ep` and `cord_lt`, something like this:
ifneq (,$(filter cord_lc,$(USEMODULE)))
  USEMODULE += clif

ifneq (,$(filter cord_lc cord_ep,$(USEMODULE)))
  USEMODULE += core_thread_flags
  USEMODULE += cord_common
  ifneq (,$(filter shell_commands,$(USEMODULE)))
    USEMODULE += sock_util

ifneq (,$(filter cord_common,$(USEMODULE)))
  USEMODULE += gcoap
  USEMODULE += fmt
  USEMODULE += luid

This way there is not duplicate deps declaration for those modules anymore.

> +static size_t _res_buf_len;
+static char _rd_lookif[NANOCOAP_URI_MAX];
+static mutex_t _mutex = MUTEX_INIT;
+static volatile thread_t *_waiter;
+static uint8_t buf[BUFSIZE];
+static void _lock(void)
+    mutex_lock(&_mutex);
+    _waiter = sched_active_thread;
+static int _sync(void)

It would still be nice not to duplicate this code, but then again I don't see this as a show stopper for now. So lets postpone this step until after this PR is merged...

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200518/7e383f42/attachment.htm>

More information about the notifications mailing list