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

esp_https_ota API might be calling custom http_client_init_cb after performing the request instead of before doing so. (IDFGH-8080) #9581

Closed
rommo911 opened this issue Aug 18, 2022 · 6 comments
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@rommo911
Copy link

Environment

  • Development Kit: ESP32-DevKitCe
  • Kit version (for WroverKit/PicoKit/DevKitC): v1
  • Module or chip used: ESP32-WROVER-I
  • IDF version (run git describe --tags to find it): v4.4.1
  • Build System: idf.py
  • Compiler version (crosstool-NG esp-2021r2-patch3) 8.4.0
  • Operating System: Windows
  • (Windows only) environment type: PowerShell
  • Using an IDE?: [Yes (please give details) : VsCode using IDF extension
  • Power Supply: usb external 5V

Problem Description

In the esp_https_ota_config_t there is a section for adding an http_client_init_cb to add custom headers to the http request.
i notices it did not work for me to add my custom authorization token when asking for a firmware file from my server
the i checked esp_https_ota.c and found out that the calling of this callback (http_client_init_cb ) is done after esp_http_client_perform() , this means the custom headers are added after the http is perfomred. is this normal ? i moved the line of the http_client_init_cb calling right befor the esp_http_client_perform() and after esp_http_client_init() and things worked fine for me.

Expected Behavior

calling the http_client_init_cb before esp_http_client_perform() in the esp_https_ota.c

Actual Behavior

the http_client_init_cb called after esp_http_client_perform() in the esp_https_ota.c

Steps to reproduce

step 1: typical use of esp_https_ota api with adding (http_client_init_cb ) to the handle.

// the code should be wrapped in the ```cpp tag so that it will be displayed better.
#include "esp_log.h"

static esp_err_t AddCutomHeaders(esp_http_client_handle_t http_client)
{
    esp_err_t err = ESP_OK;
    err = esp_http_client_set_header(http_client, "Authorization", "Token ***********************************");
    return err;
}
void app_main()
{
    std::string fname = "https://myserverThatUsesCustomHeadersAuthorization/ota.bin";
    esp_http_client_config_t http_ota{};
    memset(&http_ota, 0, sizeof(http_ota));
    http_ota.url = fname.c_str();
    esp_https_ota_config_t ota_config = {};
    ota_config.http_config = &http_ota;
    ota_config.http_client_init_cb = AddCutomHeaders; 
}

sdkconfig.txt

@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 18, 2022
@github-actions github-actions bot changed the title esp_https_ota API might be calling custom http_client_init_cb after performing the request instead of before doing so. esp_https_ota API might be calling custom http_client_init_cb after performing the request instead of before doing so. (IDFGH-8080) Aug 18, 2022
@hmalpani
Copy link
Contributor

Hello @rommo911 ,
Thanks for reporting the issue. Attaching a patch here. Can you verify if it solves the problem? Thanks!
IDFGH-8080.txt

@rommo911
Copy link
Author

hello @hmalpani ,
Thanks for your reply. yes this resolve the issue.
Thanks!
is it going to be pushed to branch or a tag ? i may have to use it with an IDF docker image ...

@hmalpani
Copy link
Contributor

@rommo911 Are you also using partial download feature with your example?

@rommo911
Copy link
Author

@hmalpani nope not at the moment.

@hmalpani
Copy link
Contributor

@rommo911 Can you verify once if partial download is enabled or not? I tested with both the cases and header was not added only when partial download was enabled.

@rommo911
Copy link
Author

@hmalpani actually sorry i was mistaken , the partial download was active indeed in my code. i will test again later today to disable it.

@espressif-bot espressif-bot added Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Aug 29, 2022
espressif-bot pushed a commit that referenced this issue Sep 3, 2022
…sp_http_client_perform()` instead of before.

Closes #9581
Jason2866 added a commit to Jason2866/esp-idf that referenced this issue Sep 3, 2022
* timer: propagate isr register failure

Closes espressif#9651

* mcpwm: fix multiplication overflow in converting us to compare ticks

Closes espressif#9648

* heap: remove misleading info about malloc being equivalent to heap_caps_malloc(p, MALLOC_CAP_8BIT)

The actual memory allocated for malloc() depends on a lot of factors, see heap_caps_malloc_default()

Closes espressif#7659

* esp-rom: fixed error in miniz header documention for tdefl_init

Closes espressif#8435

* temperature_sensor: Fix issue that value is not accurate on ESP32-S3

* esp_https_ota: fix bug where `http_client_init_cb` is called after `esp_http_client_perform()` instead of before.

Closes espressif#9581

* Tasmota changes

* Fix linker error for C3

* Avoid bootloop if chip is unknown
   In case the PSIRAM chip is unknown, return an error and disable PSRAM
   instead of calling abort() and causing a bootloop
* Support for xiaomi single core ESP32
* Fix linker error for rom_temp_to_power
* fix linker error r_lld_ext_adv_dynamic_aux_pti_process

* Hide download percent when not interactive

* list(APPEND esptool_elf2image_args --dont-append-digest)

* Use native Apple ARM toolchains

* add package.json

* add submodules

* 8575d75

Co-authored-by: morris <[email protected]>
Co-authored-by: Marius Vikhammer <[email protected]>
Co-authored-by: Cao Sen Miao <[email protected]>
Co-authored-by: Harshit Malpani <[email protected]>
Co-authored-by: Mahavir Jain <[email protected]>
Jason2866 added a commit to Jason2866/esp-idf that referenced this issue Sep 3, 2022
* timer: propagate isr register failure

Closes espressif#9651

* mcpwm: fix multiplication overflow in converting us to compare ticks

Closes espressif#9648

* heap: remove misleading info about malloc being equivalent to heap_caps_malloc(p, MALLOC_CAP_8BIT)

The actual memory allocated for malloc() depends on a lot of factors, see heap_caps_malloc_default()

Closes espressif#7659

* esp-rom: fixed error in miniz header documention for tdefl_init

Closes espressif#8435

* temperature_sensor: Fix issue that value is not accurate on ESP32-S3

* esp_https_ota: fix bug where `http_client_init_cb` is called after `esp_http_client_perform()` instead of before.

Closes espressif#9581

* Tasmota changes

* Fix linker error for C3

* Avoid bootloop if chip is unknown
   In case the PSIRAM chip is unknown, return an error and disable PSRAM
   instead of calling abort() and causing a bootloop
* Support for xiaomi single core ESP32
* Fix linker error for rom_temp_to_power
* fix linker error r_lld_ext_adv_dynamic_aux_pti_process

* Hide download percent when not interactive

* list(APPEND esptool_elf2image_args --dont-append-digest)

* Use native Apple ARM toolchains

* add package.json

* add submodules

* 8575d75

* Arduino tinyusb v0.14.0 stripped

Co-authored-by: morris <[email protected]>
Co-authored-by: Marius Vikhammer <[email protected]>
Co-authored-by: Cao Sen Miao <[email protected]>
Co-authored-by: Harshit Malpani <[email protected]>
Co-authored-by: Mahavir Jain <[email protected]>
espressif-bot pushed a commit that referenced this issue Sep 22, 2022
…sp_http_client_perform()` instead of before.

Closes #9581
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

3 participants