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

e-rk · 2024-05-20T07:14:54Z

This PR consists of two commits:

nrf_rpc: Two-way handshake initialization
nrf_rpc: add NOWAIT group type

See descriptions for more details.

Damian-Nordic

Have some comments, mostly to improve comments, but I like the idea overall :)

nrf_rpc/include/nrf_rpc.h

nrf_rpc/nrf_rpc.c

nrf_rpc/include/nrf_rpc.h

anhmolt · 2024-05-21T14:27:52Z

nrf_rpc/nrf_rpc.c

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


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?

Yes, I think so. Thanks for catching it.

doki-nordic

Approved with minor suggestions.

doki-nordic · 2024-05-22T11:23:21Z

nrf_rpc/nrf_rpc.c

+		bound_handler(*group);
+	}
+
+	if (first_init && wait_on_init && ++initialized_group_count == waiting_group_count) {


Do not use ++x operator inside "if" expression (it should not have side-effects).

Removed the side effect from if.

doki-nordic · 2024-05-22T11:29:47Z

nrf_rpc/nrf_rpc.c

+	group_data = (*group)->data;
+	first_init = group_data->dst_group_id == NRF_RPC_ID_UNKNOWN;
+	group_data->dst_group_id = hdr->src_group_id;
+	bool wait_on_init = (*group)->wait_on_init;


Local variable declarations should be at the beginning.

Moved the declaration to the beginning.

e-rk · 2024-05-22T13:30:31Z

I have applied @Damian-Nordic suggestion in a separate commit. Let me know if you prefer groups to have single flags field. If yes, I will squash the commit. If not, I will remove it.

nrf_rpc/include/nrf_rpc.h

KAGA164 · 2024-05-23T08:37:54Z

nrf_rpc/include/nrf_rpc.h

+ *                       to reset the local state.
+ * @param _initiator     The group is the initiator.
+ */
+#define NRF_RPC_GROUP_DEFINE_NOWAIT(_name, _strid, _transport, _ack_handler,      \


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__()

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?

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

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

Done as suggested.

nrf_rpc/nrf_rpc.c

nrf_rpc/include/nrf_rpc.h

nrf_rpc/Kconfig

nrf_rpc/include/nrf_rpc.h

e-rk · 2024-05-23T09:40:37Z

@peknis applied your suggestions.

Implement two-way handshake nRF RPC initialization: When an initialization packet with the unknown destination group ID is received, reply to that with an initialization packet with the right destination group ID. This makes it possible to re-establish the link even if either peer has rebooted, or has booted with a delay. Also, allow to configure infinite group init timeout. Signed-off-by: Damian Krolik <[email protected]>

The NOWAIT group type does not block nrf_rpc_init until binding completion. The user can define regular and NOINIT groups simultaneously, but nrf_rpc_init will only wait for regular groups to complete the binding. The NOINIT group can be assigned a callback, which notifies that the group binding has been completed. If a remote peer resets and binds again, the callback will be executed again. When `initiator` flag is set, the nrf_rpc will initiate group binding when nrf_rpc_init is called. There must be at least one initiator on either side of the IPC channel. Both peers can be the initiator at the same time. Signed-off-by: Rafał Kuźnia <[email protected]>

NordicBuilder mentioned this pull request May 20, 2024

manifest: sdk-nrfxlib: nrf_rpc: two-way handshake and NOWAIT group type nrfconnect/sdk-nrf#15377

Closed

e-rk force-pushed the nrf-rpc-improvements branch from cc1588c to 31f79ef Compare May 20, 2024 07:16

NordicBuilder mentioned this pull request May 20, 2024

nrf_rpc: two-way handshake, NOWAIT groups and binding robustness nrfconnect/sdk-nrf#15378

Merged

e-rk marked this pull request as ready for review May 20, 2024 07:32

e-rk requested review from doki-nordic and KAGA164 as code owners May 20, 2024 07:32

e-rk requested a review from Damian-Nordic May 20, 2024 07:32

e-rk force-pushed the nrf-rpc-improvements branch from 31f79ef to 2e6aa11 Compare May 20, 2024 13:59

Damian-Nordic reviewed May 21, 2024

View reviewed changes

nrf_rpc/include/nrf_rpc.h Outdated Show resolved Hide resolved

nrf_rpc/include/nrf_rpc.h Outdated Show resolved Hide resolved

nrf_rpc/nrf_rpc.c Show resolved Hide resolved

nrf_rpc/include/nrf_rpc.h Outdated Show resolved Hide resolved

e-rk force-pushed the nrf-rpc-improvements branch from 2e6aa11 to 4e84aa2 Compare May 21, 2024 11:26

anhmolt reviewed May 21, 2024

View reviewed changes

e-rk force-pushed the nrf-rpc-improvements branch from 4e84aa2 to e577715 Compare May 22, 2024 09:59

doki-nordic approved these changes May 22, 2024

View reviewed changes

e-rk force-pushed the nrf-rpc-improvements branch from e577715 to 216842a Compare May 22, 2024 12:38

e-rk force-pushed the nrf-rpc-improvements branch 3 times, most recently from 5d9dc1f to 00de5c9 Compare May 22, 2024 13:32

Damian-Nordic approved these changes May 22, 2024

View reviewed changes

e-rk force-pushed the nrf-rpc-improvements branch from 00de5c9 to aa9b85b Compare May 23, 2024 06:22

KAGA164 requested a review from peknis May 23, 2024 08:46

KAGA164 reviewed May 23, 2024

View reviewed changes

peknis requested changes May 23, 2024

View reviewed changes

e-rk force-pushed the nrf-rpc-improvements branch from 84b8597 to 47293c1 Compare May 23, 2024 09:40

peknis approved these changes May 23, 2024

View reviewed changes

e-rk force-pushed the nrf-rpc-improvements branch 2 times, most recently from 3ff8922 to 3c477d9 Compare May 23, 2024 10:25

KAGA164 approved these changes May 23, 2024

View reviewed changes

e-rk added this to the 2.7.0 milestone May 23, 2024

Damian-Nordic and others added 2 commits May 24, 2024 13:05

e-rk force-pushed the nrf-rpc-improvements branch from 3c477d9 to 0924311 Compare May 24, 2024 11:05

rlubos merged commit dd0aa77 into nrfconnect:main May 24, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

e-rk commented May 20, 2024

Damian-Nordic left a comment

anhmolt May 21, 2024

e-rk May 22, 2024

e-rk May 22, 2024

doki-nordic left a comment

doki-nordic May 22, 2024

e-rk May 22, 2024

doki-nordic May 22, 2024

e-rk May 22, 2024

e-rk commented May 22, 2024

KAGA164 May 23, 2024

e-rk May 23, 2024 •

edited

Loading

KAGA164 May 23, 2024

KAGA164 May 23, 2024

e-rk May 23, 2024

e-rk commented May 23, 2024

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

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

Conversation

e-rk commented May 20, 2024

Damian-Nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doki-nordic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-rk commented May 22, 2024

Choose a reason for hiding this comment

e-rk May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-rk commented May 23, 2024

e-rk May 23, 2024 •

edited

Loading