--slot argument unclear

[JPHutchins]

Both upload and upgrade commands take a --slot argument. I'm suggesting getting rid of that and instead calling it --image. The source of truth for how this is mapped is here:

config MCUBOOT_SERIAL_DIRECT_IMAGE_UPLOAD
    bool "Allow to select image number for DFU"
    depends on !SINGLE_APPLICATION_SLOT
    help
      With the option enabled, the mcuboot serial recovery will
      respect the "image" field in mcumgr image update frame
      header.
      The mapping of image number to partition is as follows:
        0 -> default behaviour, same as 1;
        1 -> image-0 (primary slot of the first image);
        2 -> image-1 (secondary slot of the first image);
        3 -> image-2;
        4 -> image-3.
      Note that 0 is default upload target when no explicit
      selection is done.

Probably the user should be informed that this MCUBoot kconfig needs to be enabled for their choice to be respected.

The reason I prefer --image to --slot is that if we use --slot, then --slot 0 and --slot 1 mean the same thing (see the kconfig snippet) which I think is confusing.

FYI @raykamp

[raykamp]

As smpmgr is a general-purpose tool, is there some value in being able to specify the default image (slot 0) ? I assume this default image index would be defined in the firmware, and presumably it’s value to maintain that as a SSOT. Is there another way for the PC (smp client) to detect the default image index? I.e. will there be feature parity with this change?

…

On Tue, Jan 2, 2024 at 5:35 PM J.P. Hutchins ***@***.***> wrote: Both upload and upgrade commands take a --slot argument. I'm suggesting getting rid of that and instead calling it --image. The source of truth for how this is mapped is here: config MCUBOOT_SERIAL_DIRECT_IMAGE_UPLOAD bool "Allow to select image number for DFU" depends on !SINGLE_APPLICATION_SLOT help With the option enabled, the mcuboot serial recovery will respect the "image" field in mcumgr image update frame header. The mapping of image number to partition is as follows: 0 -> default behaviour, same as 1; 1 -> image-0 (primary slot of the first image); 2 -> image-1 (secondary slot of the first image); 3 -> image-2; 4 -> image-3. Note that 0 is default upload target when no explicit selection is done. Probably the user should be informed that this MCUBoot kconfig needs to be enabled for their choice to be respected. The reason I prefer --image to --slot is that if we use --slot, then --slot 0 and --slot 1 mean the same thing (see the kconfig snippet) which I think is confusing. FYI @raykamp <https://github.com/raykamp> — Reply to this email directly, view it on GitHub <#3>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFPZEYJIAGYX75LLA3X52TYMSYWLAVCNFSM6AAAAABBKYUFJOVHI2DSMVQWIX3LMV43ASLTON2WKOZSGA3DGMJTGYZDAMY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[JPHutchins]

I think that the default image is always the first slot. I just noticed that the doc phrases it as "The mapping of image number to partition is as follows:"

So...

• "image number 0" -> default
• "image number 1" -> image-0
• "image number 2" -> image-1

This is sorta the source of the bug in smpmgr. The argment called --slot is being used as "image number".

• --slot 0 -> "image number 0" -> default
• --slot 1 -> "image number 1" -> image-0
• --slot 2 -> "image number 2" -> image-1

If the user specifies anything other than 0, then it tries to do a swap, but it's not a valid operation if --slot 1 was provided because the image requested to swap is already at image-0.

[JPHutchins]

Makes me want to keep it as slot but simply add 1 to what the user requests to translate it to "image number". If there's two images, then the valid options would be 0 and 1. This is how the SMP Server reports the slots anyway.

[JPHutchins]

I'm trying to figure out why the device is resetting if it gets an image number > what's there. I need to stop being lazy and use a debugger.

There are some clues though:

https://github.com/mcu-tools/mcuboot/blob/b994ba2ce29425587957dcbb6c96d4e1872b5737/boot/zephyr/flash_map_extended.c#L94-L124

That function is a bit sketchy. It can return -EINVAL but otherwise returns a partition ID.

https://github.com/mcu-tools/mcuboot/blob/b994ba2ce29425587957dcbb6c96d4e1872b5737/boot/boot_serial/src/boot_serial.c#L707-L709

So it is a problem that the return value of that function is not checked for -EINVAL here. It's sending -EINVAL as the id arg to flash_area_open:

int flash_area_open(uint8_t id, const struct flash_area **fap)
{
	const struct flash_area *area;

	if (flash_map == NULL) {
		return -EACCES;
	}

	area = get_flash_area_from_id(id);
	if (area == NULL) {
		return -ENOENT;
	}

	if (!area->fa_dev || !device_is_ready(area->fa_dev)) {
		return -ENODEV;
	}

	*fap = area;

	return 0;
}

It still should not cause a reset, it should cause get_flash_area_from_id to return NULL.

[JPHutchins]

Oh wait, when it sends a negative value to a uint8 is it setting it to 0 instead of wrapping? If it set to 0, then it would get a valid flash area, but the wrong one...

[JPHutchins]

I'm losin my gat damn mind: https://github.com/mcu-tools/mcuboot/blob/b994ba2ce29425587957dcbb6c96d4e1872b5737/boot/cypress/MCUBootApp/sysflash/sysflash.h#L50-L69