Nrf rpc sm_ipt backend by dchat-nordic · Pull Request #525 · nrfconnect/sdk-nrfxlib

dchat-nordic · 2021-07-27T07:53:44Z

Added support for sm_ipt backend
Updated templates, updated Kconfig
to support sm_ipt.

Depends on #524

Signed-off-by: Dominik Chat dominik.chat@nordicsemi.no

Co-authored-by: Dominik Kilian Dominik.Kilian@nordicsemi.no

doki-nordic · 2021-08-23T13:04:00Z

nrf_rpc/include/nrf_rpc.h

 #define _NRF_RPC_HEADER_SIZE 4

+#ifndef NRF_RPC_TR_MAX_HEADER_SIZE
+#define NRF_RPC_TR_MAX_HEADER_SIZE 0


Don't define them here. Transport header file is responsible for defining it.
The same for NRF_RPC_TR_AUTO_FREE_RX_BUF.

This is ifdef def, so if transport have not defined anything the values of macros are set to default.

doki-nordic · 2021-08-23T13:11:02Z

nrf_rpc/nrf_rpc.c

-#include "nrf_rpc_os.h"
+#include <nrf_rpc.h>
+#include <nrf_rpc_tr.h>
+#include <nrf_rpc_os.h>


Don't change this.

I tried to make it consistent with nrt_rpc.h

doki-nordic · 2021-08-23T13:11:42Z

nrf_rpc/nrf_rpc.c

 	return err;
 }

+


Remove unnecessary line

doki-nordic · 2021-08-23T13:21:26Z

nrf_rpc/sm_ipt_backend.c

@@ -0,0 +1,3 @@
+#include "sm_ipt_backend.h"


Add copyright comment at the beginning of a file.

doki-nordic · 2021-08-23T13:23:44Z

nrf_rpc/template/nrf_rpc_os_tmpl.h

 * @return         0 on success or negative error code.
 */
-int nrf_rpc_os_init(nrf_rpc_os_work_t callback);
+int nrf_rpc_os_init(nrf_rpc_os_work_t callback, void *user_data);


user_data is not longer needed. Right?

doki-nordic · 2021-08-23T13:25:00Z

nrf_rpc/Kconfig

+	help
+	  If enabled selects shared memory as a transport layer for nRF PRC.
+	  It requires additional API implemented in nrf_rpc_os.h file.
+


We are using SM IPT module, so we can use its name instead of just shared memory.

doki-nordic · 2021-08-23T13:29:30Z

nrf_rpc/include/nrf_rpc.h

 *                     a newly allocated packet buffer.
 * @param[in]  _len    Requested length of the packet.
 */
+#ifdef nrf_rpc_tr_alloc_tx_buf


Maybe it is better to explain this #ifdef, because it is not obvious.

/* If nrf_rpc_tr_alloc_tx_buf is a macro, put it directly, because it may allocate memory on stack. */

Comment added

doki-nordic · 2021-08-23T13:32:35Z

nrf_rpc/nrf_rpc.c


 static void cmd_ctx_free(struct nrf_rpc_cmd_ctx *ctx)
 {
+	NRF_RPC_DBG("Command context %d free", ctx->id);


This line can be deleted, it is not related to this PR.

KAGA164 · 2021-08-27T06:46:41Z

nrf_rpc/CMakeLists.txt

@@ -1,14 +1,15 @@
 #
-# Copyright (c) 2020 Nordic Semiconductor
+# Copyright (c) 2020-2021 Nordic Semiconductor


I don;t need to edit this line

KAGA164 · 2021-08-27T06:47:14Z

nrf_rpc/Kconfig

@@ -1,5 +1,5 @@
 #
-# Copyright (c) 2020 Nordic Semiconductor
+# Copyright (c) 2020-2021 Nordic Semiconductor


You don't need to edit this line

KAGA164 · 2021-08-27T06:47:23Z

nrf_rpc/include/nrf_rpc.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020 Nordic Semiconductor ASA
+ * Copyright (c) 2020-2021 Nordic Semiconductor ASA


KAGA164 · 2021-08-27T06:47:32Z

nrf_rpc/include/nrf_rpc_cbor.h

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020 Nordic Semiconductor ASA
+ * Copyright (c) 2020-2021 Nordic Semiconductor ASA


No needed. Please remove licence changes also in other files

KAGA164 · 2021-08-27T06:50:49Z

nrf_rpc/CMakeLists.txt


 zephyr_library_sources_ifdef(CONFIG_NRF_RPC_CBOR nrf_rpc_cbor.c)
+zephyr_library_sources_ifdef(CONFIG_NRF_RPC_TR_SHMEM sm_ipt_backend.c)
+zephyr_library_sources_ifdef(CONFIG_NRF_RPC nrf_rpc.c)


Do you have any reason to add this source conditionally ?

Suggested change

zephyr_library_sources_ifdef(CONFIG_NRF_RPC nrf_rpc.c)

zephyr_library_sources(nrf_rpc.c)

KAGA164 · 2021-08-27T06:51:51Z

nrf_rpc/nrf_rpc.c

+{
+	uint8_t *packet;
+
+	NRF_RPC_ASSERT(len < 0x70000000);


Magic number ?

Size limit check. Changed to define now.

KAGA164 · 2021-08-27T06:52:21Z

nrf_rpc/nrf_rpc.c

+	NRF_RPC_ASSERT(len < 0x70000000);
+
+	nrf_rpc_tr_alloc_tx_buf(&packet, _NRF_RPC_HEADER_SIZE + len);
+	return &packet[_NRF_RPC_HEADER_SIZE];


Blank line before return ?

KAGA164 · 2021-08-27T06:55:10Z

nrf_rpc/nrf_rpc_cbor.c


 void _nrf_rpc_cbor_prepare(struct nrf_rpc_cbor_ctx *ctx, size_t len)
 {
+#ifndef nrf_rpc_tr_alloc_tx_buf


Where is this defined ?

Transport layer provides this function (nrf_rpc_tr.h). Here it is as a template (*_tmpl.h)

SM_IPT is OS independent shared memory communication protocol, which works on shared memory and provides os-porting header templates. Signed-off-by: Dominik Chat <dominik.chat@nordicsemi.no> Co-authored-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>

Added support for sm_ipt backend Updated templates, updated Kconfig to support sm_ipt. Signed-off-by: Dominik Chat <dominik.chat@nordicsemi.no> Co-authored-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>

tejlmand

Why is this in nrfxlib and not in sdk-nrf/lib ?

I don't see any prebuilt libraries in this PR, which is usually the case for nrfxlib libs.

doki-nordic · 2021-12-06T09:39:32Z

Why is this in nrfxlib and not in sdk-nrf/lib ?

I don't see any prebuilt libraries in this PR, which is usually the case for nrfxlib libs.

@tejlmand, just FYI, the nrfxlib is not only for prebuild libraries. It is place for modules that are independent from Zephyr/NCS, so user can take some module from nrfxlib and use it e.g. in FreeRTOS or baremetal. Other example is nrf_rpc in this repo. If this PR will be reopen then it will be exactly for such case.

Edit: After reconsideration, I see now that it is not very clear if nrfxlib is only for prebuild libraries or not. I see no better place to put this PR (and nrf_rpc which is similar case), so I am still thinking that this RP should go into nrfxlib if reopened.

dchat-nordic mentioned this pull request Jul 27, 2021

sm_ipt: Provide zephyr porting layer nrfconnect/sdk-nrf#5191

Closed

dchat-nordic force-pushed the nrf_rpc_sm_ipt_backend branch from 3ca3257 to ff05ccf Compare July 28, 2021 07:04

dchat-nordic force-pushed the nrf_rpc_sm_ipt_backend branch 5 times, most recently from 4cf1673 to 3cef3ac Compare August 9, 2021 13:58

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Aug 9, 2021

dchat-nordic force-pushed the nrf_rpc_sm_ipt_backend branch from 3cef3ac to d74c71a Compare August 20, 2021 08:22

doki-nordic requested changes Aug 23, 2021

View reviewed changes

dchat-nordic force-pushed the nrf_rpc_sm_ipt_backend branch 3 times, most recently from c887491 to 636f1b5 Compare August 26, 2021 08:03

dchat-nordic marked this pull request as ready for review August 26, 2021 08:04

dchat-nordic requested review from KAGA164, b-gent, carlescufi and tejlmand as code owners August 26, 2021 08:04

dchat-nordic requested a review from doki-nordic August 26, 2021 08:04

KAGA164 suggested changes Aug 27, 2021

View reviewed changes

dchat-nordic force-pushed the nrf_rpc_sm_ipt_backend branch from 636f1b5 to 6cc6ece Compare August 30, 2021 08:38

nrf_rpc: Added sm_ipt backend support

0167f65

Added support for sm_ipt backend Updated templates, updated Kconfig to support sm_ipt. Signed-off-by: Dominik Chat <dominik.chat@nordicsemi.no> Co-authored-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>

dchat-nordic force-pushed the nrf_rpc_sm_ipt_backend branch from 6cc6ece to 0167f65 Compare August 31, 2021 09:47

dchat-nordic requested a review from KAGA164 August 31, 2021 11:51

tejlmand requested changes Nov 29, 2021

View reviewed changes

dchat-nordic closed this Dec 6, 2021

	zephyr_library_sources_ifdef(CONFIG_NRF_RPC nrf_rpc.c)
	zephyr_library_sources(nrf_rpc.c)

Conversation

dchat-nordic commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

doki-nordic commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dchat-nordic commented Jul 27, 2021 •

edited

Loading

doki-nordic commented Dec 6, 2021 •

edited

Loading