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.

Sign up for GitHub

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

Jump to bottom

doc: Update the ipc_radio enable doc section #16043

Merged
merged 1 commit into from
Jun 17, 2024
Merged

doc: Update the ipc_radio enable doc section #16043

merged 1 commit into from
Jun 17, 2024

Conversation

dchat-nordic
Copy link
Contributor

Correct the ipc_radio configuration Kconfig for enable.

The Pr is related to the comment: #15830 (review)

Jira: NCSDK-27939

@dchat-nordic dchat-nordic added this to the 2.7.0 milestone Jun 17, 2024
@github-actions github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jun 17, 2024
@dchat-nordic dchat-nordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 17, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jun 17, 2024 •
edited
Loading

Test specification

CI/Jenkins/NRF

  • Skipped

CI/Jenkins/integration

  • Skipped

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

@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.

peknis
peknis requested changes Jun 17, 2024
View reviewed changes
applications/ipc_radio/README.rst Outdated

You can set the supported radio configurations using the following sysbuild Kconfig options:

* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_HCI_IPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_RPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_IEEE802154`

.. note::
For |NCS| samples and applications the :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO` is used instead of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO` in order to enable to firmware.
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
For |NCS| samples and applications the :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO` is used instead of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO` in order to enable to firmware.
For |NCS| samples and applications, use the :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO` sysbuild configuration instead of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO` to enable to firmware.

applications/ipc_radio/README.rst Outdated
@@ -76,14 +76,17 @@ The Bluetooth Low Energy and IEEE 802.15.4 functionalities can operate simultane
Sysbuild Kconfig options
========================

To enable the firmware, use the sysbuild configuration of :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO`.
To enable the firmware, use the sysbuild configuration of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO`.
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
To enable the firmware, use the sysbuild configuration of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO`.
To enable the firmware, use the sysbuild configuration :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO`.

@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 Jun 17, 2024
@dchat-nordic dchat-nordic requested a review from peknis June 17, 2024 09:38
peknis
peknis requested changes Jun 17, 2024
View reviewed changes
applications/ipc_radio/README.rst Outdated
@@ -76,14 +76,17 @@ The Bluetooth Low Energy and IEEE 802.15.4 functionalities can operate simultane
Sysbuild Kconfig options
========================

To enable the firmware, use the sysbuild configuration of :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO`.
To enable the firmware, use the sysbuild configuration :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO`.
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
To enable the firmware, use the sysbuild configuration :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO`.
To enable the firmware, use the sysbuild configuration ``SB_CONFIG_NETCORE_IPC_RADIO``.

Format the SB Kconfig options with double backticks until we have a better solution that makes these linkable.

applications/ipc_radio/README.rst Outdated
Comment on lines 83 to 85
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_HCI_IPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_RPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_IEEE802154`
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
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_HCI_IPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_RPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_IEEE802154`
* ``SB_CONFIG_NETCORE_IPC_RADIO_BT_HCI_IPC``
* ``SB_CONFIG_NETCORE_IPC_RADIO_BT_RPC``
* ``SB_CONFIG_NETCORE_IPC_RADIO_IEEE802154``

applications/ipc_radio/README.rst Outdated

You can set the supported radio configurations using the following sysbuild Kconfig options:

* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_HCI_IPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_BT_RPC`
* :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO_IEEE802154`

.. note::
For |NCS| samples and applications, use the :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO` sysbuild configuration instead of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO` to enable to firmware.
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
For |NCS| samples and applications, use the :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO` sysbuild configuration instead of :kconfig:option:`SB_CONFIG_NETCORE_IPC_RADIO` to enable to firmware.
For |NCS| samples and applications, use the ``SB_CONFIG_NRF_DEFAULT_IPC_RADIO`` sysbuild configuration instead of ``SB_CONFIG_NETCORE_IPC_RADIO`` to enable to firmware.

@dchat-nordic dchat-nordic requested a review from peknis June 17, 2024 09:45
@dchat-nordic dchat-nordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 17, 2024
nordicjm
nordicjm reviewed Jun 17, 2024
View reviewed changes
applications/ipc_radio/README.rst
To enable the firmware, use the sysbuild configuration of :kconfig:option:`SB_CONFIG_NRF_DEFAULT_IPC_RADIO`.
To enable the firmware, use the sysbuild configuration ``SB_CONFIG_NETCORE_IPC_RADIO``.
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why we aren't using the :kconfig: syntax, it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peknis informed me that Sysbuild Kconfig options are not linking properly yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't seen it working. It does not create a link to the Kconfig option description.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't link but it is a valid tag. I don't think linking is possible because sysbuild uses a different Kconfig tree @gmarull thoughts? But I think we should use :kconfig: so we can do a grep for these and maybe some verification/validation/linking in future

Copy link
Member

Choose a reason for hiding this comment

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

do these Kconfig options actually map to an existing Kconfig option? ie, if we link to the option without SB_ prefix, would it work?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, they are new

Copy link
Member

Choose a reason for hiding this comment

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

Then we have no easy option. I'd avoid using the Kconfig role, we can always grep for SB_CONFIG_.*.

greg-fer
greg-fer requested changes Jun 17, 2024
View reviewed changes
applications/ipc_radio/README.rst Outdated
* ``SB_CONFIG_NETCORE_IPC_RADIO_IEEE802154``

.. note::
For |NCS| samples and applications, use the ``SB_CONFIG_NRF_DEFAULT_IPC_RADIO`` sysbuild configuration instead of ``SB_CONFIG_NETCORE_IPC_RADIO`` to enable to firmware.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note typo in to firmware.
Does it make sense to mention what SB_CONFIG_NETCORE_IPC_RADIO is used for?

Suggested change
For |NCS| samples and applications, use the ``SB_CONFIG_NRF_DEFAULT_IPC_RADIO`` sysbuild configuration instead of ``SB_CONFIG_NETCORE_IPC_RADIO`` to enable to firmware.
For |NCS| samples and applications, use the ``SB_CONFIG_NRF_DEFAULT_IPC_RADIO`` sysbuild configuration to enable the firmware instead of ``SB_CONFIG_NETCORE_IPC_RADIO`` (which should only be used for production).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SB_CONFIG_NRF_DEFAULT_IPC_RADIO is present and used in all our (radio capable) samples, so I thought to mention the config and distingiush it from SB_CONFIG_NETCORE_IPC_RADIO to avoid confusion from the user perspective.

@dchat-nordic
doc: Update the ipc_radio enable doc section
4e27810 
Correct the ipc_radio configuration Kconfig for enable.

Jira: NCSDK-27939

Signed-off-by: Dominik Chat <[email protected]>
@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 Jun 17, 2024
@dchat-nordic dchat-nordic removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jun 17, 2024
@anangl anangl merged commit d96e501 into nrfconnect:main Jun 17, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peknis peknis peknis approved these changes

@greg-fer greg-fer greg-fer approved these changes

@gmarull gmarull Awaiting requested review from gmarull

@nordicjm nordicjm Awaiting requested review from nordicjm

Assignees
No one assigned
Labels
10-min-review doc only doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
2.7.0
Development

Successfully merging this pull request may close these issues.
7 participants
@dchat-nordic @NordicBuilder @gmarull @nordicjm @greg-fer @peknis @anangl