-
Notifications
You must be signed in to change notification settings - Fork 317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Nrf rpc sm_ipt backend #525
Conversation
3ca3257
to
ff05ccf
Compare
4cf1673
to
3cef3ac
Compare
3cef3ac
to
d74c71a
Compare
@@ -28,6 +28,14 @@ extern "C" { | |||
/* Internal definition used in the macros. */ | |||
#define _NRF_RPC_HEADER_SIZE 4 | |||
|
|||
#ifndef NRF_RPC_TR_MAX_HEADER_SIZE | |||
#define NRF_RPC_TR_MAX_HEADER_SIZE 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't define them here. Transport header file is responsible for defining it.
The same for NRF_RPC_TR_AUTO_FREE_RX_BUF
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ifdef def, so if transport have not defined anything the values of macros are set to default.
nrf_rpc/nrf_rpc.c
Outdated
#include "nrf_rpc_os.h" | ||
#include <nrf_rpc.h> | ||
#include <nrf_rpc_tr.h> | ||
#include <nrf_rpc_os.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make it consistent with nrt_rpc.h
nrf_rpc/nrf_rpc.c
Outdated
@@ -767,6 +768,7 @@ int nrf_rpc_init(nrf_rpc_err_handler_t err_handler) | |||
return err; | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary line
@@ -0,0 +1,3 @@ | |||
#include "sm_ipt_backend.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add copyright comment at the beginning of a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
nrf_rpc/template/nrf_rpc_os_tmpl.h
Outdated
* | ||
* @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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
user_data
is not longer needed. Right?
help | ||
If enabled selects shared memory as a transport layer for nRF PRC. | ||
It requires additional API implemented in nrf_rpc_os.h file. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using SM IPT module, so we can use its name instead of just shared memory
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -230,9 +238,17 @@ struct nrf_rpc_err_report { | |||
* a newly allocated packet buffer. | |||
* @param[in] _len Requested length of the packet. | |||
*/ | |||
#ifdef nrf_rpc_tr_alloc_tx_buf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added
nrf_rpc/nrf_rpc.c
Outdated
@@ -95,6 +95,7 @@ static struct nrf_rpc_cmd_ctx *cmd_ctx_alloc(void) | |||
|
|||
static void cmd_ctx_free(struct nrf_rpc_cmd_ctx *ctx) | |||
{ | |||
NRF_RPC_DBG("Command context %d free", ctx->id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line can be deleted, it is not related to this PR.
c887491
to
636f1b5
Compare
nrf_rpc/CMakeLists.txt
Outdated
@@ -1,14 +1,15 @@ | |||
# | |||
# Copyright (c) 2020 Nordic Semiconductor | |||
# Copyright (c) 2020-2021 Nordic Semiconductor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don;t need to edit this line
nrf_rpc/Kconfig
Outdated
@@ -1,5 +1,5 @@ | |||
# | |||
# Copyright (c) 2020 Nordic Semiconductor | |||
# Copyright (c) 2020-2021 Nordic Semiconductor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to edit this line
nrf_rpc/include/nrf_rpc.h
Outdated
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright (c) 2020 Nordic Semiconductor ASA | |||
* Copyright (c) 2020-2021 Nordic Semiconductor ASA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No needed
nrf_rpc/include/nrf_rpc_cbor.h
Outdated
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright (c) 2020 Nordic Semiconductor ASA | |||
* Copyright (c) 2020-2021 Nordic Semiconductor ASA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No needed. Please remove licence changes also in other files
nrf_rpc/CMakeLists.txt
Outdated
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any reason to add this source conditionally ?
zephyr_library_sources_ifdef(CONFIG_NRF_RPC nrf_rpc.c) | |
zephyr_library_sources(nrf_rpc.c) |
nrf_rpc/nrf_rpc.c
Outdated
{ | ||
uint8_t *packet; | ||
|
||
NRF_RPC_ASSERT(len < 0x70000000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Size limit check. Changed to define now.
NRF_RPC_ASSERT(len < 0x70000000); | ||
|
||
nrf_rpc_tr_alloc_tx_buf(&packet, _NRF_RPC_HEADER_SIZE + len); | ||
return &packet[_NRF_RPC_HEADER_SIZE]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank line before return ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
@@ -227,6 +227,9 @@ void _nrf_rpc_cbor_proxy_handler(const uint8_t *packet, size_t len, | |||
|
|||
void _nrf_rpc_cbor_prepare(struct nrf_rpc_cbor_ctx *ctx, size_t len) | |||
{ | |||
#ifndef nrf_rpc_tr_alloc_tx_buf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this defined ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
636f1b5
to
6cc6ece
Compare
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>
6cc6ece
to
0167f65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 Edit: After reconsideration, I see now that it is not very clear if |
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