Only validate image when anti-rollback is enabled?

[RichPiano]

I'm using the example "advanced_https_ota" as a base for my app development. Recently I've gotten more and more confused by about following code passage which is triggered in main() after successful wifi connection is established:

Code: Select all

#if defined(CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE) && defined(CONFIG_BOOTLOADER_APP_ANTI_ROLLBACK)
    /**
     * We are treating successful WiFi connection as a checkpoint to cancel rollback
     * process and mark newly updated firmware image as active. For production cases,
     * please tune the checkpoint behavior per end application requirement.
     */
    const esp_partition_t *running = esp_ota_get_running_partition();
    esp_ota_img_states_t ota_state;
    if (esp_ota_get_state_partition(running, &ota_state) == ESP_OK) {
        if (ota_state == ESP_OTA_IMG_PENDING_VERIFY) {
            if (esp_ota_mark_app_valid_cancel_rollback() == ESP_OK) {
                ESP_LOGI(TAG, "App is valid, rollback cancelled successfully");
            } else {
                ESP_LOGE(TAG, "Failed to cancel rollback");
            }
        }
    }
#endif

It seems that the app only validates when both directives, CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE and CONFIG_BOOTLOADER_APP_ANTI_ROLLBACK are set. However, the documentation clearly states the following:

If the self-test has completed successfully, then you must call the function esp_ota_mark_app_valid_cancel_rollback() because the application is awaiting confirmation of operability (ESP_OTA_IMG_PENDING_VERIFY state).

Thus my question: Shouldn't I always confirm the image whenever CONFIG_BOOTLOADER_APP_ROLLBACK_ENABLE is set without the need for CONFIG_BOOTLOADER_APP_ANTI_ROLLBACK also to also be enabled? Is this a bug in the demo app?

[ESP_Sprite]

Agree that that line smells funny. I'll see if I can ask the person who wrote that line.

[ESP_Shubham]

Hi, thanks for reporting this issue.

I agree that image should also be validated when ROLLBACK is enabled. We were focusing on Anti-Rollback test while adding the change, due to which this condition was added.

I have added the fix in internal merge queue and is under review. It will be pushed to GitHub once all internal checks are passed.

Thanks,
Shubham

[RichPiano]

Thank you! In this case I can wholeheartedly remove this line from my own app