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

Don't select zcbor #15776

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Don't select zcbor #15776

wants to merge 2 commits into from

Conversation

hakonfam
Copy link
Contributor

@hakonfam hakonfam commented Jun 6, 2024

No description provided.

@github-actions github-actions bot added manifest changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jun 6, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jun 6, 2024 •
edited
Loading

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

Name Old Revision New Revision Diff
nrfxlib nrfconnect/sdk-nrfxlib@35461fb (main) nrfconnect/sdk-nrfxlib#1372 nrfconnect/sdk-nrfxlib#1372/files

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

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jun 6, 2024 •
edited
Loading

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-ble X
test-fw-nrfconnect-ble_samples X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-fem X
test-fw-nrfconnect-nrf-iot_cloud X
test-fw-nrfconnect-nrf-iot_mosh X
test-fw-nrfconnect-nrf-iot_positioning X
test-fw-nrfconnect-nrf-iot_serial_lte_modem X
test-fw-nrfconnect-nrf-iot_thingy91 X
test-fw-nrfconnect-nrf-iot_zephyr_lwm2m X
test-fw-nrfconnect-rpc X
test-fw-nrfconnect-rs X
test-fw-nrfconnect-zigbee X
test-sdk-dfu X
test-sdk-find-my X
test-sdk-sidewalk X

Detailed information of selected test modules

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

@Damian-Nordic
Copy link
Contributor

Damian-Nordic commented Jun 6, 2024 •
edited
Loading

What's the rationale for this change? All applications based on nRF RPC will have to explicitly enable ZCBOR while this seems like an implementation detail that should be enabled automatically.

Edit: ok, I found a description in sdk-nrfxlib PR but still some pitfall examples would be appreciated :)

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

@hakonfam
Copy link
Contributor Author

hakonfam commented Jun 6, 2024

What's the rationale for this change?

I general no option with prompt should ever use select (as per the zephyr documentation).

All applications based on nRF RPC will have to explicitly enable ZCBOR while this seems like an implementation detail that should be enabled automatically.

We can use the same mechanism that I did for SSF where we check for the presence of the devicetree nodes required for the module to be enabled, and then extend the definition of ZCBOR so that it will be default y in that case.

Edit: ok, I found a description in sdk-nrfxlib PR but still some pitfall examples would be appreciated :)

It is currently not possible to add depends on ZCBOR_CANONICAL - which we need to do for SSF services due to the circular dependency introduced by excess use of select.

@hakonfam
kconfig: don't 'select' ZCBOR
8682fe9 
When an option depends on another option being set, its definition
should use 'depends on' - not 'select'.

Using 'select' introduces a two way dependency which makes
it impossible to express proper kconfig dependencies in some
cases.

See 'select pitfalls' chapter in zephyr documentation for details.

Enable ZCBOR by default if required IPC nodes are present.

Ref: NCSDK-27818

Signed-off-by: Håkon Amundsen <[email protected]>
@hakonfam
kconfig: ssf: depend on ZCBOR CANONICAL
c2af693 
All interaction with SSF has to be done with canonical encoded
ZCBOR.

Ref: NCSDK-27818
Signed-off-by: Håkon Amundsen <[email protected]>
@plskeggs
Copy link
Contributor

plskeggs commented Jun 6, 2024 via email

jayteemo
jayteemo suggested changes Jun 6, 2024
View reviewed changes
subsys/net/lib/nrf_cloud/Kconfig.nrf_cloud_coap
@@ -5,8 +5,8 @@

menuconfig NRF_CLOUD_COAP
bool "nRF Cloud COAP"
depends on ZCBOR
Copy link
Contributor

Choose a reason for hiding this comment

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

in the nrf_cloud_multi_service sample please add CONFIG_ZCBOR=y to the .conf files that set CONFIG_NRF_CLOUD_COAP=y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there some condition in Kconfig or DTS that can let us know that NRF_CLOAD_COAP is enabled? For instance we can add some kconfig fragment so that ZCBOR will have default y if one of those conditions are met.

if NRF_CLOUD_COAP

config ZCBOR
  default y

endif

Copy link
Contributor

@glarsennordic glarsennordic Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe so, but personally I'd rather not have ZCBOR intelligently enable itself based on the presence of a downstream dependent library.

I'd rather just have users of NRF_CLOUD_COAP enable ZCBOR now that it is no longer automatically selected by the library

subsys/net/lib/nrf_cloud/Kconfig.nrf_cloud_fota
@@ -90,8 +90,8 @@ endif # NRF_CLOUD_REST

menuconfig NRF_CLOUD_FOTA_FULL_MODEM_UPDATE
bool "Enable full modem FOTA updates"
depends on ZCBOR
Copy link
Contributor

Choose a reason for hiding this comment

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

in the nrf_cloud_multi_service, asset_tracker_v2, and nrf_cloud_rest_fota samples, please add CONFIG_ZCBOR=y to the .conf files that set CONFIG_NRF_CLOUD_FOTA_FULL_MODEM_UPDATE=y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related to my comment above, is there some condition we can check that is common for all of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

NRF_CLOUD_FOTA_FULL_MODEM_UPDATE and NRF_CLOUD_COAP would be enabled by the application, so I don't know know of any such condition.

@hakonfam
Copy link
Contributor Author

hakonfam commented Jun 7, 2024

Is this for NCS 2.7.0?

No, we did a simpler approach for 2.7.0 (#15790) but we should get this fixed properly once 2.7.0 is out.

@maciejpietras maciejpietras requested a review from tomchy July 4, 2024 10:54
@tomchy tomchy requested a review from robertstypa July 9, 2024 07:21
KAGA164
KAGA164 reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@KAGA164 KAGA164 left a comment

Choose a reason for hiding this comment

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

Please enable ZCBOR in all configuration that use nRF RPC, especially for Bluetooth serialization

@shanthanordic shanthanordic requested review from miha-nordic and removed request for sigvartmh September 13, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KAGA164 KAGA164 KAGA164 left review comments

@jayteemo jayteemo jayteemo requested changes

@robertstypa robertstypa robertstypa approved these changes

@jonathannilsen jonathannilsen jonathannilsen approved these changes

@tejlmand tejlmand tejlmand approved these changes

@tomchy tomchy tomchy approved these changes

@anhmolt anhmolt Awaiting requested review from anhmolt

@doki-nordic doki-nordic Awaiting requested review from doki-nordic

@plskeggs plskeggs Awaiting requested review from plskeggs

@glarsennordic glarsennordic Awaiting requested review from glarsennordic

@rlubos rlubos Awaiting requested review from rlubos

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@carlescufi carlescufi Awaiting requested review from carlescufi

@miha-nordic miha-nordic Awaiting requested review from miha-nordic

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. DNM manifest manifest-nrfxlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.