Improve nRF Cloud library and sample user experience #17635

plskeggs · 2024-10-03T22:31:06Z

Per IRIS-9151, add detection of which root CA certs are likely to be present, and gate the connection
to the cloud based on what is required for the current connection type (CoAP, MQTT, or REST).

Indicate the FOTA, shadow, and provisioning update intervals more clearly and at INF levels.

Indicate when cloud logging level or alerts are changed by request of the cloud at the INF level.

Slow down provisioning check once the device connects to the cloud.

Jira: IRIS-9151

In modem_key_mgmt_read(), if *len parameter is too small to fit the certificate read, return -ENOMEM and set *len to the size needed. This is helpful to check the size of a certificate without actually needing the contents. Signed-off-by: Pete Skeggs <[email protected]>

NordicBuilder · 2024-10-03T22:32:20Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 5

Inputs:

Sources:

sdk-nrf: PR head: a4eb03c19e54c97ebab74d4d6d693def4ceee205

more details

sdk-nrf:

PR head: a4eb03c19e54c97ebab74d4d6d693def4ceee205
merge base: 547aaab19c52cc03660d57943a963480a042e14f
target head (main): f7258d0ad63ff6f736b08f60b9d4c2e28398de68
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (14)

doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
include
│  ├── modem
│  │  │ modem_key_mgmt.h
│  ├── net
│  │  │ nrf_cloud.h
lib
│  ├── modem_key_mgmt
│  │  │ modem_key_mgmt.c
samples
│  ├── cellular
│  │  ├── nrf_cloud_multi_service
│  │  │  ├── Kconfig
│  │  │  ├── overlay_coap.conf
│  │  │  ├── src
│  │  │  │  ├── cloud_connection.c
│  │  │  │  │ fota_support_coap.c
│  │  ├── nrf_cloud_rest_fota
│  │  │  ├── src
│  │  │  │  │ main.c
subsys
│  ├── net
│  │  ├── lib
│  │  │  ├── nrf_cloud
│  │  │  │  ├── Kconfig
│  │  │  │  ├── src
│  │  │  │  │  ├── nrf_cloud_alert.c
│  │  │  │  │  ├── nrf_cloud_credentials.c
│  │  │  │  │  │ nrf_cloud_log.c
│  │  │  ├── nrf_provisioning
│  │  │  │  ├── src
│  │  │  │  │  │ nrf_provisioning.c

Outputs:

Toolchain

Version: 9583beca34
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:9583beca34_81ed5a52d6

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 311
❌ Integration tests
- ✅ test-fw-nrfconnect-boot
- ✅ test-fw-nrfconnect-nrf-iot_cloud
- ✅ test-fw-nrfconnect-nrf-iot_serial_lte_modem
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- ❌ test-fw-nrfconnect-nrf-iot_samples
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m
- ✅ test-fw-nrfconnect-nrf-iot_thingy91
- ✅ test-fw-nrfconnect-nrf-iot_mosh
- ✅ test-fw-nrfconnect-nrf-iot_positioning
- ✅ test-sdk-mcuboot
- ⚠️ test-fw-nrfconnect-fw-update

Disabled integration tests

- desktop52_verification
- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-proprietary_esb
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

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

NordicBuilder · 2024-10-03T22:47:29Z

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.

jayteemo · 2024-10-03T22:34:11Z

subsys/net/lib/nrf_provisioning/src/nrf_provisioning.c

@@ -493,7 +493,7 @@ int nrf_provisioning_schedule(void)
 	/* To even the load on server side */
 	retry_s += spread_s;

-	LOG_DBG("Connecting in %llds", retry_s);
+	LOG_INF("Checking for provisioning commands in %llds seconds", retry_s);


Suggested change

LOG_INF("Checking for provisioning commands in %llds seconds", retry_s);

LOG_DBG("Checking for provisioning commands in %llds seconds", retry_s);

Read the ticket

I prefer this as INF

jayteemo · 2024-10-03T22:34:35Z

subsys/net/lib/nrf_cloud/src/nrf_cloud_log.c

@@ -319,7 +319,7 @@ void nrf_cloud_log_enable(bool enable)
 		logs_backend_enable(enable);
 #endif
 		enabled = enable;
-		LOG_DBG("enabled = %d", enabled);
+		LOG_INF("Changing cloud logging enabled to:%d", enabled);


Suggested change

LOG_INF("Changing cloud logging enabled to:%d", enabled);

LOG_DBG("Changing cloud logging enabled to:%d", enabled);

This is only logged when the user changes the level compared to what the firmware was built for. This will rarely be logged and is not an issue to be INF. It will improve the user experience so they can see that a change they requested actually happened, especially for our samples. Same for alerts.

jayteemo · 2024-10-03T22:34:47Z

subsys/net/lib/nrf_cloud/src/nrf_cloud_log.c

@@ -305,7 +305,7 @@ int nrf_cloud_log_control_get(void)
 void nrf_cloud_log_level_set(int level)
 {
 	if (nrf_cloud_log_level != level) {
-		LOG_DBG("Changing log level from:%d to:%d", nrf_cloud_log_level, level);
+		LOG_INF("Changing cloud log level from:%d to:%d", nrf_cloud_log_level, level);


Suggested change

LOG_INF("Changing cloud log level from:%d to:%d", nrf_cloud_log_level, level);

LOG_DBG("Changing cloud log level from:%d to:%d", nrf_cloud_log_level, level);

subsys/net/lib/nrf_cloud/src/nrf_cloud_credentials.c

jayteemo · 2024-10-03T22:39:32Z

subsys/net/lib/nrf_cloud/src/nrf_cloud_credentials.c

instead of making the logs INF, i would prefer to add a new log module control: CONFIG_NRF_CLOUD_CREDENTIALS_LOG_LEVEL
and if you want extra logs in the sample, set the new control to DBG.

same for the nrf_cloud_alert and nrf_cloud_log files

samples/cellular/nrf_cloud_multi_service/src/fota_support_coap.c

subsys/net/lib/nrf_cloud/src/nrf_cloud_credentials.c

glarsennordic

request for changes mainly applies to the changes to MSS

samples/cellular/nrf_cloud_multi_service/src/cloud_connection.c

subsys/net/lib/nrf_cloud/Kconfig

subsys/net/lib/nrf_cloud/src/nrf_cloud_credentials.c

glarsennordic · 2024-10-03T23:33:04Z

subsys/net/lib/nrf_provisioning/src/nrf_provisioning.c

@@ -493,7 +493,7 @@ int nrf_provisioning_schedule(void)
 	/* To even the load on server side */
 	retry_s += spread_s;

-	LOG_DBG("Connecting in %llds", retry_s);
+	LOG_INF("Checking for provisioning commands in %llds seconds", retry_s);


I prefer this as INF

Add two new Kconfig symbols to define thresholds for approximate size of the CoAP root CA and the size of combined CoAP + AWS root CAs. Retrieve size of configured sec_tag's root CA. Compare the size to the thresholds, display which root CA(s) are likely present, and use this to judge whether a cloud connection will likely succeed. If connection seems possible, proceed with connection as before. Jira: IRIS-9151 Signed-off-by: Pete Skeggs <[email protected]>

Indicate the units of time as well as the purpose for reconnecting. Log at the INF level. See ticket for reasoning. Jira: IRIS-9151 Signed-off-by: Pete Skeggs <[email protected]>

When the cloud (through the shadow) requests a change in the cloud-based logging level or the alert enable/disable flag, log at the INF level so it is always easy to see that the request was effective. See ticket for reasoning. Jira: IRIS-9151 Signed-off-by: Pete Skeggs <[email protected]>

Once device has connected to the cloud, slow down the provisioning check. Improve logging of shadow and FOTA polling intervals for CoAP. Make CoAP shadow and FOTA check intervals the same as the sensor update interval. Jira: IRIS-9151 Signed-off-by: Pete Skeggs <[email protected]>

Document changes. Signed-off-by: Pete Skeggs <[email protected]>

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst

include/net/nrf_cloud.h

jayteemo · 2024-10-04T16:42:20Z

subsys/net/lib/nrf_cloud/src/nrf_cloud_credentials.c

+		/* These flags are approximate and only useful for logging to help diagnose
+		 * possible provisioning mistakes.
+		 */
+		cs->ca_combined = (cs->ca_size > CONFIG_NRF_CLOUD_COMBINED_CA_CERT_SIZE_THRESHOLD);


can the combined flag be removed?
if both coap and aws flags are set then that indicates it is combined.

if (size > combined_min) { coap = 1 aws = 1 } else if (size <= coap_max) { coap = 1 aws = 0 } else if (size > coap_max) { coap = 0 aws = 1 }

jayteemo · 2024-10-04T16:48:31Z

subsys/net/lib/nrf_cloud/src/nrf_cloud_credentials.c

+		if (!cs.ca_combined && !cs.ca_aws && cs.ca_coap) {
+			/* There is a CA, but not large enough to be correct */
+			LOG_ERR("Cannot connect using MQTT or REST; "
+				"AWS root CA certificate is missing but CoAP root CA is present.");


similar to the other "likely" log message change, maybe this should be something like:
LOG_WRN("Connection using MQTT or REST may fail as the size of the CA cert indicates it is a CoAP root CA");

same for below.

Co-authored-by: Pekka Niskanen <[email protected]> Co-authored-by: Justin Morton <[email protected]>

plskeggs requested review from a team as code owners October 3, 2024 22:31

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Oct 3, 2024

plskeggs added this to the 2.8.0 milestone Oct 3, 2024

jayteemo reviewed Oct 3, 2024

View reviewed changes

plskeggs force-pushed the IRIS-9151 branch from fdd6039 to b8d37aa Compare October 3, 2024 23:29

glarsennordic requested changes Oct 3, 2024

View reviewed changes

plskeggs force-pushed the IRIS-9151 branch from b8d37aa to 554439f Compare October 3, 2024 23:52

plskeggs requested a review from glarsennordic October 4, 2024 00:01

plskeggs added 5 commits October 3, 2024 17:31

net: lib: nrf_provisioning: Improve interval logs

5cb0ddb

Indicate the units of time as well as the purpose for reconnecting. Log at the INF level. See ticket for reasoning. Jira: IRIS-9151 Signed-off-by: Pete Skeggs <[email protected]>

doc: Add changes for nrf_cloud ux improvements

573c30f

Document changes. Signed-off-by: Pete Skeggs <[email protected]>

plskeggs force-pushed the IRIS-9151 branch from 554439f to 573c30f Compare October 4, 2024 00:31

glarsennordic approved these changes Oct 4, 2024

View reviewed changes

peknis requested changes Oct 4, 2024

View reviewed changes

doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved

juhaylinen approved these changes Oct 4, 2024

View reviewed changes

rlubos approved these changes Oct 4, 2024

View reviewed changes

anhmolt approved these changes Oct 4, 2024

View reviewed changes

jayteemo reviewed Oct 4, 2024

View reviewed changes

Apply suggestions from code review

a4eb03c

Co-authored-by: Pekka Niskanen <[email protected]> Co-authored-by: Justin Morton <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve nRF Cloud library and sample user experience #17635

Improve nRF Cloud library and sample user experience #17635

plskeggs commented Oct 3, 2024

NordicBuilder commented Oct 3, 2024 •

edited

Loading

NordicBuilder commented Oct 3, 2024

jayteemo Oct 3, 2024

plskeggs Oct 3, 2024

glarsennordic Oct 3, 2024

jayteemo Oct 3, 2024

plskeggs Oct 3, 2024

jayteemo Oct 3, 2024

jayteemo Oct 3, 2024

jayteemo Oct 3, 2024

glarsennordic left a comment

glarsennordic Oct 3, 2024

jayteemo Oct 4, 2024

jayteemo Oct 4, 2024

	LOG_INF("Checking for provisioning commands in %llds seconds", retry_s);
	LOG_DBG("Checking for provisioning commands in %llds seconds", retry_s);

	LOG_INF("Changing cloud logging enabled to:%d", enabled);
	LOG_DBG("Changing cloud logging enabled to:%d", enabled);

	LOG_INF("Changing cloud log level from:%d to:%d", nrf_cloud_log_level, level);
	LOG_DBG("Changing cloud log level from:%d to:%d", nrf_cloud_log_level, level);

Improve nRF Cloud library and sample user experience #17635

Are you sure you want to change the base?

Improve nRF Cloud library and sample user experience #17635

Conversation

plskeggs commented Oct 3, 2024

NordicBuilder commented Oct 3, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Oct 3, 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

glarsennordic left a 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

NordicBuilder commented Oct 3, 2024 •

edited

Loading