OpenThread RPC DNS client #17859

konradderda · 2024-10-14T05:27:40Z

No description provided.

CLAassistant · 2024-10-14T05:27:45Z

All committers have signed the CLA.

NordicBuilder · 2024-10-14T05:28:54Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 14

Inputs:

Sources:

sdk-nrf: PR head: c8580e3185adc4a7e0eb60aa0d97f754d082a2ef

more details

sdk-nrf:

PR head: c8580e3185adc4a7e0eb60aa0d97f754d082a2ef
merge base: 6266884de899a254b7f09868d6ad4947d0dfc879
target head (main): 7f5d9d4569795fa90576034981ea043516fd3ec5
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 (16)

samples
│  ├── nrf_rpc
│  │  ├── protocols_serialization
│  │  │  ├── client
│  │  │  │  ├── src
│  │  │  │  │  │ ot_shell.c
subsys
│  ├── net
│  │  ├── openthread
│  │  │  ├── rpc
│  │  │  │  ├── Kconfig
│  │  │  │  ├── client
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  │ ot_rpc_dns_client.c
│  │  │  │  ├── common
│  │  │  │  │  ├── ot_rpc_common.c
│  │  │  │  │  ├── ot_rpc_common.h
│  │  │  │  │  │ ot_rpc_ids.h
│  │  │  │  ├── server
│  │  │  │  │  ├── CMakeLists.txt
│  │  │  │  │  │ ot_rpc_dns_client.c
tests
│  ├── subsys
│  │  ├── net
│  │  │  ├── openthread
│  │  │  │  ├── rpc
│  │  │  │  │  ├── client
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── dns_client_suite.c
│  │  │  │  │  │  │  │ thread_suite.c
│  │  │  │  │  ├── common
│  │  │  │  │  │  │ test_rpc_env.h
│  │  │  │  │  ├── server
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── common_fakes.c
│  │  │  │  │  │  │  ├── common_fakes.h
│  │  │  │  │  │  │  ├── dns_client_suite.c
│  │  │  │  │  │  │  │ thread_suite.c

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 535
✅ Integration tests
- ✅ test-fw-nrfconnect-rpc
- ✅ test-fw-nrfconnect-thread
- ✅ test-fw-nrfconnect-ps

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- 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-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

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

NordicBuilder · 2024-10-14T05:48:54Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

Damian-Nordic

Partial review

samples/nrf_rpc/protocols_serialization/client/src/ot_shell.c

subsys/net/openthread/rpc/Kconfig

subsys/net/openthread/rpc/common/ot_rpc_common.c

subsys/net/openthread/rpc/server/ot_rpc_dns_client.c

alxelax

I guess it requires a bit more work.
My main concern is

Some places are implemented on hybrid manner. They use both pure zcbor API with checking errors over nrf_rpc_serialize API. It should be done over pure zcbor with manual checkig of returned value or over nrf_rpc_serialize and then relay in checking error over this API. I'm afraid hybrid approach might not work.
Not all tests check that everything was sent as added.
Absence of server tests.

P.S. seems this EPIC is critical: https://nordicsemi.atlassian.net/browse/MRTNEXT-78
Presence of both ways of implementation made mess up author. We need to complete it asap.

alxelax · 2024-10-16T07:51:03Z

samples/nrf_rpc/protocols_serialization/client/src/ot_shell.c

+	uint32_t ttl;
+	char addr_string[NET_IPV6_ADDR_LEN];
+
+	shell_print(sh, "DNS resolve callback error: %u", error);


Could change it on shell_error and put under else?

samples/nrf_rpc/protocols_serialization/client/src/ot_shell.c

subsys/net/openthread/rpc/client/ot_rpc_dns_client.c

subsys/net/openthread/rpc/server/ot_rpc_dns_client.c

tests/subsys/net/openthread/rpc/client/src/dns_client_suite.c

alxelax · 2024-10-21T12:50:55Z

tests/subsys/net/openthread/rpc/client/src/dns_client_suite.c

+				   RPC_RSP(CBOR_DNS_QUERY_CONFIG));
+
+	config = otDnsClientGetDefaultConfig(NULL);
+


I guess each test shall have mock_nrf_rpc_tr_expect_done(); after execution ot API to check that everything has been sent as expected in mocked transport.

not solved yet

It actually has - I put it in common tc_cleanup(void *f) function called after every test case

tests/subsys/net/openthread/rpc/client/src/dns_client_suite.c

NordicBuilder · 2024-11-21T07:51:17Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

alxelax · 2024-12-05T11:31:54Z

Hi @konradderda, there are a couple of comments those have not been solved yet.

I dismiss my review since I'm on vacation until January to not block PR by my absence.

alxelax · 2024-12-05T11:36:23Z

@Damian-Nordic, could you control comments not solved yet those are important to your point of view?

Damian-Nordic

2 final nits, but feel free to address them after merge. Also, please remember to update the list of supported functions: https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/libraries/networking/ot_rpc.html

Damian-Nordic · 2024-12-17T08:39:25Z

tests/subsys/net/openthread/rpc/server/src/common_mocks.h

@@ -0,0 +1,9 @@
+/*


Przemek has recently added common_fakes.h. Can we merge this files?

Damian-Nordic · 2024-12-17T23:27:20Z

subsys/net/openthread/rpc/server/ot_rpc_dns_client.c

+static void ot_rpc_dns_client_browse(const struct nrf_rpc_group *group,
+				     struct nrf_rpc_cbor_ctx *ctx, void *handler_data)
+{
+	char service_name[OT_DNS_MAX_LABEL_SIZE];


OT_DNS_MAX_LABEL_SIZE may be too small. The service name can be a full DNS name, not just a single component.

Good point, fixed

This commit adds RPC client and server for DNS client API. Signed-off-by: Konrad Derda <[email protected]>

Add new 'ot test_*' commands for DNS client. Signed-off-by: Konrad Derda <[email protected]>

This commit add tests suite for OpenThread RPC DNS client Signed-off-by: Konrad Derda <[email protected]>

konradderda requested review from a team as code owners October 14, 2024 05:27

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 14, 2024

konradderda force-pushed the ot_rpc_dns_client branch from 1106614 to c5e66b1 Compare October 14, 2024 05:32

konradderda force-pushed the ot_rpc_dns_client branch 2 times, most recently from a847ad0 to cd345f8 Compare October 14, 2024 08:48

Damian-Nordic reviewed Oct 15, 2024

View reviewed changes

konradderda force-pushed the ot_rpc_dns_client branch from cd345f8 to 19d0d53 Compare October 17, 2024 08:56

alxelax previously requested changes Oct 21, 2024

View reviewed changes

konradderda force-pushed the ot_rpc_dns_client branch from 19d0d53 to 5361b04 Compare November 21, 2024 07:33

konradderda force-pushed the ot_rpc_dns_client branch 2 times, most recently from 2e7c8ef to 82632ab Compare December 5, 2024 09:57

konradderda force-pushed the ot_rpc_dns_client branch from 82632ab to b25f807 Compare December 16, 2024 10:44

Damian-Nordic approved these changes Dec 17, 2024

View reviewed changes

konradderda force-pushed the ot_rpc_dns_client branch from b25f807 to 70de528 Compare December 20, 2024 13:35

konradderda added 2 commits December 23, 2024 09:34

net: openthread: rpc: add DNS client serialization

6fa1da4

This commit adds RPC client and server for DNS client API. Signed-off-by: Konrad Derda <[email protected]>

net: openthread: rpc: add DNS client test commands to shell

15ae52e

Add new 'ot test_*' commands for DNS client. Signed-off-by: Konrad Derda <[email protected]>

konradderda force-pushed the ot_rpc_dns_client branch from 70de528 to bc8fbd3 Compare December 23, 2024 08:54

tests: subsys: net: openthread: add DNS client tests

c8580e3

This commit add tests suite for OpenThread RPC DNS client Signed-off-by: Konrad Derda <[email protected]>

konradderda force-pushed the ot_rpc_dns_client branch from bc8fbd3 to c8580e3 Compare December 23, 2024 10:05

nordicjm approved these changes Dec 23, 2024

View reviewed changes

nordicjm merged commit 9dd07c8 into nrfconnect:main Dec 23, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenThread RPC DNS client #17859

OpenThread RPC DNS client #17859

konradderda commented Oct 14, 2024

CLAassistant commented Oct 14, 2024 •

edited

Loading

NordicBuilder commented Oct 14, 2024 •

edited

Loading

NordicBuilder commented Oct 14, 2024

Damian-Nordic left a comment

alxelax left a comment

alxelax Oct 16, 2024

alxelax Dec 5, 2024

konradderda Dec 16, 2024

alxelax Oct 21, 2024

alxelax Dec 5, 2024

konradderda Dec 16, 2024

NordicBuilder commented Nov 21, 2024

alxelax commented Dec 5, 2024

alxelax commented Dec 5, 2024

Damian-Nordic left a comment

Damian-Nordic Dec 17, 2024

konradderda Dec 20, 2024

Damian-Nordic Dec 17, 2024

konradderda Dec 20, 2024

		RPC_RSP(CBOR_DNS_QUERY_CONFIG));

		config = otDnsClientGetDefaultConfig(NULL);

OpenThread RPC DNS client #17859

OpenThread RPC DNS client #17859

Conversation

konradderda commented Oct 14, 2024

CLAassistant commented Oct 14, 2024 • edited Loading

NordicBuilder commented Oct 14, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

NordicBuilder commented Oct 14, 2024

Damian-Nordic left a comment

Choose a reason for hiding this comment

alxelax 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NordicBuilder commented Nov 21, 2024

alxelax commented Dec 5, 2024

alxelax commented Dec 5, 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

Choose a reason for hiding this comment

CLAassistant commented Oct 14, 2024 •

edited

Loading

NordicBuilder commented Oct 14, 2024 •

edited

Loading