Skip to content

nrf_rpc: Add function for single group initialization #1593

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions nrf_rpc/include/nrf_rpc.h
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,20 @@ void nrf_rpc_set_bound_handler(nrf_rpc_group_bound_handler_t bound_handler);
*/
int nrf_rpc_init(nrf_rpc_err_handler_t err_handler);

/** @brief Initialize a single nRF RPC group
*
* Initialize a single nRF RPC group. It can be used to initialize all groups using this function
* and avoid calling nrf_rpc_init. It is also possible to initialize a subset of the groups and
* then call nrf_rpc_init.
* This function doesn't support providing an error handler, a default error handler will be used
* if an erorr occurs. If a custom error handler is needed, use nrf_rpc_init.
*
* @param group Group to initialize
*
* @return 0 on success or negative error code.
*/
int nrf_rpc_init_group(const struct nrf_rpc_group *group);

/** @brief Send a command and provide callback to handle response.
*
* @param group Group that command belongs to.
Expand Down
162 changes: 121 additions & 41 deletions nrf_rpc/nrf_rpc.c
Original file line number Diff line number Diff line change
Expand Up @@ -411,49 +411,51 @@ static void internal_tx_handler(void)
}
}

static int transport_init(nrf_rpc_tr_receive_handler_t receive_cb)
static int transport_init_single(nrf_rpc_tr_receive_handler_t receive_cb, const struct nrf_rpc_group *group)
{
int err = 0;
void *iter;
const struct nrf_rpc_group *group;
const struct nrf_rpc_tr *transport = group->transport;
struct nrf_rpc_group_data *data = group->data;
int err;

for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,
const struct nrf_rpc_group)) {
const struct nrf_rpc_tr *transport = group->transport;
struct nrf_rpc_group_data *data = group->data;
NRF_RPC_ASSERT(transport != NULL);

NRF_RPC_ASSERT(transport != NULL);
if (group->data->transport_initialized) {
return 0;
}

/* Initialize all dependencies of `receive_handler` before calling the transport
* init to avoid possible data race if `receive_handler` was invoked before this
* function was completed. */
if (auto_free_rx_buf(transport)) {
err = nrf_rpc_os_event_init(&data->decode_done_event);
if (err < 0) {
continue;
}
/* Initialize all dependencies of `receive_handler` before calling the transport
* init to avoid possible data race if `receive_handler` was invoked before this
* function was completed. */
if (auto_free_rx_buf(transport)) {
err = nrf_rpc_os_event_init(&data->decode_done_event);
if (err < 0) {
return err;
}
}

err = transport->api->init(transport, receive_cb, NULL);
if (err) {
NRF_RPC_ERR("Failed to initialize transport, err: %d", err);
continue;
}
err = transport->api->init(transport, receive_cb, NULL);
if (err) {
NRF_RPC_ERR("Failed to initialize transport, err: %d", err);
return err;
}

group->data->transport_initialized = true;
group->data->transport_initialized = true;

if (group->flags & NRF_RPC_FLAGS_INITIATOR) {
err = group_init_send(group);
if (err) {
NRF_RPC_ERR("Failed to send group init packet for group id: %d strid: %s err: %d",
data->src_group_id, group->strid, err);
continue;
}
if (group->flags & NRF_RPC_FLAGS_INITIATOR) {
err = group_init_send(group);
if (err) {
NRF_RPC_ERR("Failed to send group init packet for group id: %d strid: %s err: %d",
data->src_group_id, group->strid, err);
return err;
}
}

/* Group initialization errors are not propagated to the caller. */
err = 0;
return 0;
}

static int groups_init_event_wait(void)
{
int err = 0;

if (waiting_group_count > 0) {
err = nrf_rpc_os_event_wait(&groups_init_event, CONFIG_NRF_RPC_GROUP_INIT_WAIT_TIME);
Expand All @@ -465,6 +467,27 @@ static int transport_init(nrf_rpc_tr_receive_handler_t receive_cb)
return err;
}

static int transport_init_all(nrf_rpc_tr_receive_handler_t receive_cb)
{
void *iter;
const struct nrf_rpc_group *group;

for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,
const struct nrf_rpc_group)) {

transport_init_single(receive_cb, group);
Copy link
Contributor

Choose a reason for hiding this comment

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

error is not checked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional though, because I saw in the previous implementation they didn't return an error when a single initialization failed.
The logic here:

for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,

Should be the same with the logic in the function transport_init_single basically.

The error that I return in this function is the logic which exists here:

if (waiting_group_count > 0) {

}

/* Group initialization errors are not propagated to the caller. */
return groups_init_event_wait();
}

static void default_err_handler(const struct nrf_rpc_err_report *report)
{
NRF_RPC_ERR("nRF RPC error %d ocurred. See nRF RPC logs for more details", report->code);
nrf_rpc_os_fatal_error();
}

/* ======================== Receiving Packets ======================== */

/* Find in array and execute command or event handler */
Expand Down Expand Up @@ -1098,23 +1121,18 @@ void nrf_rpc_set_bound_handler(nrf_rpc_group_bound_handler_t bound_handler)
global_bound_handler = bound_handler;
}

int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)
static int nrf_rpc_prepare_init(void)
{
int err;
int i;
void *iter;
const struct nrf_rpc_group *group;
uint8_t group_id = 0;
uint8_t wait_count = 0;

NRF_RPC_DBG("Initializing nRF RPC module");

if (is_initialized) {
return 0;
}

global_err_handler = err_handler;

for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,
const struct nrf_rpc_group)) {
struct nrf_rpc_group_data *data = group->data;
Expand Down Expand Up @@ -1151,20 +1169,82 @@ int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)
return err;
}

for (i = 0; i < CONFIG_NRF_RPC_CMD_CTX_POOL_SIZE; i++) {
for (size_t i = 0; i < CONFIG_NRF_RPC_CMD_CTX_POOL_SIZE; i++) {
cmd_ctx_pool[i].id = i;
err = nrf_rpc_os_msg_init(&cmd_ctx_pool[i].recv_msg);
if (err < 0) {
return err;
}
}

err = transport_init(receive_handler);
global_err_handler = default_err_handler;

is_initialized = true;

return 0;
}

static bool all_groups_transport_finished(void){
void *iter;
const struct nrf_rpc_group *group;

for (NRF_RPC_AUTO_ARR_FOR(iter, group, &nrf_rpc_groups_array,
const struct nrf_rpc_group)) {
if (!group->data->transport_initialized) {
return false;
}
}

return true;
}

int nrf_rpc_init_group(const struct nrf_rpc_group *group)
{
int err = nrf_rpc_prepare_init();
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that this code is not thread-safe. Is there an assumption that nrf_rpc_init_group must be called from the main thread only? If so, I think we should document it. Otherwise, nrf_rpc_prepare_init may end up being executed from two threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that this is not thread safe, but is it not the case that the whole nrf_pc is not thread safe? The variable is_initialized was static before, I didn't change that. I am not too familiar with the library but it I see many static objects being manipulated so I thought that it is not thread safe. Am I missing something?

Copy link
Contributor

@Damian-Nordic Damian-Nordic Mar 27, 2025

Choose a reason for hiding this comment

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

Calling nRF RPC commands is thread-safe as far as I understand but the init is not. @doki-nordic has more background but my impression is that the authors originally assumed that that the init would be called once. If we now allow more flexibility it becomes harder to enforce this, especially if this is not documented.

if (err < 0) {
return err;
}

err = transport_init_single(receive_handler, group);
if (err < 0) {
return err;
}

/* If all the groups have their transport initialized this is the last call
* to nrf_rpc_init_group.
*/
if (all_groups_transport_finished()) {
return groups_init_event_wait();
}

return 0;
}

int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

The err_handler is never assigned to global handler.

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, I changed that.

{
int err;

/* Everything is initialized, nothing to do here */
if (group_count > 0 && group_count == initialized_group_count) {
return 0;
}

NRF_RPC_DBG("Initializing nRF RPC module");

err = nrf_rpc_prepare_init();
if (err < 0) {
return err;
}

/* The nrf_rpc_prepare_init sets a default error handler,
* override it here with the one passed as parameter */
global_err_handler = err_handler;

err = transport_init_all(receive_handler);
if (err < 0) {
return err;
}

is_initialized = true;
NRF_RPC_DBG("Done initializing nRF RPC module");

return err;
Expand Down
Loading