lib: nrf_modem: init once on dfu apply #105

eivindj-nordic · 2023-05-30T07:55:39Z

It is no longer necessary to call nrf_modem_init twice when applying modem firmware updates.

tokangas · 2023-06-21T11:56:50Z

samples/nrf9160/modem_shell/src/fota/fota.c

 mosh_error("FOTA: Expected DFU result from modem library, "
 "but no DFU was triggered");


This is now the expected behavior, so make it visible that firmware update was successful. This output is also expected by our tests.

Suggested change

mosh_error("FOTA: Expected DFU result from modem library, "

"but no DFU was triggered");

mosh_print("FOTA: Modem firmware update successful!");

samples/nrf9160/azure_fota/src/main.c

tomi-font · 2023-09-06T11:07:07Z

applications/serial_lte_modem/src/slm_at_fota.c

- handle_nrf_modem_lib_init_ret(modem_lib_init_ret);
-
- nrf_modem_at_cmd(buf, sizeof(buf), "%s", "AT%SHORTSWVER");
- if (strstr(buf, "1.3.4") || strstr(buf, "1.3.5")) {


Note, this has been removed already by nrfconnect#12086.
This branch/repo is not quite up to date with NCS's main.

tomi-font · 2023-09-06T11:21:56Z

applications/serial_lte_modem/src/main.c

@@ -379,20 +369,20 @@ int main(void)
 LOG_WRN("Failed to init slm settings");
 }

- const int ret = nrf_modem_lib_init();
-
+ int ret = nrf_modem_lib_init();


Do I understand correctly that nrf_modem_on_dfu_res() will get called right before nrf_modem_lib_init() returns?

Yes, or rather, right before nrf_modem_init() returns to nrf_modem_lib_init(), so the NRF_MODEM_LIB_ON_INIT hooks are called after.

tomi-font · 2023-09-06T11:23:42Z

applications/serial_lte_modem/src/slm_at_fota.h

 /**
 * @brief Handles @ref nrf_modem_lib_init() return values
 * relating to modem firmware update.
 *
 * @return Whether the modem must be re-initialized.
 */


Suggested change

/**

* @brief Handles @ref nrf_modem_lib_init() return values

* relating to modem firmware update.

*

* @return Whether the modem must be re-initialized.

*/

tomi-font · 2023-09-06T11:23:54Z

applications/serial_lte_modem/src/slm_at_fota.h

+void handle_nrf_modem_lib_init_ret(int modem_lib_init_ret);



Suggested change

void handle_nrf_modem_lib_init_ret(int modem_lib_init_ret);

This function does not exist anymore.

tomi-font · 2023-09-06T11:27:02Z

applications/serial_lte_modem/src/slm_at_commands.c

@@ -250,17 +250,17 @@ static int handle_at_modemreset(enum at_cmd_type type)
 ++step;

 ret = nrf_modem_lib_init();
+


It could be good to add a small comment saying that nrf_modem_on_dfu_res() will get indirectly called to help follow the logic of the fota_* variables (which was already messy).

tomi-font · 2023-09-06T11:34:17Z

include/modem/nrf_modem_lib.h

+ .callback = _callback, \
+ .context = _context, \
+ };
+


nrf_modem_lib_init()'s description probably has to be updated (at least regarding the return values)?

tomi-font · 2023-09-06T11:36:26Z

applications/serial_lte_modem/src/main.c

+ LOG_INF("nRF Modem firmware update succeeded");
+ if (fota_stage == FOTA_STAGE_ACTIVATE) {
+ fota_stage = FOTA_STAGE_COMPLETE;
+ fota_status = FOTA_STATUS_OK;
+ fota_info = 0;
+ }


Suggested change

LOG_INF("nRF Modem firmware update succeeded");

if (fota_stage == FOTA_STAGE_ACTIVATE) {

fota_stage = FOTA_STAGE_COMPLETE;

fota_status = FOTA_STATUS_OK;

fota_info = 0;

}

LOG_INF("nRF Modem firmware update succeeded");

fota_stage = FOTA_STAGE_COMPLETE;

fota_status = FOTA_STATUS_OK;

fota_info = 0;

Not that it should matter, but just to keep the logic as-is...

tomi-font · 2023-09-06T11:45:06Z

applications/serial_lte_modem/src/main.c

+ /* TODO: This should indicate to the host MCU that the modem is not initialized and
+ * whether it can be retried or if it is required to reprogram the modem firmware.
+ * Also, when the modem lib is not initialized, this will fail at start_execute().
+ */


There could be a log conditionally output (if we're here due to some reason other than the LOW_VOLTAGE error) to say something similar to line 269?
Otherwise, if LOW_VOLTAGE happened, then I think there is nothing to do as nrf_modem_on_dfu_res() should already have updated the fota_* variables appropriately. In that case the SLM should work normally, with the modem running its original firmware.

applications/serial_lte_modem/src/main.c

tomi-font

Good to me from the SLM's perspective.

MirkoCovizzi · 2023-09-11T13:06:44Z

applications/asset_tracker_v2/src/main.c

+{
+ int ret = nrf_modem_lib_init();
+
+ /* Handle return values*/


Suggested change

/* Handle return values*/

/* Handle return values */

MirkoCovizzi · 2023-09-11T13:10:16Z

samples/cellular/location/nrf9160dk_with_nrf7002ek.overlay

@@ -4,8 +4,16 @@
 * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
 */

+/* Use high drive mode on SPI pins that handle communication with nRF7002


Why does this change belong to this commit?

Uhm, this should be removed. Thanks!

lemrey · 2023-09-13T12:08:19Z

applications/asset_tracker_v2/src/main.c

+ int ret = nrf_modem_lib_init();
+
+ /* Handle return values */
+ switch (ret) {


this can be a simple if statement now

lemrey · 2023-09-13T12:09:06Z

applications/asset_tracker_v2/src/main.c

- * considered irrecoverable and a reboot is needed.
- */
+ /* Unknown DFU result code */
+ LOG_ERR("nRF modem DFU failed, error: %d", dfu_res);


Since these errors are defined in hexadecimal, we could print them as hexadecimal so it's easier to see what errors we get (applies everywhere else where we print these)

lemrey · 2023-09-13T12:13:14Z

lib/nrf_modem_lib/nrf_modem_lib.c

@@ -109,6 +112,15 @@ static void log_fw_version_uuid(void)
 }
 }

+static void nrf_modem_lib_dfu_handler(uint32_t dfu_res)
+{
+ LOG_DBG("Modem library dfu res %d", dfu_res);


nit: Modem firmware update result 0x%x

lemrey · 2023-09-13T12:13:59Z

samples/cellular/azure_fota/src/main.c

- LOG_INF("Modem firmware update successful!");
- LOG_INF("Modem will run the new firmware after reboot");
- k_thread_suspend(k_current_get());
+ LOG_DBG("nRF Modem firmware updated");


nit: why nRF here?

Applies to the rest of the PR as well

lemrey · 2023-09-13T12:19:17Z

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

- case NRF_MODEM_DFU_RESULT_AUTH_ERROR:
- case NRF_MODEM_DFU_RESULT_HARDWARE_ERROR:
- LOG_ERR("Modem FOTA error: %d", modem_lib_init_result);
+ } else if (modem_lib_init_result == -EIO || modem_lib_init_result == -EAGAIN) {


We are changing the behavior here.

Earlier, failing to update the modem firmware (for whatever reason) would let this function return _VALIDATE_FAIL.
Now, failing to update the modem firmware will let this function return _VALIDATE_FAIL only if the error is one of the two fatal errors, or low power. And actually in all those cases, we are unable to report that to the cloud in any way, since the modem can't be initialized.

If the modem init fails because of a FOTA error, we either return EAGAIN (retriable, e.g. voltage low error) or EIO (non retriable). If the error is something else, the modem has responded with an initialization error on the second init. So the update may have succeeded but the modem is not initialized.

what I am trying to say is that earlier NRF_MODEM_DFU_RESULT_UUID_ERROR resulted in NRF_CLOUD_FOTA_VALIDATE_FAIL, since the update failed.

Now we NRF_MODEM_DFU_RESULT_UUID_ERROR lets nrf_modem_lib_init return 0, and this function returns NRF_CLOUD_FOTA_VALIDATE_PASS, right?

Ah, I see, yes, you're right!

Guess we'll need a hook here as well then.

lemrey · 2023-09-13T12:19:47Z

tests/subsys/net/lib/lwm2m_fota_utils/src/firmware.c

@@ -641,7 +641,7 @@ ZTEST(lwm2m_client_utils_firmware, test_firmware_update)
 printf("State %d\r\n", state);
 zassert_equal(state, STATE_DOWNLOADED, "wrong result value");

- lwm2m_firmware_emulate_modem_lib_init(NRF_MODEM_DFU_RESULT_AUTH_ERROR);
+ lwm2m_firmware_emulate_modem_lib_init(-NRF_EAGAIN);


why -EAGAIN instead of -EIO?

MarkusLassila

SLM looks good.

Working around the modem firmware bug is now taken care of by the modem library. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

It is no longer necessary to call nrf_modem_init twice when applying modem firmware updates. Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

Add modem init return values. Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

eivindj-nordic self-assigned this May 30, 2023

eivindj-nordic requested review from lemrey, MirkoCovizzi and rlubos as code owners May 30, 2023 07:55

eivindj-nordic force-pushed the dfu_init_once branch from fcbb417 to 0e6162c Compare June 12, 2023 08:46

eivindj-nordic changed the title ~~[WiP] lib: nrf_modem: init once on dfu apply~~ lib: nrf_modem: init once on dfu apply Jun 12, 2023

eivindj-nordic force-pushed the dfu_init_once branch from 0e6162c to 492d59a Compare June 21, 2023 11:44

eivindj-nordic requested review from trantanen and tokangas as code owners June 21, 2023 11:44

tokangas reviewed Jun 21, 2023

View reviewed changes

trantanen reviewed Jun 21, 2023

View reviewed changes

samples/nrf9160/azure_fota/src/main.c Outdated Show resolved Hide resolved

eivindj-nordic force-pushed the dfu_init_once branch from 492d59a to 921f9aa Compare June 21, 2023 14:40

tokangas approved these changes Jun 22, 2023

View reviewed changes

eivindj-nordic force-pushed the dfu_init_once branch 3 times, most recently from 2da4291 to d79ba8d Compare July 18, 2023 08:41

eivindj-nordic force-pushed the dfu_init_once branch from d79ba8d to 28caa9c Compare July 24, 2023 08:43

eivindj-nordic force-pushed the dfu_init_once branch 3 times, most recently from c26df4c to 848b0f3 Compare August 24, 2023 12:44

eivindj-nordic force-pushed the dfu_init_once branch 3 times, most recently from 284e124 to a6ef103 Compare September 1, 2023 14:21

tomi-font self-requested a review September 6, 2023 10:47

tomi-font reviewed Sep 6, 2023

View reviewed changes

eivindj-nordic force-pushed the dfu_init_once branch from a6ef103 to 01c6545 Compare September 8, 2023 11:32

eivindj-nordic requested a review from MarkusLassila as a code owner September 8, 2023 11:32

VTPeltoketo mentioned this pull request Sep 8, 2023

applications: serial_lte_modem: Full modem fota support for SLM nrfconnect/sdk-nrf#11892

Merged

tomi-font reviewed Sep 8, 2023

View reviewed changes

applications/serial_lte_modem/src/main.c Show resolved Hide resolved

tomi-font approved these changes Sep 8, 2023

View reviewed changes

eivindj-nordic force-pushed the dfu_init_once branch 3 times, most recently from 67cecd2 to 01a9fa7 Compare September 11, 2023 07:35

MirkoCovizzi reviewed Sep 11, 2023

View reviewed changes

eivindj-nordic force-pushed the dfu_init_once branch from 01a9fa7 to 64dc437 Compare September 11, 2023 13:18

lemrey reviewed Sep 13, 2023

View reviewed changes

eivindj-nordic force-pushed the dfu_init_once branch from 64dc437 to a8e4ea5 Compare September 13, 2023 13:05

MarkusLassila approved these changes Sep 14, 2023

View reviewed changes

eivindj-nordic force-pushed the dfu_init_once branch from a8e4ea5 to a91c458 Compare September 14, 2023 12:46

tomi-font and others added 2 commits September 18, 2023 10:03

applications: serial_lte_modem: Remove modem delta FW update workaround

0b30d87

Working around the modem firmware bug is now taken care of by the modem library. Signed-off-by: Tomi Fontanilles <tomi.fontanilles@nordicsemi.no>

lib: nrf_modem: init once on dfu apply

bf8a8ea

It is no longer necessary to call nrf_modem_init twice when applying modem firmware updates. Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

eivindj-nordic force-pushed the dfu_init_once branch 2 times, most recently from 405fcdf to 1117c8d Compare September 18, 2023 10:55

lib: nrf_modem: update modem init return values

5e465f7

Add modem init return values. Signed-off-by: Eivind Jølsgard <eivind.jolsgard@nordicsemi.no>

eivindj-nordic force-pushed the dfu_init_once branch from 1117c8d to 5e465f7 Compare September 19, 2023 08:53

lemrey merged commit 76be615 into lemrey:libmodem Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: nrf_modem: init once on dfu apply #105

lib: nrf_modem: init once on dfu apply #105

eivindj-nordic commented May 30, 2023 •

edited

Loading

tokangas Jun 21, 2023

tomi-font Sep 6, 2023

tomi-font Sep 6, 2023

eivindj-nordic Sep 8, 2023

tomi-font Sep 6, 2023

tomi-font Sep 6, 2023

tomi-font Sep 6, 2023

tomi-font Sep 6, 2023

tomi-font Sep 6, 2023

tomi-font Sep 6, 2023

tomi-font left a comment

MirkoCovizzi Sep 11, 2023

MirkoCovizzi Sep 11, 2023

eivindj-nordic Sep 11, 2023

eivindj-nordic Sep 11, 2023

lemrey Sep 13, 2023

lemrey Sep 13, 2023

lemrey Sep 13, 2023

lemrey Sep 13, 2023

lemrey Sep 15, 2023

lemrey Sep 13, 2023

eivindj-nordic Sep 13, 2023

lemrey Sep 14, 2023

eivindj-nordic Sep 14, 2023

eivindj-nordic Sep 14, 2023

eivindj-nordic Sep 14, 2023

lemrey Sep 13, 2023

MarkusLassila left a comment

		mosh_error("FOTA: Expected DFU result from modem library, "
		"but no DFU was triggered");

	mosh_error("FOTA: Expected DFU result from modem library, "
	"but no DFU was triggered");
	mosh_print("FOTA: Modem firmware update successful!");

		@@ -250,17 +250,17 @@ static int handle_at_modemreset(enum at_cmd_type type)
		++step;

		ret = nrf_modem_lib_init();

lib: nrf_modem: init once on dfu apply #105

lib: nrf_modem: init once on dfu apply #105

Conversation

eivindj-nordic commented May 30, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomi-font 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

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

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

MarkusLassila left a comment

Choose a reason for hiding this comment

eivindj-nordic commented May 30, 2023 •

edited

Loading