net: openthread: Host-Processor support #47256

pdgendt · 2022-07-04T12:11:58Z

This PR adds support to operate as an OpenThread Host-Processor connected to an RCP radio controller over SPI. Both FTD and MTD modes are supported.

Some remarks up for improvement:

Added a default SPI peripheral device init priority, should this be moved to a device specific one?
The dummy IEEE802.15.4 interface feels iffy, not sure if there is an alternative though.
Currently only SPI support, UART/USB should be fairly straightforward to be added in the future.
C++ source files are injected into the OpenThread library, this is the easiest approach to make sure all OpenThread configs are applied to the header-only included files.
Ported POSIX SPI implementation from OpenThread's repository. The source file license header has to be kept, not sure if this breaks compliance.

Tested on a custom board using NXP's i.MX RT1064 connected to a Nordic nRF52840.

Add a Kconfig option for SPI peripheral devices' default init priority. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>

This commit adds support to use OpenThread on a host-processor device, connected to an RCP device over SPI. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>

Add a Kconfig option for a host-controller to enable RCP restoration. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>

pdgendt

Added a self-review for some comments/fixes.

pdgendt · 2022-07-06T12:16:37Z

dts/bindings/misc/google,openthread-spinel.yaml

+    required: false
+    description: Interrupt pin
+
+      Optional interript pin asserted by the RCP


Suggested change

Optional interript pin asserted by the RCP

Optional interrupt pin asserted by the RCP

pdgendt · 2022-07-06T12:17:36Z

subsys/net/lib/openthread/platform/radio_spinel.cpp

+#ifdef CONFIG_OPENTHREAD_HOSTPROCESSOR_SPI
+#include "spi_interface.hpp"
+
+typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformSpiRadioSpinel;


Suggested change

typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformSpiRadioSpinel;

typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformRadioSpinel;

For my own understanding and for the sake of argumenting the use of C++ code here (it's kind of a gray area when it comes to the Zephyr codebase), please correct me if I'm wrong:

In order to implement OT host functionality, we need to use an ot::Spinel::RadioSpinel template class, which takes two template parameters: InterfaceType and ProcessContextType. The latter is not a concern, but InterfaceType is expected to be a class implementing certain methods (defined in spi_interface.hpp), therefore there's no other way around than using C++ for that.

The ot::Spinel::RadioSpinel template class does not expose a C API, therefore the spinel-based otPlatRadio* driver also needs to be implemented in C++.

Is that more or less correct?

Yes, kind of, we could try to re-implement the entire Spinel protocol, however this would be almost impossible to maintain.

Yes

pdgendt · 2022-07-06T12:19:56Z

subsys/net/lib/openthread/platform/radio_spinel.cpp

+{
+	OT_UNUSED_VARIABLE(aInstance);
+
+	SuccessOrDie(RadioSpinelInstance().GetIeeeEui64(aIeeeEui64));


Not sure if the SuccessOrDie is the best approach to "fail".

MIght not be the best idea, from what I've seen the newlib wrapper for this function will simply end up in a busy-loop.

Some caveat here, the included radio_spinel_impl.hpp also uses these functions a lot.

pdgendt · 2022-07-06T12:21:18Z

subsys/net/lib/openthread/platform/spi_host.c

+
+	if (!data->ready) {
+		data->ready = true;
+		gpio_add_callback(config->irq_gpio.port, &data->irq_gpio_callback);


Suggested change

gpio_add_callback(config->irq_gpio.port, &data->irq_gpio_callback);

if (config->irq_gpio.port) {

gpio_add_callback(config->irq_gpio.port, &data->irq_gpio_callback);

}

pdgendt · 2022-07-06T12:24:32Z

subsys/net/lib/openthread/platform/spi_interface.cpp

+/*
+ *  Copyright (c) 2019, The OpenThread Authors.
+ *  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without
+ *  modification, are permitted provided that the following conditions are met:
+ *  1. Redistributions of source code must retain the above copyright
+ *     notice, this list of conditions and the following disclaimer.
+ *  2. Redistributions in binary form must reproduce the above copyright
+ *     notice, this list of conditions and the following disclaimer in the
+ *     documentation and/or other materials provided with the distribution.
+ *  3. Neither the name of the copyright holder nor the
+ *     names of its contributors may be used to endorse or promote products
+ *     derived from this software without specific prior written permission.
+ *
+ *  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ *  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ *  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ *  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+ *  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ *  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ *  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ *  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ *  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ *  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ *  POSSIBILITY OF SUCH DAMAGE.
+ */


The original header from the posix ported SPI implementation.

pdgendt · 2022-07-06T12:26:13Z

subsys/net/lib/openthread/platform/spi_interface.hpp

+struct RadioSpinelContext {
+	otInstance *mInstance;
+};


Simply wraps an instance pointer, as the Process method only accepts const arguments.

rlubos

Some initial high-level feedback after digesting (and hopefully understanding) this codebase.

rlubos · 2022-07-08T12:46:48Z

subsys/net/lib/openthread/platform/alarm.c

 }

+#ifdef CONFIG_OPENTHREAD_HOSTPROCESSOR
+uint64_t otPlatTimeGet(void)


I wonder if it'd make sense to create time.c platform implementation and implement API defined in OT's time.h in there, there are only 2 functions in there, but currently, they're scattered around different files.

rlubos · 2022-07-08T13:48:56Z

subsys/net/lib/openthread/platform/radio_spinel.cpp

+#ifdef CONFIG_OPENTHREAD_HOSTPROCESSOR_SPI
+#include "spi_interface.hpp"
+
+typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformSpiRadioSpinel;


For my own understanding and for the sake of argumenting the use of C++ code here (it's kind of a gray area when it comes to the Zephyr codebase), please correct me if I'm wrong:

In order to implement OT host functionality, we need to use an ot::Spinel::RadioSpinel template class, which takes two template parameters: InterfaceType and ProcessContextType. The latter is not a concern, but InterfaceType is expected to be a class implementing certain methods (defined in spi_interface.hpp), therefore there's no other way around than using C++ for that.

The ot::Spinel::RadioSpinel template class does not expose a C API, therefore the spinel-based otPlatRadio* driver also needs to be implemented in C++.

Is that more or less correct?

rlubos · 2022-07-08T13:51:28Z

subsys/net/lib/openthread/platform/radio_spinel.cpp

+{
+	OT_UNUSED_VARIABLE(aInstance);
+
+	SuccessOrDie(RadioSpinelInstance().GetIeeeEui64(aIeeeEui64));


MIght not be the best idea, from what I've seen the newlib wrapper for this function will simply end up in a busy-loop.

rlubos · 2022-07-08T13:54:02Z

modules/openthread/CMakeLists.txt

 endif()

+if(CONFIG_OPENTHREAD_HOSTPROCESSOR)
+  # Inject our platform spinel implementation


Can you elaborate on why is it needed to inject those sources into OT libs, why can't it end up in the openthread_platform library with other platform files?

The OpenThread build system has private compile definitions for each library. This was the easiest way to make sure exactly the same definitions are set.
Because of the C++ template for the RadioSpinel instance, which is header-only.

rlubos · 2022-07-08T13:55:52Z

drivers/ieee802154/ieee802154_ot_spinel.c

+	.get_capabilities = ot_spinel_get_capabilities,
+	.cca              = ot_spinel_cca,
+	.set_channel      = ot_spinel_set_channel,
+	.filter           = ot_spinel_filter,
+	.set_txpower      = ot_spinel_set_txpower,
+	.tx               = ot_spinel_tx,
+	.start            = ot_spinel_start,
+	.stop             = ot_spinel_stop,
+	.configure        = ot_spinel_configure,


Are those dummy-implementations even needed, couldn't they be just NULL pointers? Noone should actaully call them as the radio layer for OT gets a custom implementation for this configuration.

I wasn't sure how to add this file, but if the API isn't called from other subsystems, I can leave them as NULL. Isn't some shell command able to call these?

AFAIK they're not.

rlubos · 2022-07-08T14:04:10Z

subsys/net/lib/openthread/platform/spi_interface.cpp

+ *  Copyright (c) 2019, The OpenThread Authors.
+ *  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without


This is a problem, AFAIK no license other than Apache-2.0 is allowed in Zephyr tree.

Yeah, I figured that much. If this blocks the PR, I could move this into the zephyrproject/openthread repo.
But then the code is split across repositories.

That's also an option, although still not perfect, as we're aiming to keep the fork as close to the original repo as possible. Pehrpas you could ask the OT community if they're if we reuse parts of the code under Zephyr license? It seems that there's not that much overlap between the original file and the version you've posted here.

pdgendt

@rlubos Thanks for the review, I'm going to clean it up and I'll also try to add the HDCL (UART) so it's easier to test with an USB dongle.

See my responses for more information.

pdgendt · 2022-07-08T14:10:08Z

drivers/ieee802154/ieee802154_ot_spinel.c

+	.get_capabilities = ot_spinel_get_capabilities,
+	.cca              = ot_spinel_cca,
+	.set_channel      = ot_spinel_set_channel,
+	.filter           = ot_spinel_filter,
+	.set_txpower      = ot_spinel_set_txpower,
+	.tx               = ot_spinel_tx,
+	.start            = ot_spinel_start,
+	.stop             = ot_spinel_stop,
+	.configure        = ot_spinel_configure,


I wasn't sure how to add this file, but if the API isn't called from other subsystems, I can leave them as NULL. Isn't some shell command able to call these?

pdgendt · 2022-07-08T14:12:17Z

modules/openthread/CMakeLists.txt

 endif()

+if(CONFIG_OPENTHREAD_HOSTPROCESSOR)
+  # Inject our platform spinel implementation


The OpenThread build system has private compile definitions for each library. This was the easiest way to make sure exactly the same definitions are set.
Because of the C++ template for the RadioSpinel instance, which is header-only.

pdgendt · 2022-07-08T14:14:42Z

subsys/net/lib/openthread/platform/radio_spinel.cpp

+#ifdef CONFIG_OPENTHREAD_HOSTPROCESSOR_SPI
+#include "spi_interface.hpp"
+
+typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformSpiRadioSpinel;


Yes, kind of, we could try to re-implement the entire Spinel protocol, however this would be almost impossible to maintain.

Yes

pdgendt · 2022-07-08T14:15:54Z

subsys/net/lib/openthread/platform/radio_spinel.cpp

+{
+	OT_UNUSED_VARIABLE(aInstance);
+
+	SuccessOrDie(RadioSpinelInstance().GetIeeeEui64(aIeeeEui64));


Some caveat here, the included radio_spinel_impl.hpp also uses these functions a lot.

pdgendt · 2022-07-08T14:18:41Z

subsys/net/lib/openthread/platform/spi_interface.cpp

+ *  Copyright (c) 2019, The OpenThread Authors.
+ *  All rights reserved.
+ *
+ *  Redistribution and use in source and binary forms, with or without


Yeah, I figured that much. If this blocks the PR, I could move this into the zephyrproject/openthread repo.
But then the code is split across repositories.

github-actions · 2022-09-07T00:32:34Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

pdgendt · 2022-09-21T06:29:04Z

I'm going to pick this up again later.

pdgendt added 3 commits July 4, 2022 13:53

drivers: spi: Add default SPI peripheral device init priority

c8d6552

Add a Kconfig option for SPI peripheral devices' default init priority. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>

net: openthread: Add host-processor support

c7f8cce

This commit adds support to use OpenThread on a host-processor device, connected to an RCP device over SPI. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>

net: openthread: Add Spinel RCP restoration option

2221a8b

Add a Kconfig option for a host-controller to enable RCP restoration. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be>

pdgendt requested a review from edmont July 4, 2022 12:11

pdgendt added area: OpenThread area: IEEE 802.15.4 labels Jul 4, 2022

pdgendt commented Jul 6, 2022

View reviewed changes

rlubos reviewed Jul 8, 2022

View reviewed changes

pdgendt commented Jul 8, 2022

View reviewed changes

github-actions bot added the Stale label Sep 7, 2022

github-actions bot closed this Sep 21, 2022

pdgendt mentioned this pull request Oct 16, 2024

OpenThread HDLC RCP communication support #75986

Merged

	Optional interript pin asserted by the RCP
	Optional interrupt pin asserted by the RCP

	typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformSpiRadioSpinel;
	typedef ot::Spinel::RadioSpinel<SpiInterface, RadioSpinelContext> PlatformRadioSpinel;

net: openthread: Host-Processor support #47256

net: openthread: Host-Processor support #47256

Uh oh!

Conversation

pdgendt commented Jul 4, 2022

Uh oh!

pdgendt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rlubos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdgendt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 7, 2022

Uh oh!

pdgendt commented Sep 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants