OpenThread HDLC RCP communication support #75986

xavraz · 2024-07-17T08:45:37Z

This PR adds support to an RCP design, the core of OpenThread lives on the host processor connected to an RCP radio controller over a HDLC interface.

OpenThread HDLC RCP communication added with its hdlc_api interface APIs and a NXP driver
Add of spinel hdlc rcp interface for an OpenThread rcp host
Adapt openthread.c file to support RCP interface

Tested on a board using NXP's RW612 component.

github-actions · 2024-07-17T08:46:15Z

Hello @xavraz, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

canisLupus1313 · 2024-07-17T09:11:28Z

@xavraz thanks for your contribution.
It seems that You have included some of the Openthread files in the Zephyr code. I would say would be better to include form openthread module.
More over I don't think that C++ code is accepted in zephyr core.

Also, can you explain what is the purpose of this PR? The RCP architecture is supported in zephyr.

xavraz · 2024-07-18T08:53:25Z

@canisLupus1313, thank you for your comment.

OT-RCP Architecture

This PR corresponds to an ot-rcp host support. The existing support in Zephyr is for an ot-rcp device, as follows :
https://developer.nordicsemi.com/nRF_Connect_SDK/doc/1.7.1/nrf/ug_thread_architectures.html#thread-architectures-designs-cp-rcp.
This means Zephyr supports only a radio coprocessor associated with a host processor.

Zephyr code

Concerning your question "included some of the Openthread files in the Zephyr code."
openthread\platform\radio_spinel.cpp and spinel_hdlc.cpp files are interfaces :

radio_spinel.cpp
Interface between net\l2\openthread HOST layer and the spinel driver from the openthread library.
zephyr\subsys\net\l2\openthread\openthread.c (not in modules\lib\openthread)
↓
zephyr\modules\openthread\platform\radio_spinel.cpp
↓
modules\lib\openthread\src\lib\spinel\spinel_driver.cpp
spinel_hdlc.cpp
Interface between the spinel driver from the openthread library and the HDLC interface.
modules\lib\openthread\src\lib\spinel\spinel_driver.cpp
↓
zephyr\modules\openthread\platform\spinel_hdlc.cpp
↓
zephyr\drivers\hdlc_rcp_if\hdlc_rcp_if_nxp.c

C++ code in Zephyr

There are already few files in zephyr directory as follows for example

rlubos

THe commit message titles need to follow https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-message-guidelines

modules/openthread/platform/radio_spinel.cpp

modules/openthread/platform/platform_spinel.c

modules/openthread/platform/platform.h

canisLupus1313 · 2024-07-18T10:00:33Z

drivers/hdlc_rcp_if/Kconfig.nxp

+# Openthead HDLC RCP communication Interface configuration options
+# Copyright 2024 NXP
+# All rights reserved.
+# SPDX-License-Identifier: BSD-3-Clause


BSD license should be changed to Apache-2.0

I will change the licences following all your comments.

Note that you can only change the license if you are the author. If you took the code from somewhere else, then you need to use the original license.

canisLupus1313 commented 2 days ago
@xavraz thanks for your contribution.
It seems that You have included some of the Openthread files in the Zephyr code. I would say would be better to include form openthread module.

Concerning the OpenThread files included in the Zephyr code, this has been done to ensure the compatibility if the OpenThread library version is modified. This permits to go back to an older OpenThread version. Otherwise, if the source code is set in modules\lib\openthread\src\xxx, then, it is not possible to go back to an older OpenThread version.

canisLupus1313 · 2024-07-18T10:17:13Z

modules/openthread/CMakeLists.txt

+    ${ZEPHYR_BASE}/arch/arm/include
+    ${ZEPHYR_BASE}/../modules/lib/openthread/src
+    ${ZEPHYR_BASE}/../modules/lib/openthread/src/core
+    ${ZEPHYR_BASE}/../modules/lib/openthread/third_party/mbedtls/repo/include


Zephyr is using it's own mbed TLS: https://github.com/zephyrproject-rtos/mbedtls not exactly same as the on in OT.

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c

canisLupus1313 · 2024-07-18T10:28:21Z

modules/openthread/platform/platform.h

+#define OT_PLAT_DBG_LEVEL_NONE 0
+#define OT_PLAT_DBG_LEVEL_ERR 1
+#define OT_PLAT_DBG_LEVEL_WARNING 2
+#define OT_PLAT_DBG_LEVEL_INFO 3
+#define OT_PLAT_DBG_LEVEL_DEBUG 4


Why are you defining those?
It should be possible to reuse logging from https://docs.zephyrproject.org/latest/services/logging/index.html

modules/openthread/platform/spinel_hdlc.hpp

canisLupus1313 · 2024-07-18T10:40:23Z

@canisLupus1313, thank you for your comment.

OT-RCP Architecture

This PR corresponds to an ot-rcp host support. The existing support in Zephyr is for an ot-rcp device, as follows : https://developer.nordicsemi.com/nRF_Connect_SDK/doc/1.7.1/nrf/ug_thread_architectures.html#thread-architectures-designs-cp-rcp. This means Zephyr supports only a radio coprocessor associated with a host processor.

Zephyr code

Concerning your question "included some of the Openthread files in the Zephyr code." openthread\platform\radio_spinel.cpp and spinel_hdlc.cpp files are interfaces :

radio_spinel.cpp
Interface between net\l2\openthread HOST layer and the spinel driver from the openthread library.
zephyr\subsys\net\l2\openthread\openthread.c (not in modules\lib\openthread)
↓
zephyr\modules\openthread\platform\radio_spinel.cpp
↓
modules\lib\openthread\src\lib\spinel\spinel_driver.cpp

spinel_hdlc.cpp
Interface between the spinel driver from the openthread library and the HDLC interface.
modules\lib\openthread\src\lib\spinel\spinel_driver.cpp
↓
zephyr\modules\openthread\platform\spinel_hdlc.cpp
↓
zephyr\drivers\hdlc_rcp_if\hdlc_rcp_if_nxp.c

C++ code in Zephyr

There are already few files in zephyr directory as follows for example

• zephyr\modules\thrift\src\thrift\transport\TSSLSocket.cpp
• zephyr\modules\thrift\src\thrift\server\TServerFramework.cpp

@xavraz Thanks for clarification.
The idea of what You are trying to do seems great. I'm wondering if the parts of code corresponding to the Spinel interface wouldn't fit more into OT repository than zephyr's. Then You chould just provide the Platform API for sending the data. and altering integration in openthread.c as you have already done. Have You considered that?

drivers/hdlc_rcp_if/Kconfig

drivers/hdlc_rcp_if/Kconfig.nxp

modules/openthread/platform/spinel_hdlc.cpp

modules/openthread/platform/platform_spinel.c

modules/openthread/platform/spinel_hdlc.hpp

JA-NXP · 2024-10-16T09:16:35Z

I didn't changed the settings_nvs.c file so I don't know why I have this error:

rlubos · 2024-10-16T10:31:22Z

@JA-NXP Settings subsys will fetch the storage partition info from device tree, and it seems that storage_partition is not defined for frdm_rw612, hence the error. rd_rw612_bga doesn't seem to have this problem:

zephyr/boards/nxp/rd_rw612_bga/rd_rw612_bga.dtsi

Line 166 in 8c705ca

storage_partition: partition@623000 {

pdgendt · 2024-10-16T13:16:10Z

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c

Why do we need to use middleware that again translates to UART/SPI? There is already UART and SPI abstraction in zephyr.

I once opened a PoC PR to support over a generic SPI interface: #47256

Since for our RW612 board we don't use SPI/UART for RCP however we use a custom interface for inter cpu communication. So we are thinking to create a generic driver interface for HDLC in order to map any interface type.

zephyrbot · 2024-10-17T09:51:56Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@`4a4741f` (master)	zephyrproject-rtos/hal_nxp#453	zephyrproject-rtos/hal_nxp#453/files

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

dleach02 · 2024-10-25T20:59:41Z

#79882 updated the TSC ticket with TSC notes. There is a specific question on why we need to pull the particular files in.

pdgendt · 2024-11-01T16:19:02Z

modules/openthread/platform/hdlc_interface.cpp

+	hdlc_api = (struct hdlc_api *)radio_dev->api;
+	if (!hdlc_api) {
+		otLogDebgPlat("Radio device initialization failed");
+		assert(0);


Why are you mixing zephyr's __ASSERT macros with these assert calls?

Agree & resolved

pdgendt · 2024-11-01T16:21:15Z

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c

+	return ret;
+}
+
+static struct hdlc_api hdlc_api = {.iface_api.init = hdlc_iface_init,


Do not use a name that is the same as the struct type, also this should be const.

I wonder why compliance doesn't complain here?

Agree & resolved

pdgendt · 2024-11-01T16:25:41Z

modules/openthread/platform/hdlc_interface.cpp

+	(void)buffer;
+	(void)len;


Use some macro here, OT_UNUSED_VARIABLE or maybe even prefer ARG_UNUSED?

Agree & resolved

pdgendt · 2024-11-05T13:39:16Z

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c

+		status = PLATFORM_InitHdlcInterface(hdlc_rx_callback, param);
+		if (status < 0) {
+			LOG_ERR("HDLC RX callback registration failed");
+			ret = EIO;


Error return values are typically negative.

Also I don't think the do {} while (false); makes sense here.

Agree & resolved

pdgendt · 2024-11-05T14:51:09Z

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c

+	return ret;
+}
+
+static struct hdlc_api nxp_hdlc_api = {.iface_api.init = hdlc_iface_init,


Suggested change

static struct hdlc_api nxp_hdlc_api = {.iface_api.init = hdlc_iface_init,

static const struct hdlc_api nxp_hdlc_api = {.iface_api.init = hdlc_iface_init,

pdgendt · 2024-11-05T14:52:52Z

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c

+static struct hdlc_api nxp_hdlc_api = {.iface_api.init = hdlc_iface_init,
+				   .register_rx_cb = hdlc_register_rx_cb,
+				   .send = hdlc_send,
+				   .deinit = hdlc_deinit};


Add a trailing comma so it gets formatted with only 1 indentation.

Nice to know. Resolved.

Add a HDLC RCP communication with its hdlc_api interface APIs and a NXP driver. Signed-off-by: Jamel Arbi <[email protected]>

Add a spinel support to an RCP design, the core of OpenThread lives on the host processor connected to an RCP radio controller over a HDLC interface. Signed-off-by: Jamel Arbi <[email protected]>

Adapt the openthread.c file to support the host RCP interface. Signed-off-by: Jamel Arbi <[email protected]>

Add OT RCP config for NXP RW612 platform Signed-off-by: Jamel Arbi <[email protected]>

Add flash partirions for NXP FRDM RW612 board Signed-off-by: Jamel Arbi <[email protected]>

Update hal_nxp to include Openthread RCP changes Signed-off-by: Jamel Arbi <[email protected]>

xavraz requested review from mmahadevan108 and dleach02 as code owners July 17, 2024 08:45

zephyrbot added area: OpenThread platform: NXP Drivers NXP Semiconductors, drivers area: Networking labels Jul 17, 2024

zephyrbot requested review from canisLupus1313, danieldegrasse, dbaluta, decsny, edmont, jukkar, maciejbaczmanski, manuargue, mariuszpos, pdgendt, rlubos, ssharks and tbursztyka July 17, 2024 08:46

zephyrbot assigned rlubos Jul 17, 2024

rlubos reviewed Jul 18, 2024

View reviewed changes

modules/openthread/platform/radio_spinel.cpp Show resolved Hide resolved

modules/openthread/platform/platform_spinel.c Outdated Show resolved Hide resolved

modules/openthread/platform/platform.h Outdated Show resolved Hide resolved

canisLupus1313 reviewed Jul 18, 2024

View reviewed changes

drivers/hdlc_rcp_if/hdlc_rcp_if_nxp.c Show resolved Hide resolved

canisLupus1313 reviewed Jul 18, 2024

View reviewed changes

modules/openthread/platform/spinel_hdlc.hpp Outdated Show resolved Hide resolved

rlubos reviewed Jul 18, 2024

View reviewed changes

dleach02 mentioned this pull request Oct 15, 2024

Add OpenThread Spinel HDLC RCP host interface files to Zehyr #79882

Closed

JA-NXP force-pushed the feature_openthread_hdlc_rcp branch from 678cee7 to a492241 Compare October 16, 2024 09:23

pdgendt reviewed Oct 16, 2024

View reviewed changes

zephyrbot added manifest manifest-hal_nxp labels Oct 17, 2024

JA-NXP force-pushed the feature_openthread_hdlc_rcp branch from 7f6b72f to bd0e312 Compare October 17, 2024 12:38

dleach02 force-pushed the feature_openthread_hdlc_rcp branch 2 times, most recently from f5e1f04 to 20acadd Compare October 21, 2024 16:27

rlubos previously approved these changes Oct 31, 2024

View reviewed changes

pdgendt reviewed Nov 1, 2024

View reviewed changes

JA-NXP dismissed rlubos’s stale review via 4d090ab November 5, 2024 13:36

JA-NXP force-pushed the feature_openthread_hdlc_rcp branch from 20acadd to 4d090ab Compare November 5, 2024 13:36

pdgendt reviewed Nov 5, 2024

View reviewed changes

JA-NXP force-pushed the feature_openthread_hdlc_rcp branch 2 times, most recently from 28d957b to c22284e Compare November 5, 2024 14:47

pdgendt reviewed Nov 5, 2024

View reviewed changes

JA-NXP force-pushed the feature_openthread_hdlc_rcp branch 2 times, most recently from 4844f76 to 1afb64b Compare November 5, 2024 16:53

JA-NXP added 6 commits November 5, 2024 18:28

drivers: openthread: nxp: Add a HDLC RCP communication

fa39d64

Add a HDLC RCP communication with its hdlc_api interface APIs and a NXP driver. Signed-off-by: Jamel Arbi <[email protected]>

modules: openthread: Add of spinel hdlc rcp host interface

aaa5adf

Add a spinel support to an RCP design, the core of OpenThread lives on the host processor connected to an RCP radio controller over a HDLC interface. Signed-off-by: Jamel Arbi <[email protected]>

net: openthread: Adapt OpenThread to support RCP interface

d1f29b4

Adapt the openthread.c file to support the host RCP interface. Signed-off-by: Jamel Arbi <[email protected]>

samples: net: sockets: Add OT RCP config for echo client, server

a606738

Add OT RCP config for NXP RW612 platform Signed-off-by: Jamel Arbi <[email protected]>

boards: nxp: frdm_rw612: add flash partitions

4901a24

Add flash partirions for NXP FRDM RW612 board Signed-off-by: Jamel Arbi <[email protected]>

west.yml: hal_nxp update from upstream

f51d525

Update hal_nxp to include Openthread RCP changes Signed-off-by: Jamel Arbi <[email protected]>

JA-NXP force-pushed the feature_openthread_hdlc_rcp branch from 1afb64b to f51d525 Compare November 5, 2024 17:35

	static struct hdlc_api nxp_hdlc_api = {.iface_api.init = hdlc_iface_init,
	static const struct hdlc_api nxp_hdlc_api = {.iface_api.init = hdlc_iface_init,

OpenThread HDLC RCP communication support #75986

Are you sure you want to change the base?

OpenThread HDLC RCP communication support #75986

Conversation

xavraz commented Jul 17, 2024

github-actions bot commented Jul 17, 2024

canisLupus1313 commented Jul 17, 2024

xavraz commented Jul 18, 2024

OT-RCP Architecture

Zephyr code

C++ code in Zephyr

rlubos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavraz Jul 18, 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

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

canisLupus1313 commented Jul 18, 2024

OT-RCP Architecture

Zephyr code

C++ code in Zephyr

JA-NXP commented Oct 16, 2024 • edited Loading

rlubos commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zephyrbot commented Oct 17, 2024 • edited Loading

dleach02 commented Oct 25, 2024

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

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

xavraz Jul 18, 2024 •

edited

Loading

JA-NXP commented Oct 16, 2024 •

edited

Loading

zephyrbot commented Oct 17, 2024 •

edited

Loading