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: two-way handshake and NOWAIT group type #1335

Merged
merged 2 commits into from
May 24, 2024
Merged

nrf_rpc: two-way handshake and NOWAIT group type #1335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9d096e
nrf_rpc: Two-way handshake initialization
Damian-Nordic May 9, 2024
0924311
nrf_rpc: add NOWAIT group type
e-rk May 20, 2024
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
8 changes: 3 additions & 5 deletions nrf_rpc/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,11 @@ config NRF_RPC_CMD_CTX_POOL_SIZE
threads in both local and remote pool.

config NRF_RPC_GROUP_INIT_WAIT_TIME
int "Group initialization timeout"
int "Group initialization timeout in milliseconds"
default 1000
range 10 5000
help
The time period to wait for the remote cores to send group init responses with
destination group ids.
The time is given in miliseconds.
The number of milliseconds to wait for the remote cores to send group
init packets with destination group IDs. Set to -1 to wait forever.

config NRF_RPC_THREAD_POOL_SIZE
int "Number of threads in local thread pool"
Expand Down
133 changes: 111 additions & 22 deletions nrf_rpc/include/nrf_rpc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,18 @@ extern "C" {
/** @brief Special value to indicate that ID is unknown or irrelevant. */
#define NRF_RPC_ID_UNKNOWN 0xFF

/** @brief Flag indicating that the group does not block on initialization. */
#define NRF_RPC_FLAGS_WAIT_ON_INIT BIT(0)

/** @brief Flag indicating that the peer must initiate group binding. */
#define NRF_RPC_FLAGS_INITIATOR BIT(1)

/** @brief Helper macro for conditional flag initialization. */
#define NRF_RPC_FLAG_COND(_cond, _flag) ((_cond) ? (_flag) : 0UL)

/* Forward declaration. */
struct nrf_rpc_err_report;
struct nrf_rpc_group;

/** @brief Type of packet.
*
Expand Down Expand Up @@ -96,6 +106,14 @@ typedef void (*nrf_rpc_ack_handler_t)(uint8_t id, void *handler_data);
*/
typedef void (*nrf_rpc_err_handler_t)(const struct nrf_rpc_err_report *report);

/** @brief Callback called when the command group is bound.
*
* @see NRF_RPC_GROUP_DEFINE
*
* @param group Pointer to the bound group.
*/
typedef void (*nrf_rpc_group_bound_handler_t)(const struct nrf_rpc_group *group);

/* Structure used internally to define registered command or event decoder. */
struct _nrf_rpc_decoder {
uint8_t id;
Expand Down Expand Up @@ -127,6 +145,8 @@ struct nrf_rpc_group {
void *ack_handler_data;
const char *strid;
nrf_rpc_err_handler_t err_handler;
nrf_rpc_group_bound_handler_t bound_handler;
const uint32_t flags;
};

/** @brief Error report.
Expand All @@ -151,6 +171,58 @@ struct nrf_rpc_err_report {
enum nrf_rpc_packet_type packet_type;
};

/** @brief Internal macro for parametrizing nrf_rpc groups.
*
* @param _name Symbol name of the group.
* @param _strid String containing a unique identifier of the group. Naming
* conventions are the same as with C symbol name. Groups on local
* and remote must have the same unique identifier.
* @param _transport Group transport. It is used by group to communicate with
* a remote processor.
* @param _ack_handler Handler of type @ref nrf_rpc_ack_handler_t, called when
* ACK was received after event completion. Can be NULL if
* the group does not want to receive ACK notifications.
* @param _ack_data Opaque pointer for the `_ack_handler`.
* @param _err_handler Handler of type @ref nrf_rpc_err_handler_t, called when
* an error occurred in context of this group. Can be NULL if
* the group does not want to receive error notifications.
* @param _bound_handler Handler of type @ref nrf_rpc_group_bound_handler_t, called
* when the group was successfuly bound. The callback is called
* each time the remote peer binds to the group. This can be used
* to detect a remote peer reset and can be used by the application
* to reset the local state.
* @param _wait_on_init The group does not block until it is bound.
* @param _initiator The group is the initiator.
*/
#define NRF_RPC_GROUP_DEFINE_INTERNAL__(_name, _strid, _transport, _ack_handler, \
_ack_data, _err_handler, _bound_handler, \
_wait_on_init, _initiator) \
NRF_RPC_AUTO_ARR(NRF_RPC_CONCAT(_name, _cmd_array), \
"cmd_" NRF_RPC_STRINGIFY(_name)); \
NRF_RPC_AUTO_ARR(NRF_RPC_CONCAT(_name, _evt_array), \
"evt_" NRF_RPC_STRINGIFY(_name)); \
\
static struct nrf_rpc_group_data NRF_RPC_CONCAT(_name, _group_data) = { \
.src_group_id = NRF_RPC_ID_UNKNOWN, \
.dst_group_id = NRF_RPC_ID_UNKNOWN, \
.transport_initialized = false, \
}; \
\
NRF_RPC_AUTO_ARR_ITEM(const struct nrf_rpc_group, _name, "grp", \
_strid) = { \
.cmd_array = &NRF_RPC_CONCAT(_name, _cmd_array), \
.evt_array = &NRF_RPC_CONCAT(_name, _evt_array), \
.data = &NRF_RPC_CONCAT(_name, _group_data), \
.ack_handler = _ack_handler, \
.ack_handler_data = _ack_data, \
.strid = _strid, \
.transport = _transport, \
.err_handler = _err_handler, \
.bound_handler = _bound_handler, \
.flags = NRF_RPC_FLAG_COND(_wait_on_init, NRF_RPC_FLAGS_WAIT_ON_INIT) \
| NRF_RPC_FLAG_COND(_initiator, NRF_RPC_FLAGS_INITIATOR), \
}

/** @brief Define a group of commands and events.
*
* @param _name Symbol name of the group.
Expand All @@ -168,28 +240,45 @@ struct nrf_rpc_err_report {
*/
#define NRF_RPC_GROUP_DEFINE(_name, _strid, _transport, _ack_handler, _ack_data, \
_err_handler) \
NRF_RPC_AUTO_ARR(NRF_RPC_CONCAT(_name, _cmd_array), \
"cmd_" NRF_RPC_STRINGIFY(_name)); \
NRF_RPC_AUTO_ARR(NRF_RPC_CONCAT(_name, _evt_array), \
"evt_" NRF_RPC_STRINGIFY(_name)); \
\
static struct nrf_rpc_group_data NRF_RPC_CONCAT(_name, _group_data) = { \
.src_group_id = NRF_RPC_ID_UNKNOWN, \
.dst_group_id = NRF_RPC_ID_UNKNOWN, \
.transport_initialized = false, \
}; \
\
NRF_RPC_AUTO_ARR_ITEM(const struct nrf_rpc_group, _name, "grp", \
_strid) = { \
.cmd_array = &NRF_RPC_CONCAT(_name, _cmd_array), \
.evt_array = &NRF_RPC_CONCAT(_name, _evt_array), \
.data = &NRF_RPC_CONCAT(_name, _group_data), \
.ack_handler = _ack_handler, \
.ack_handler_data = _ack_data, \
.strid = _strid, \
.transport = _transport, \
.err_handler = _err_handler, \
}
NRF_RPC_GROUP_DEFINE_INTERNAL__(_name, _strid, _transport, _ack_handler, \
_ack_data, _err_handler, NULL, true, \
true) \

/** @brief Define a non-blocking group of commands and events.
*
* The NOWAIT group does not block the @ref nrf_rpc_init until binding completion.
* When the NOWAIT group is bound, the @ref nrf_rpc_group_bound_handler_t will
* be called. The NOWAIT group can have two roles: initiator and follower.
* The initiator initiates the endpoint binding. The follower waits for the initiator
* to bind the group. Both peers can be initiators, but there must always be at least
* one on either side of the IPC channel.
*
* @param _name Symbol name of the group.
* @param _strid String containing a unique identifier of the group. Naming
* conventions are the same as with C symbol name. Groups on local
* and remote must have the same unique identifier.
* @param _transport Group transport. It is used by group to communicate with
* a remote processor.
* @param _ack_handler Handler of type @ref nrf_rpc_ack_handler_t, called when
* ACK was received after event completion. Can be NULL if
* the group does not want to receive ACK notifications.
* @param _ack_data Opaque pointer for the `_ack_handler`.
* @param _err_handler Handler of type @ref nrf_rpc_err_handler_t, called when
* an error occurred in context of this group. Can be NULL if
* the group does not want to receive error notifications.
* @param _bound_handler Handler of type @ref nrf_rpc_group_bound_handler_t, called
* when the group was successfuly bound. The callback is called
* each time the remote peer binds to the group. This can be used
* to detect a remote peer reset and can be used by the application
* to reset the local state.
* @param _initiator The group is the initiator.
*/
#define NRF_RPC_GROUP_DEFINE_NOWAIT(_name, _strid, _transport, _ack_handler, \
Copy link
Contributor

Choose a reason for hiding this comment

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

This macro looks almost the same like NRF_RPC_GROUP_DEFINE You can consider simplification by creating, local macro and use it under these two. For example NRF_RPC_GROUP_DEFINE_INTERNAL__()

Copy link
Contributor Author

@e-rk e-rk May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure, then the user could use the internal macro to create a group that is neither NOWAIT or normal. Do we want to encourage that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean to keep both macros and use internal macro under them to avoid duplication in macro body

Copy link
Contributor

Choose a reason for hiding this comment

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

I think when user use something which has internal in name then he does it on his own responsibility

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 as suggested.

_ack_data, _err_handler, _bound_handler, \
_initiator) \
NRF_RPC_GROUP_DEFINE_INTERNAL__(_name, _strid, _transport, _ack_handler, \
_ack_data, _err_handler, _bound_handler, \
false, _initiator) \

/** @brief Extern declaration of a group.
*
Expand Down
87 changes: 63 additions & 24 deletions nrf_rpc/nrf_rpc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ static struct nrf_rpc_os_event groups_init_event;

/* Number of groups */
static uint8_t group_count;
static uint8_t waiting_group_count;

static uint8_t initialized_group_count;

Expand Down Expand Up @@ -297,7 +298,7 @@ static int group_init_send(const struct nrf_rpc_group *group)
hdr.type = NRF_RPC_PACKET_TYPE_INIT;
hdr.id = 0;
hdr.src_group_id = group->data->src_group_id;
hdr.dst_group_id = NRF_RPC_ID_UNKNOWN;
hdr.dst_group_id = group->data->dst_group_id;

len = strlen(group->strid) + NRF_RPC_PROTOCOL_VERSION_FIELD_SIZE;

Expand Down Expand Up @@ -344,7 +345,7 @@ static inline bool packet_validate(const uint8_t *packet)

static int transport_init(nrf_rpc_tr_receive_handler_t receive_cb)
{
int err;
int err = 0;
void *iter;
const struct nrf_rpc_group *group;

Expand All @@ -369,17 +370,25 @@ static int transport_init(nrf_rpc_tr_receive_handler_t receive_cb)
}

group->data->transport_initialized = true;
err = group_init_send(group);
if (err) {
NRF_RPC_ERR("Failed to send group init packet for group id: %d strid: %s",
data->src_group_id, group->strid);
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);
continue;
}
}
}

err = nrf_rpc_os_event_wait(&groups_init_event, CONFIG_NRF_RPC_GROUP_INIT_WAIT_TIME);
if (err) {
NRF_RPC_ERR("Not all groups are ready to use.");
/* Group initialization errors are not propagated to the caller. */
err = 0;

if (waiting_group_count > 0) {
err = nrf_rpc_os_event_wait(&groups_init_event, CONFIG_NRF_RPC_GROUP_INIT_WAIT_TIME);
if (err) {
NRF_RPC_ERR("Not all groups are ready to use.");
}
}

return err;
Comment on lines +387 to 394
Copy link
Contributor

Choose a reason for hiding this comment

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

In case waiting_group_count is zero, the err value from either transport->api->init(), nrf_rpc_os_event_init() or group_init_send() (in the last round of the for-loop) will be returned.

Should err be zeroed before the waiting_group_count > 0 conditional?

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, I think so. Thanks for catching it.

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

Expand Down Expand Up @@ -518,8 +527,7 @@ static bool protocol_version_check(const struct init_packet_data *init_data)

return false;
}
static const struct nrf_rpc_group *remote_group_init(uint8_t src_group_id, const char *strid,
size_t strid_len)
static const struct nrf_rpc_group *group_from_strid(const char *strid, size_t strid_len)
{
void *iter;
const struct nrf_rpc_group *group;
Expand All @@ -528,16 +536,10 @@ static const struct nrf_rpc_group *remote_group_init(uint8_t src_group_id, const
const struct nrf_rpc_group)) {
if ((strid_len == strlen(group->strid)) &&
(memcmp(strid, group->strid, strid_len) == 0)) {
group->data->dst_group_id = src_group_id;
NRF_RPC_DBG("Found corresponding local group. Remote id: %d, Local id: %d",
src_group_id, group->data->dst_group_id);

return group;
}
}

NRF_RPC_ERR("Remote group does not match local group");

return NULL;
}

Expand Down Expand Up @@ -578,6 +580,9 @@ static int init_packet_handle(struct header *hdr, const struct nrf_rpc_group **g
{
int err;
struct init_packet_data init_data = {0};
struct nrf_rpc_group_data *group_data;
bool first_init;
bool wait_on_init;

*group = NULL;

Expand All @@ -597,19 +602,48 @@ static int init_packet_handle(struct header *hdr, const struct nrf_rpc_group **g
}

/* Check for the corresponding local group and initialize it. */
*group = remote_group_init(hdr->src_group_id, init_data.strid, init_data.strid_len);
*group = group_from_strid(init_data.strid, init_data.strid_len);
if (*group == NULL) {
NRF_RPC_ERR("Remote group does not match local group");
NRF_RPC_ASSERT(false);
return -NRF_EFAULT;
}

initialized_group_count++;
if (initialized_group_count == group_count) {
/* All group are initialized. */
nrf_rpc_os_event_set(&groups_init_event);
group_data = (*group)->data;
first_init = group_data->dst_group_id == NRF_RPC_ID_UNKNOWN;
group_data->dst_group_id = hdr->src_group_id;
wait_on_init = (*group)->flags & NRF_RPC_FLAGS_WAIT_ON_INIT;
nrf_rpc_group_bound_handler_t bound_handler = (*group)->bound_handler;

NRF_RPC_DBG("Found corresponding local group. Remote id: %d, Local id: %d",
hdr->src_group_id, group_data->src_group_id);

if (bound_handler != NULL) {
Damian-Nordic marked this conversation as resolved.
Show resolved Hide resolved
bound_handler(*group);
}

return 0;
if (first_init && wait_on_init) {
++initialized_group_count;

if (initialized_group_count == waiting_group_count) {
/* All groups are initialized. */
nrf_rpc_os_event_set(&groups_init_event);
}
}

if (hdr->dst_group_id == NRF_RPC_ID_UNKNOWN) {
/*
* If remote processor does not know our group id, send an init packet back,
* since it might have missed our original init packet.
*/
err = group_init_send(*group);
if (err) {
NRF_RPC_ERR("Failed to send group init packet for group id: %d strid: %s",
group_data->src_group_id, (**group).strid);
}
}

return err;
}

/* Callback from transport layer that handles incoming. */
Expand Down Expand Up @@ -966,6 +1000,7 @@ int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)
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");

Expand All @@ -986,9 +1021,13 @@ int nrf_rpc_init(nrf_rpc_err_handler_t err_handler)
NRF_RPC_DBG("Group '%s' has id %d", group->strid, group_id);
data->src_group_id = group_id;
group_id++;
if (group->flags & NRF_RPC_FLAGS_WAIT_ON_INIT) {
wait_count++;
}
}

group_count = group_id;
waiting_group_count = wait_count;

memset(&cmd_ctx_pool, 0, sizeof(cmd_ctx_pool));

Expand Down
Loading