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.

Sign up for GitHub

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: Add function for single group initialization #1593

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+119 −41
Draft

nrf_rpc: Add function for single group initialization #1593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
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
146 changes: 105 additions & 41 deletions nrf_rpc/nrf_rpc.c
View file
Open in desktop
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:

sdk-nrfxlib/nrf_rpc/nrf_rpc.c

Line 420 in 4929937

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:

sdk-nrfxlib/nrf_rpc/nrf_rpc.c

Line 458 in 4929937

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 @@ -1091,23 +1114,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 @@ -1144,20 +1162,66 @@ 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;
}

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 •
edited
Loading

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;
}

NRF_RPC_ASSERT(group_count > 0);
if (initialized_group_count == group_count - 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you use waiting_group_count? If there are groups without NRF_RPC_FLAGS_WAIT_ON_INIT flag, the wait below will hang indefinitely. By the way, it's non-obvious that the function waits only if there's only one group that has not been initialized so far, so this should be documented or there should be a follow up filed to make the group initialization less coupled (so that each group can only wait for its own initialization completion).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I was trying to do here to here is to call the event_wait function only for the last group which was getting initialized. The function groups_init_event_wait checks for waiting threads so it will actually not hang. But on the other hand now that I look at it again there is a bug in this indeed. The initialized_group_count only increases for the threads which have the NRF_RPC_FLAGS_WAIT_ON_INIT flag enabled, so if there is one thread that doesn't then the wait will never get called. I will have to refactor this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so another issue is that if you have two groups and you execute nrf_rpc_init_group for both of them without a delay, you may end up in a situation that neither call will enter this if (because init packet for the first group may not arrive before you execute the second nrf_rpc_init_group). So my concern is that if the goal of this function is to make sure that all groups are initialized after you call this function for the last group, you may be surprised in some scenarios :).

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