Skip to content
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 IPC Service transport #7510

Merged
merged 6 commits into from
Jun 14, 2022

Conversation

KAGA164
Copy link
Contributor

@KAGA164 KAGA164 commented May 5, 2022

It adds nRF RPC IPC Service no-copy transport. Also it alignes the nRF RPC library port and all client libraries or samples with introduced multi-instance transport. It means that each nRF RPC group can use different transport.

@KAGA164 KAGA164 requested a review from kapi-no May 5, 2022 13:45
@github-actions github-actions bot added changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. doc-required PR must not be merged without tech writer approval. manifest labels May 5, 2022
@KAGA164 KAGA164 requested review from grochu and maje-emb May 5, 2022 13:45
@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 5, 2022

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
nrfxlib nrfconnect/sdk-nrfxlib@53e83ab nrfconnect/sdk-nrfxlib@a5d317a (main) nrfconnect/sdk-nrfxlib@53e83abe..a5d317a6

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@KAGA164 KAGA164 requested a review from peknis May 6, 2022 07:30
doc/nrf/libraries/nrf_rpc/nrf_rpc_ipc.rst Outdated Show resolved Hide resolved
@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from 09ed215 to e75c358 Compare May 10, 2022 12:14
@KAGA164 KAGA164 marked this pull request as ready for review May 10, 2022 12:15
@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from e75c358 to c7aed8a Compare May 10, 2022 12:36
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 10, 2022
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 10, 2022

Integration test specification

Test Module File based changes Manually selected West overwrite
test-fw-nrfconnect-ble X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-nfc X
test-fw-nrfconnect-nrf_crypto X
test-fw-nrfconnect-rpc X
test-fw-nrfconnect-tfm X
test-fw-nrfconnect-zigbee X
test-sdk-find-my X
test-sdk-homekit X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from c7aed8a to 66787cd Compare May 11, 2022 09:52
Copy link
Contributor

@maje-emb maje-emb left a comment

Choose a reason for hiding this comment

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

Please add alignment for internal files in Bluetooth RPC.

include/nrf_rpc/nrf_rpc_ipc.h Outdated Show resolved Hide resolved
subsys/nrf_rpc/nrf_rpc_ipc.c Outdated Show resolved Hide resolved
subsys/nrf_rpc/nrf_rpc_ipc.c Outdated Show resolved Hide resolved
@maje-emb
Copy link
Contributor

Running the entropy sample, from time to time there is a issue with memory allocation:
<err> nrf_rpc_ipc: Failed to allocate Tx buffer. ipc_service_get_tx_buffer returned err: -12

@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from 66787cd to c3ae2ae Compare May 24, 2022 11:27
@KAGA164
Copy link
Contributor Author

KAGA164 commented May 24, 2022

Please add alignment for internal files in Bluetooth RPC.

I totally forgot about this part of the API. Thanks, done.

@KAGA164
Copy link
Contributor Author

KAGA164 commented May 24, 2022

Running the entropy sample, from time to time there is a issue with memory allocation: <err> nrf_rpc_ipc: Failed to allocate Tx buffer. ipc_service_get_tx_buffer returned err: -12

It is fixed now

@KAGA164 KAGA164 requested review from maje-emb and peknis May 24, 2022 11:29
doc/nrf/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/nrf_rpc/nrf_rpc_ipc.rst Outdated Show resolved Hide resolved
doc/nrf/libraries/nrf_rpc/nrf_rpc_ipc.rst Outdated Show resolved Hide resolved
include/nrf_rpc/nrf_rpc_ipc.h Outdated Show resolved Hide resolved
include/nrf_rpc/nrf_rpc_ipc.h Outdated Show resolved Hide resolved
include/nrf_rpc/nrf_rpc_ipc.h Outdated Show resolved Hide resolved
subsys/nrf_rpc/Kconfig Outdated Show resolved Hide resolved
@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from c3ae2ae to f8b2f00 Compare May 24, 2022 12:22
LOG_ERR("ipc_service_send_nocpy returned err: %d", err);
LOG_ERR("Dropping Tx buffer");

err2 = ipc_service_drop_tx_buffer(&endpoint->ept, data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently ipc_rpmsg_static_vrings backend does not support drop the buffer: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/ipc/ipc_service/backends/ipc_rpmsg_static_vrings.c#L577-L582
Doesn't this cause a potential issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be. It looks like there is outgoing work on adding support for it in the OpenAMP: OpenAMP/open-amp#361

@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from f8b2f00 to 0cfd801 Compare May 25, 2022 14:19
@KAGA164
Copy link
Contributor Author

KAGA164 commented May 25, 2022

@maje-emb I switched back to use IPC Service without nocopy functionality. When the missed API will be added to OpenAMP then I will use the nocopy mechanism

@KAGA164 KAGA164 changed the title nRF RPC IPC Service no-copy transport nRF RPC IPC Service transport May 26, 2022
Copy link
Contributor

@maje-emb maje-emb left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@grochu grochu left a comment

Choose a reason for hiding this comment

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

Please find a couple of comments.

@@ -777,6 +777,11 @@ Other libraries

* Fixed a bug where the random key would not be deleted from RAM after being generated and written.

* :ref:`lib_nrf_rpc`:

* This library can use different transports for the nRF RPC group.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This library can use different transports for the nRF RPC group.
* This library can use different transport implementation for each nRF RPC group.

or something similar to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I like your proposition here

/** @brief Extern nRF RPC IPC Service transport declaration.
*
* Can be used in header files. It is useful when several nRF RPC group
* are defined amongst different source file but they can share the same
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* are defined amongst different source file but they can share the same
* are defined amongst different source files but they can share the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


/** @brief Sent response to a command as a void. */
void ser_rsp_send_void(void);
/** @brief Sent response to a command as a void.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @brief Sent response to a command as a void.
/** @brief Send response to a command as a void.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in all places

@@ -1318,14 +1345,15 @@ NRF_RPC_CBOR_CMD_DECODER(bt_rpc_grp, bt_conn_auth_cb_register_on_remote,
BT_CONN_AUTH_CB_REGISTER_ON_REMOTE_RPC_CMD,
bt_conn_auth_cb_register_on_remote_rpc_handler, NULL);


Copy link
Contributor

Choose a reason for hiding this comment

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

Is this blank line needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes removed

range 1 1000
default 100
help
Miliseconds to wait for endpoint binding.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Miliseconds to wait for endpoint binding.
Time in miliseconds to wait for endpoint binding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

help
Miliseconds to wait for endpoint binding.
This timeout depends on the time to initialize all the cores
the nRF RPC is going to communicate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
the nRF RPC is going to communicate.
the nRF RPC is going to communicate with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

default 100
help
Miliseconds to wait for endpoint binding.
This timeout depends on the time to initialize all the cores
Copy link
Contributor

Choose a reason for hiding this comment

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

How about saying "remote devices" instead of "cores"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ipc_config->used = false;

LOG_ERR("IPC endpoint bond timeout");
return translate_error(-NRF_EPIPE);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like this error code needed translation (looks like native nRF RPC code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed

* NRF_RPC_GROUP_DEFINE(group_2, "Group_2", &nrf_rpc_2, NULL, NULL, NULL);
*
* @param[in] _name nRF RPC IPC Service transport instance name.
* @param[in] _ipc The instance used for the IPC Service to transfer data between cores.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we say "CPUs" maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done

@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from 0cfd801 to d4552c3 Compare June 10, 2022 08:53
@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch 2 times, most recently from 9e4321b to 73c42c1 Compare June 13, 2022 11:49
west.yml Outdated
@@ -109,7 +109,7 @@ manifest:
- name: nrfxlib
repo-path: sdk-nrfxlib
path: nrfxlib
revision: 53e83abe25d1e07443e7179e1cd012bc0238e11a
revision: pull/728/head
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update SHA: a5d317a62db9a57d19a1d580346c0cb233cfb7e6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

KAGA164 added 6 commits June 14, 2022 11:44
It aligns the nRF RPC IPC Service transport and
nRF RPC porting layer to use multi-instance transport
instead of single tranport basing on the template.

NCSDK-15026

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
It updates the entropy sample to use the nRF RPC
multi-instance transport.

NCSDK-15026

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
This alignes the Bluetooth serialization with changes in
the nRF RPC.

NCSDK-15026

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
It adds a documentation for the nRF RPC IPC Service
transport.

NCSDK-15026

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
Update changelog with the new nRF RPC IPC transport and
support for different transports per each group.

NCSDK-15026

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
Updates nrfxlib version with changes
in the nRF RPC library.

NCSDK-15026

Signed-off-by: Kamil Gawor <Kamil.Gawor@nordicsemi.no>
@KAGA164 KAGA164 force-pushed the nrf_rpc_multi_instance_transport branch from 73c42c1 to 58727cc Compare June 14, 2022 09:45
@NordicBuilder NordicBuilder removed the DNM label Jun 14, 2022
@rlubos rlubos merged commit 8aeab18 into nrfconnect:main Jun 14, 2022
@greg-fer greg-fer mentioned this pull request Sep 7, 2022
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval. manifest manifest-nrfxlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants