Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

samples: applications: Use ipc_radio as default radio firmware. #15435

Merged
merged 1 commit into from
May 29, 2024

Conversation

dchat-nordic
Copy link
Contributor

@dchat-nordic dchat-nordic commented May 22, 2024

Use ipc_radio as default radio firmware.

test_audio: PR-259
test_chip: PR-760
test_thread: PR-1172

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 22, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented May 22, 2024

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
desktop52_verification X
test-fw-nrfconnect-apps X
test-fw-nrfconnect-ble_mesh X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-nfc X
test-fw-nrfconnect-thread X
test-fw-nrfconnect-zigbee X
test-sdk-audio X
test-sdk-dfu X

Detailed information of selected test modules

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

Copy link
Contributor

@ArekBalysNordic ArekBalysNordic left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@LuDuda LuDuda left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

@NordicBuilder
Copy link
Contributor

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.

Copy link
Contributor

@kapi-no kapi-no left a comment

Choose a reason for hiding this comment

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

Approving the Fast Pair part

@dchat-nordic dchat-nordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 22, 2024
@dchat-nordic dchat-nordic requested a review from peknis May 24, 2024 12:08
@@ -105,7 +105,7 @@ The following snippets are available:
Not compatible with the ``tcat`` snippet.

.. note::
When building with the ``multiprotocol`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` build target, the :kconfig:option:`SB_CONFIG_NETCORE_MULTIPROTOCOL_RPMSG` Kconfig option has to be set to ``y``.
When building with the ``multiprotocol`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` board target, set the :makevar:`FILE_SUFFIX` CMake variable to ``ble``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When building with the ``multiprotocol`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` board target, set the :makevar:`FILE_SUFFIX` CMake variable to ``ble``.
When building with the ``multiprotocol`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` board target, set the :makevar:`FILE_SUFFIX` CMake option to ``ble``.
See :ref:`app_build_file_suffixes` and :ref:`cmake_options` for more information.

@@ -129,7 +129,7 @@ The following snippets are available:
* ``multiprotocol_ble`` - Enables the Multiprotocol Bluetooth LE extension.

.. note::
When building with ``multiprotocol_ble`` snippet, for the ``nrf5340dk/nrf5340/cpuapp`` build target, additional :kconfig:option:`SB_CONFIG_NETCORE_MULTIPROTOCOL_RPMSG` option has to be set.
When building with the ``multiprotocol_ble`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` board target, set the additional :makevar:`FILE_SUFFIX` CMake variable to ``ble``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When building with the ``multiprotocol_ble`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` board target, set the additional :makevar:`FILE_SUFFIX` CMake variable to ``ble``.
When building with the ``multiprotocol_ble`` snippet for the ``nrf5340dk/nrf5340/cpuapp`` board target, set the additional :makevar:`FILE_SUFFIX` CMake option to ``ble``.
See :ref:`app_build_file_suffixes` and :ref:`cmake_options` for more information.

Copy link
Member

@maciejbaczmanski maciejbaczmanski left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@alstrzebonski alstrzebonski left a comment

Choose a reason for hiding this comment

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

I don't like the current approach. Earlier we had an abstraction of having NRF_DEFAULT_BLUETOOTH option - sample was declaring that it wants to have Bluetooth. Now we have samples that enables NRF_DEFAULT_IPC_RADIO also for targets with one core which looks odd. Maybe we could just change defaults in choice NETCORE in sysbuild/Kconfig.netcore so that NETCORE would default to NETCORE_IPC_RADIO if NRF_DEFAULT_BLUETOOTH is enabled and we could introduce the NRF_DEFAULT_HCI_IPC option that would default the NETCORE to NETCORE_HCI_IPC (NRF_DEFAULT_HCI_IPC would be doing what NRF_DEAFULT_BLUETOOTH is doing now).

@dchat-nordic
Copy link
Contributor Author

I don't like the current approach. Earlier we had an abstraction of having NRF_DEFAULT_BLUETOOTH option - sample was declaring that it wants to have Bluetooth. Now we have samples that enables NRF_DEFAULT_IPC_RADIO also for targets with one core which looks odd. Maybe we could just change defaults in choice NETCORE in sysbuild/Kconfig.netcore so that NETCORE would default to NETCORE_IPC_RADIO if NRF_DEFAULT_BLUETOOTH is enabled and we could introduce the NRF_DEFAULT_HCI_IPC option that would default the NETCORE to NETCORE_HCI_IPC.

The current approach has merit, because it explicitly states which application is used.
Moreover, in case of zigbee samples declaring NRF_DEFAULT_BLUETOOTH would be at the least confusing.

@alstrzebonski
Copy link
Contributor

alstrzebonski commented May 28, 2024

I don't like the current approach. Earlier we had an abstraction of having NRF_DEFAULT_BLUETOOTH option - sample was declaring that it wants to have Bluetooth. Now we have samples that enables NRF_DEFAULT_IPC_RADIO also for targets with one core which looks odd. Maybe we could just change defaults in choice NETCORE in sysbuild/Kconfig.netcore so that NETCORE would default to NETCORE_IPC_RADIO if NRF_DEFAULT_BLUETOOTH is enabled and we could introduce the NRF_DEFAULT_HCI_IPC option that would default the NETCORE to NETCORE_HCI_IPC.

The current approach has merit, because it explicitly states which application is used. Moreover, in case of zigbee samples declaring NRF_DEFAULT_BLUETOOTH would be at the least confusing.

Enabling NRF_DEFAULT_IPC_RADIO for one-core targets just looks bad for me. Zigbee could still enable NRF_DEFAULT_IPC_RADIO if it doesn't want to rely on default option ( NRF_DEFAULT_BLUETOOTH).

@dchat-nordic
Copy link
Contributor Author

I don't like the current approach. Earlier we had an abstraction of having NRF_DEFAULT_BLUETOOTH option - sample was declaring that it wants to have Bluetooth. Now we have samples that enables NRF_DEFAULT_IPC_RADIO also for targets with one core which looks odd. Maybe we could just change defaults in choice NETCORE in sysbuild/Kconfig.netcore so that NETCORE would default to NETCORE_IPC_RADIO if NRF_DEFAULT_BLUETOOTH is enabled and we could introduce the NRF_DEFAULT_HCI_IPC option that would default the NETCORE to NETCORE_HCI_IPC.

The current approach has merit, because it explicitly states which application is used. Moreover, in case of zigbee samples declaring NRF_DEFAULT_BLUETOOTH would be at the least confusing.

Enabling NRF_DEFAULT_IPC_RADIO for one-core targets just looks bad for me. Zigbee could still enable NRF_DEFAULT_IPC_RADIO if it doesn't want to rely on default option ( NRF_DEFAULT_BLUETOOTH).

None of the network core images specified in

choice NETCORE
is ever enabled for single-core devices because of dependecies and ifs specified in said file.

Use ipc_radio as default radio firmware over:
-hci_ipc
-multiprotocol_rpmsg
-802154_rpmsg

Jira: NCSDK-27179

Co-authored-by: Kamil Gawor <[email protected]>
Signed-off-by: Dominik Chat <[email protected]>
@kapi-no
Copy link
Contributor

kapi-no commented May 29, 2024

I don't like the current approach. Earlier we had an abstraction of having NRF_DEFAULT_BLUETOOTH option - sample was declaring that it wants to have Bluetooth. Now we have samples that enables NRF_DEFAULT_IPC_RADIO also for targets with one core which looks odd. Maybe we could just change defaults in choice NETCORE in sysbuild/Kconfig.netcore so that NETCORE would default to NETCORE_IPC_RADIO if NRF_DEFAULT_BLUETOOTH is enabled and we could introduce the NRF_DEFAULT_HCI_IPC option that would default the NETCORE to NETCORE_HCI_IPC.

The current approach has merit, because it explicitly states which application is used. Moreover, in case of zigbee samples declaring NRF_DEFAULT_BLUETOOTH would be at the least confusing.

Enabling NRF_DEFAULT_IPC_RADIO for one-core targets just looks bad for me. Zigbee could still enable NRF_DEFAULT_IPC_RADIO if it doesn't want to rely on default option ( NRF_DEFAULT_BLUETOOTH).

None of the network core images specified in

choice NETCORE

is ever enabled for single-core devices because of dependecies and ifs specified in said file.

The NRF_DEFAULT_IPC_RADIO option is an independent option that is not part of the choice NETCORE and is enabled for nRF52 devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.