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

Allow passing custom HTTPClient to HTTPUpdate #4959

Merged
merged 4 commits into from
Mar 18, 2021

Conversation

MohammedNoureldin
Copy link
Contributor

@MohammedNoureldin MohammedNoureldin commented Mar 18, 2021

This enables customizing HTTP headers which adds some extra flexibility.
This does not break anything of course because this change introduces a new constructor with a new additional HTTPClient argument for HTTPUpdate class.

@me-no-dev
Copy link
Member

Wouldn't it make sense to just add something like t_httpUpdate_return update(HTTPClient& http, const String& currentVersion); and t_httpUpdate_return updateSpiffs(HTTPClient& http, const String& currentVersion); which would call handleUpdate directly and not pass a WiFiClient (which might not be necessary when initializing the HTTPClient)?

@MohammedNoureldin
Copy link
Contributor Author

MohammedNoureldin commented Mar 18, 2021

@me-no-dev Well, why not. I pushed this edit as you suggested.

@@ -58,6 +58,11 @@ HTTPUpdateResult HTTPUpdate::update(WiFiClient& client, const String& url, const
return handleUpdate(http, currentVersion, false);
}

HTTPUpdateResult HTTPUpdate::updateSpiffs(HTTPClient& httpClient, const String& url, const String& currentVersion)
Copy link
Member

Choose a reason for hiding this comment

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

Does not need the url argument :)

t_httpUpdate_return update(HTTPClient& httpClient, const String& host, uint16_t port, const String& uri = "/",
const String& currentVersion = "");

t_httpUpdate_return updateSpiffs(HTTPClient &httpClient, const String &url, const String &currentVersion = "");
Copy link
Member

Choose a reason for hiding this comment

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

Does not need the url argument :)

@@ -68,6 +73,12 @@ HTTPUpdateResult HTTPUpdate::updateSpiffs(WiFiClient& client, const String& url,
return handleUpdate(http, currentVersion, true);
}

HTTPUpdateResult HTTPUpdate::update(HTTPClient& httpClient, const String& host, uint16_t port, const String& uri,
Copy link
Member

Choose a reason for hiding this comment

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

Does not need the host, port and uri arguments :)

@@ -86,6 +86,10 @@ class HTTPUpdate

t_httpUpdate_return updateSpiffs(WiFiClient& client, const String& url, const String& currentVersion = "");

t_httpUpdate_return update(HTTPClient& httpClient, const String& host, uint16_t port, const String& uri = "/",
Copy link
Member

Choose a reason for hiding this comment

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

Does not need the host, port and uri arguments :)

@me-no-dev
Copy link
Member

you forgot to remove some arguments there :)

@me-no-dev me-no-dev closed this Mar 18, 2021
@me-no-dev me-no-dev reopened this Mar 18, 2021
These arguments are not required when passing the HTTPClient instead
of WiFiClient.
@MohammedNoureldin
Copy link
Contributor Author

@me-no-dev oh sorry! I did that it in parallel with other work. I removed these redundant arguments. This should be fine now.

@me-no-dev me-no-dev merged commit 93bcf5f into espressif:master Mar 18, 2021
@me-no-dev
Copy link
Member

Thanks :) all good now!

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.

2 participants