Skip to content

Conversation

@nashif
Copy link
Member

@nashif nashif commented Feb 18, 2019

This function conflicts with a function of the same name in mcuboot. This happens when building USB DFU support into mcuboot.

DFU over USB uses image manager and mcuboot internals to manage images downloaded to the device.

@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #13475 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #13475   +/-   ##
=======================================
  Coverage    52.5%    52.5%           
=======================================
  Files         318      318           
  Lines       46526    46526           
  Branches    10754    10754           
=======================================
  Hits        24429    24429           
  Misses      17198    17198           
  Partials     4899     4899

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9e73c9...99c9553. Read the comment docs.

@nvlsianpu
Copy link
Contributor

They conflict if we build the bootloader
with USB DFU.

What a bootloader? It is something else than mcuboot?

@nvlsianpu
Copy link
Contributor

Honestly - this change is wired - better to change function names in application your attempt to build, or if this is relay impossibly - change names here.

@nashif
Copy link
Member Author

nashif commented Feb 18, 2019

What a bootloader? It is something else than mcuboot?

yes, mcuboot.

@nashif
Copy link
Member Author

nashif commented Feb 18, 2019

Honestly - this change is wired - better to change function names in application your attempt to build, or if this is relay impossibly - change names here.

dfu usb depends on image manager and pulls mcuboot.c, mcuboot.c duplicates function names in mcuboot, but those are only used when you have the application use mcumgr (which does not make sense to enable in the bootloader), so just use those duplicated functions with mcumgr and not when building mcuboot. I do not see anything weird about that.

@nvlsianpu
Copy link
Contributor

Don't see such file name in mcuboot mainline - so this is something you working on.

USB DFU (DFU class instead of serial recovery?) in bootloader should use mcuboot primitives instead of the zephyr primitives.

@nashif
Copy link
Member Author

nashif commented Feb 18, 2019

Don't see such file name in mcuboot mainline - so this is something you working on.

boot_swap_type is both in Zephyr and in mcuboot.

USB DFU (DFU class instead of serial recovery?) in bootloader should use mcuboot primitives instead of the zephyr primitives.

it is up to me to decide how I want to build my bootloader and what features I want to add in.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. These are static and there are no functions by the same name anywhere else in the tree. Why is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Missed the thread on this.

Can we just rename them in this file to not conflict or something, since they're static? No need for extra ifdefing.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, renaming is probably a better option, I thought there might be some purpose behind the same names. Btw, we have functions defined in the is file that are used no where in the tree. dead code basically, i.e boot_read_bank_header

Copy link
Member Author

Choose a reason for hiding this comment

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

since they're static?

btw, the main culprit is not static but if we ifdef it, the others will be become 'unused', hence why they all were ifdef'ed

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, renaming is probably a better option. I thought there might be some purpose behind the same names.

That is my opinion as well, so will approve. Name were the same as function does the same as in mucboot (and had no better name before) - only that.

This function conflicts with a function of the same name in mcuboot.
This happens when building USB DFU support into mcuboot.

DFU over USB uses image manager and mcuboot internals to manage images
downloaded to the device.

Signed-off-by: Anas Nashif <[email protected]>
@nashif nashif requested a review from dbkinder as a code owner February 18, 2019 19:46
@nashif nashif changed the title dfu: mcuboot: do not build code only needed by mcumgr dfu: mcuboot: rename boot_swap_type> mcuboot_swap_type Feb 18, 2019
@carlescufi carlescufi merged commit 996c252 into zephyrproject-rtos:master Feb 19, 2019
[MCUBOOT] [INF] boot_status_source: Scratch: magic=unset, copy_done=0xe, image_ok=0xff
[MCUBOOT] [INF] boot_status_source: Boot source: slot 0
[MCUBOOT] [INF] boot_swap_type: Swap type: test
[MCUBOOT] [INF] mcuboot_swap_type: Swap type: test
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not have been changed; those log messages are from mcuboot itself

Copy link
Contributor

Choose a reason for hiding this comment

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

fix covered in #13552

[MCUBOOT] [INF] boot_status_source: Scratch: magic=unset, copy_done=0xe, image_ok=0xff
[MCUBOOT] [INF] boot_status_source: Boot source: none
[MCUBOOT] [INF] boot_swap_type: Swap type: revert
[MCUBOOT] [INF] mcuboot_swap_type: Swap type: revert
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

Choose a reason for hiding this comment

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

fix covered in #13552

nvlsianpu added a commit to nvlsianpu/zephyr that referenced this pull request Feb 20, 2019
Reverted unwanted documentation change on usb dfu sample.
Changes was introduce accidentally within PR zephyrproject-rtos#13475

Signed-off-by: Andrzej Puzdrowski <[email protected]>
nashif pushed a commit that referenced this pull request Feb 21, 2019
Reverted unwanted documentation change on usb dfu sample.
Changes was introduce accidentally within PR #13475

Signed-off-by: Andrzej Puzdrowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants