-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
applications: serial_lte_modem: Full modem fota support for SLM #11892
applications: serial_lte_modem: Full modem fota support for SLM #11892
Conversation
VTPeltoketo
commented
Jul 27, 2023
Test specificationCI/Jenkins/NRF
CI/Jenkins/integration
Detailed information of selected test modules Note: This message is automatically posted and updated by the CI |
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. |
dd659d6
to
f8ed3aa
Compare
f8ed3aa
to
742623b
Compare
ab8da56
to
332524f
Compare
This PR is now ready to review. |
I was also kind of waiting for there to be an actual overlay file. Were there no other Kconfigs that needed to be enabled? |
|
||
err = nrf_modem_lib_bootloader_init(); | ||
if (err != 0) { | ||
LOG_ERR("nrf_modem_lib_bootloader_init() failed: %d\n", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe nrf_modem_lib_init()
in that case to still try to exit from this function with a running modem?
And I think the same kind of fine tuning can be done to some of the other failure cases below.
Though I agree with your comment above, about the trickiness of this. And certain errors are likely not even possible during a modem firmware update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to reset the device if something goes wrong after nrf_modem_lib_shutdown? If something is truly wrong in the sequence, nrf_modem_lib_init() may not be successful either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we make reset here, it has to delayed one because we have to return error event. Sounds quite complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error event
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After FOTA and reset/modemreset, slm_fota_post_process() function is called. If the FOTA activation fails, slm_fota_post_process function sends an error AT event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. slm_fota_post_process()
is called later in the boot process and only depends on slm_fota_stage
not being FOTA_STAGE_INIT
to print the FOTA result. So it seems that @MarkusLassila's proposal to reset the device would be fine as the FOTA result would end up getting printed after the additional reset. Though one potential risk I see is entering into a reset loop, so it would be preferable to not attempt to do this more than once.
One scenario I can imagine working in these hypothetical failure cases is:
- Set
slm_fota_stage
toFOTA_STAGE_COMPLETE
, with the other FOTA variables indicating an error. - Save the FOTA settings.
- Reset the device.
- Avoid calling
slm_finish_modem_fota()
again. - Reach
slm_fota_post_process()
that will print the FOTA result. If for some reason the code fails before this, it means that something is terribly wrong and there is nothing we can do anymore as a reboot didn't help.
Step 4 would require changing main.c:445, probably to if (slm_fota_stage == FOTA_STAGE_ACTIVATE)
.
This is very hypothetical, but adds a safety net in case one of these calls in slm_finish_modem_fota()
would fail in a non-fatal way. Otherwise it's not impossible that the code would return from the function without a running modem. So doing something about it would allow SLM to kind of self-heal.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this might work. I have to simulate a finalization error and test. There are actually two different use cases to test, modem reset and device reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling of the full FOTA activation has been changed.
5beb85e
to
c80d1f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Please fix the compliance issues.
f9bfa96
to
14763d9
Compare
|
||
err = nrf_modem_lib_bootloader_init(); | ||
if (err != 0) { | ||
LOG_ERR("nrf_modem_lib_bootloader_init() failed: %d\n", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to reset the device if something goes wrong after nrf_modem_lib_shutdown? If something is truly wrong in the sequence, nrf_modem_lib_init() may not be successful either.
applications/serial_lte_modem/boards/nrf9161dk_nrf9161_ns.overlay
Outdated
Show resolved
Hide resolved
applications/serial_lte_modem/boards/nrf9160dk_nrf9160_ns.overlay
Outdated
Show resolved
Hide resolved
|
||
err = nrf_modem_lib_bootloader_init(); | ||
if (err != 0) { | ||
LOG_ERR("nrf_modem_lib_bootloader_init() failed: %d\n", err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error event
?
doc/nrf/releases_and_maturity/releases/release-notes-changelog.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs look good.
e634f71
to
a6b365d
Compare
The external flash is ereased automatically after a new firmware activation. | ||
|
||
The activation of a new full modem firmware is done identically to a modem delta update, by resetting either the whole device or only the modem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Activation' sounds weird to me here, but I can't find a clear precedent on the Cloud side.
My gut says application is a better word based on https://docs.nrfcloud.com/Devices/FirmwareUpdate/FOTAOverview.html, but I've also realized we don't ever actually say what this step is called. The entire process is deployment, and in some APIs the act of installing new firmware is called application, but that's still more general than what this doc is referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching 'activation' and 'application' would be confusing. Here 'activation' means the action to be made and 'application' the software itself which is downloaded.
@@ -47,10 +49,17 @@ Syntax | |||
|
|||
.. note:: | |||
|
|||
When doing modem FOTA, erasing the modem DFU area is optional since the update process will automatically erase the area if needed. | |||
When doing modem delta FOTA, erasing the modem DFU area is optional since the update process will automatically erase the area if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When doing modem delta FOTA, erasing the modem DFU area is optional since the update process will automatically erase the area if needed. | |
During a delta update to the modem over FOTA, the modem DFU area is automatically erased as needed as part of the update process. Erasing this area manually is optional. |
Trying to make the structure a little clearer. I hope I understood this correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
However, this leads to the command starting the update taking longer to complete, and also leaves the connection to the FOTA server idle while the area is being erased, which can provoke issues. | ||
|
||
.. note:: | ||
|
||
When doing full modem FOTA, external flash is used to store the firmware. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When doing full modem FOTA, external flash is used to store the firmware. | |
The firmware image is stored in external flash memory during a full modem update over FOTA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modem update over Firmware Over The Air
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, but the FOTA appreviation has been used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@melwee01 over FOTA
sounds really wrong to me. But I'll leave this to the documentation people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps 'using FOTA', then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed according to the teams chat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just a last little thing left.
@@ -230,7 +230,8 @@ static int handle_at_modemreset(enum at_cmd_type type) | |||
++step; | |||
|
|||
if (ret > 0 || (slm_fota_stage != FOTA_STAGE_INIT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the condition is slm_fota_stage == FOTA_STAGE_ACTIVATE
when doing an application reset, I suggest having the same here. Better yet, make a function that is called by those two places to make sure that they remain in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they can be in sync even if it does not affect to the execution. I will change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
However, this leads to the command starting the update taking longer to complete, and also leaves the connection to the FOTA server idle while the area is being erased, which can provoke issues. | ||
|
||
.. note:: | ||
|
||
When doing full modem FOTA, external flash is used to store the firmware. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@melwee01 over FOTA
sounds really wrong to me. But I'll leave this to the documentation people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One finding.
42d6bdf
to
8f50b55
Compare
.. note:: | ||
|
||
The firmware image is stored in external flash memory during a full modem update over FOTA. | ||
The external flash is ereased automatically after a new firmware activation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The external flash is ereased automatically after a new firmware activation. | |
The external flash is erased automatically after a new firmware activation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
The firmware image is stored in external flash memory during a full modem update over FOTA. | ||
The external flash is ereased automatically after a new firmware activation. | ||
|
||
The activation of a new full modem firmware is done identically to a modem delta update, by resetting either the whole device or only the modem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The activation of a new full modem firmware is done identically to a modem delta update, by resetting either the whole device or only the modem. | |
Activating the new full modem firmware is done identically to a modem delta update, by resetting either the whole device or only the modem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
- new user interface parameter for #XFOTA AT command - full modem fota is using an external flash - full fota is activated via overlay Signed-off-by: Veli-Tapani Peltoketo <veli-tapani.peltoketo@nordicsemi.no>
98409ef
to
1142e90
Compare