Skip to content

Conversation

@MarekPorwisz
Copy link
Contributor

Added support for stopping and restoring the RPC communication.
See also nrfconnect/sdk-nrfxlib#1886

If the RPC server becomes unresponsive it needs to be reset. In order to do this apart from releasing all the client threads waiting, callback tables need to be cleared as well.
Added cleanup handler registration that clears entire callback table when RPC communication is stopped.

@MarekPorwisz MarekPorwisz requested review from a team as code owners October 21, 2025 13:48
@NordicBuilder NordicBuilder added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Oct 21, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 21, 2025

CI Information

To view the history of this post, click the 'edited' button above
Build number: 11

Inputs:

Sources:

sdk-nrf: PR head: 6439e00e806dbf27b44f9f90d1936e4f35e0d9cd
nrfxlib: PR head: 545ef5d19d2e1d1d7a9d4e0cf238c5164dfaac14

more details

sdk-nrf:

PR head: 6439e00e806dbf27b44f9f90d1936e4f35e0d9cd
merge base: 556216922de768e93a25661a58af70f2453375db
target head (main): e081cf5b33f6c691e43fde0bda257187c3fc5ec0
Diff

nrfxlib:

PR head: 545ef5d19d2e1d1d7a9d4e0cf238c5164dfaac14
merge base: 1d88c4484f24820c7cad542916080f409db9257d
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (5)
nrfxlib
│  ├── nrf_rpc
│  │  ├── include
│  │  │  │ nrf_rpc.h
│  │  │ nrf_rpc.c
samples
│  ├── nrf_rpc
│  │  ├── protocols_serialization
│  │  │  ├── client
│  │  │  │  ├── src
│  │  │  │  │  │ rpc_utils_shell.c
subsys
│  ├── nrf_rpc
│  │  │ nrf_rpc_cbkproxy.c
west.yml

Outputs:

Toolchain

Version: cfa6b06338
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:cfa6b06338_bba2ea5f2e

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 10
  • ✅ Integration tests
    • ✅ test_ble_nrf_config
    • ✅ test-fw-nrfconnect-ble_samples - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_cloud - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-rpc - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-rs - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-dfu - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-ps-main - Skipped: Job was skipped as it succeeded in a previous run
Disabled integration tests
    • test-fw-nrfconnect-nrf_lrcs_positioning
    • desktop52_verification
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread-main
    • test-low-level
    • test-sdk-audio
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-secdom-samples-public

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

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Oct 21, 2025

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
nrfxlib nrfconnect/sdk-nrfxlib@1d88c44 nrfconnect/sdk-nrfxlib@545ef5d (main) nrfconnect/[email protected]

All manifest checks OK

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

@github-actions
Copy link

You can find the documentation preview for this PR here.

@MarekPorwisz MarekPorwisz force-pushed the rpc_cleanup_callbacks branch 3 times, most recently from 08b0a7c to 957f474 Compare October 22, 2025 12:17
msg->data = data;
msg->len = len;
(void)k_condvar_signal(&msg->event);

Copy link
Contributor

Choose a reason for hiding this comment

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

accidental newline?

#if defined(CONFIG_NRF_RPC_UTILS_SYSTEM_HEALTH)
SHELL_CMD_ARG(system_health, NULL, "Get system health", cmd_system_health, 0, 0),
#endif
SHELL_CMD_ARG(rpc, NULL, "Control RPC subsystem ", cmd_rpc, 2, 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

Using rpc as subcommand would result in usage like: rpc rpc 1, maybe we can use something like control? e.g. rpc control enable/disable (these are parsed properly by shell_strtobool())

if (argc > 1) {
bool enable;

enable = shell_strtobool(argv[1], 10, &err);
Copy link
Contributor

Choose a reason for hiding this comment

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

The call can be moved to declaration of bool enable above


static int cmd_rpc(const struct shell *sh, size_t argc, char *argv[])
{
static bool enabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be initialized to false before the first call so if you call the command without any argument it will say that RPC is disabled despite it is succesfuly running?


static uint32_t next_free_in_slot;

void ot_rpc_cbproxy_clear(void *context)
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be static

@MarekPorwisz MarekPorwisz force-pushed the rpc_cleanup_callbacks branch from 957f474 to 885201f Compare October 22, 2025 13:11
@NordicBuilder NordicBuilder requested a review from a team October 22, 2025 13:11

static uint32_t next_free_in_slot;

static void ot_rpc_cbproxy_clear(void *context)
Copy link
Contributor

Choose a reason for hiding this comment

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

This feature is not limited to OT, so the naming is confusing.

@MarekPorwisz MarekPorwisz force-pushed the rpc_cleanup_callbacks branch from 885201f to b2d9731 Compare October 23, 2025 09:10
@NordicBuilder NordicBuilder requested a review from a team October 23, 2025 09:10
Added functions to stop and restore RPC communication. Stopping the
communication automatically times out all pending tasks and prevents new
communication. Init messages, errors and acks are still allowed.
This is intended to allow smooth recovery after failures.
Additionally all calbacks that use common API may be cleared.

Signed-off-by: Marek Porwisz <[email protected]>
@MarekPorwisz MarekPorwisz force-pushed the rpc_cleanup_callbacks branch from b2d9731 to 6439e00 Compare October 24, 2025 07:42
@NordicBuilder NordicBuilder requested a review from a team October 24, 2025 07:43
@NordicBuilder NordicBuilder removed the DNM label Oct 24, 2025
@rlubos rlubos merged commit d349cf8 into nrfconnect:main Oct 24, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. manifest manifest-nrfxlib

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants