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.

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

dfu: dfu_target: New DFU target SMP #11992

Merged
merged 1 commit into from
Aug 21, 2023
Merged

Conversation

jheiskan81
Copy link
Contributor

Added new DFU target for update image to external MCU by using MCUMGR SMP client.

@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 Aug 9, 2023
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 9, 2023

Test specification

CI/Jenkins/NRF

  • Integration Platforms

CI/Jenkins/integration

Test Module File based changes Manually selected West overwrite
test-ci-nrfconnect-boot-fw-update X
test-fw-nrfconnect-chip X
test-fw-nrfconnect-nrf-iot_serial_lte_modem X
test-fw-nrfconnect-zigbee X
test-sdk-find-my X

Detailed information of selected test modules

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

@jheiskan81 jheiskan81 force-pushed the dfu_smp_target branch 2 times, most recently from 4797ce9 to c0f8226 Compare August 9, 2023 12:54
@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.

@jheiskan81
Copy link
Contributor Author

jheiskan81 commented Aug 10, 2023

@rlubos Please review. Looks like that AVSystem have some issue that LwM2M authentication & flash Test fail randomly. Looks like that Boostrastrap is allways passed but real DTLS connection timeout when it fail.

@rlubos
Copy link
Contributor

rlubos commented Aug 10, 2023

@rlubos Please review. Looks like that AVSystem have some issue that LwM2M authentication & flash Test fail randomly. Looks like that Boostrastrap is allways passed but real DTLS connection timeout when it fail.

Is this comment related to this particular PR? I don't see how Bootstrap/DTLS is releated to changes in dfu_target?

@jheiskan81
Copy link
Contributor Author

@rlubos Please review. Looks like that AVSystem have some issue that LwM2M authentication & flash Test fail randomly. Looks like that Boostrastrap is allways passed but real DTLS connection timeout when it fail.

Is this comment related to this particular PR? I don't see how Bootstrap/DTLS is releated to changes in dfu_target?

CI test was failing and I was looking for that.. DFU target uppdate is not affect that.

include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
include/dfu/dfu_target_smp.h Show resolved Hide resolved
}

if (dfu_target_reset_cb) {
upload_state.image_num = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also a bit flimsy, works OK for nrf52840dk in normal state but if someone decides to change MCUboot on that core to be e.g. direct XIP, they would want the opposite slot. Probably OK for initial commit but should have a jira created

subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
@jheiskan81 jheiskan81 force-pushed the dfu_smp_target branch 2 times, most recently from dca4dba to 6ea8b4e Compare August 11, 2023 11:23
@jheiskan81
Copy link
Contributor Author

@nordicjm thanks for comment's I have been fix those now. Reset do now erase and reset_command it do device reset which apply image update and exit from recovery mode.

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Some minor nits.

subsys/dfu/dfu_target/Kconfig Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
@jheiskan81
Copy link
Contributor Author

@nordicjm Thanks for comment's. I was removing erase from schedule update fail and fix your spotted issues.

@jheiskan81
Copy link
Contributor Author

@rlubos @nordicjm Please review. I have already stuff ready for next PR. I can update Change log If review is ready.

include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
subsys/dfu/dfu_target/src/dfu_target_smp.c Outdated Show resolved Hide resolved
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Aug 17, 2023
@github-actions github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Aug 17, 2023
@jheiskan81
Copy link
Contributor Author

Updated Change-log information about new DFU target

@jheiskan81
Copy link
Contributor Author

@annakielar @carlescufi @rlubos Please review.

|no_changes_yet_note|
* Added:

* New DFU SMP target for update image to external MCU by using MCUMGR SMP client.
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
* New DFU SMP target for update image to external MCU by using MCUMGR SMP client.
* A new DFU SMP target for update image to external MCU by using the MCUmgr SMP Client.

*
* @defgroup dfu_target_SMP SMP external MCU target
* @{
* @brief DFU Target for upgrades performed by SMP Client
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
* @brief DFU Target for upgrades performed by SMP Client
* @brief DFU Target for upgrades performed by SMP Client.

* Function that boots the target in MCUboot serial recovery mode, needed before sending
* SMP commands.
*
* @param[in] cb User defined target reset function for entering recovery mode
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
* @param[in] cb User defined target reset function for entering recovery mode
* @param[in] cb User-defined target reset function for entering recovery mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add missing full stops at the end of all full sentences in this file.

/**
* @brief Initialize dfu target SMP client
*
* @retval 0 If successful, negative errno otherwise.
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
* @retval 0 If successful, negative errno otherwise.
* @retval 0 on success, negative errno otherwise.

int dfu_target_smp_image_list_get(struct mcumgr_image_state *res_buf);

/**
* @brief Reboot SMP target device and apply new image.
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
* @brief Reboot SMP target device and apply new image.
* @brief Reboot SMP target device, and apply new image.

int dfu_target_smp_client_init(void);

/**
* @brief Read Image list
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
* @brief Read Image list
* @brief Read image list.

/**
* @brief Read Image list
*
* @param res_buf Image List buffer
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
* @param res_buf Image List buffer
* @param res_buf Image list buffer.

int dfu_target_smp_confirm_image(void);

/**
* @brief See if data in buf indicates MCUBoot style upgrade.
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
* @brief See if data in buf indicates MCUBoot style upgrade.
* @brief Check if data in buffer indicates MCUboot style upgrade.

Do you mean buffer or, e.g., buf parameter?

include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
include/dfu/dfu_target_smp.h Outdated Show resolved Hide resolved
@jheiskan81
Copy link
Contributor Author

@annakielar Thanks for review. I have fixed your proposed changes.

Added new DFU SMP target for update image to external MCU by using
MCUMGR SMP client.

Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>
@rlubos rlubos merged commit 7b698f8 into nrfconnect:main Aug 21, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants