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

OTA over HTTPS (MEGH-6086) #339

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

shreyash-b
Copy link

Description

  • This PR introduces additional component for performing OTA over HTTPS.
  • It can be used in cases when RainMaker is to be used only for device management and keeping a MQTT connection alive would be inefficient.
  • A relevant example is also added.

Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

- for checking if OTA validation is pending
- for erasing OTA rollback flag from NVS
@CLAassistant
Copy link

CLAassistant commented Oct 28, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title OTA over HTTPS OTA over HTTPS (MEGH-6086) Oct 28, 2024
ESP_ERROR_CHECK(wifi_init_connect());

// Needs to be done after WiFi is connected.
esp_rmaker_ota_https_enable(NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does https_enable needs to be done after Wi-Fi is connected?

Copy link
Author

Choose a reason for hiding this comment

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

It requires internet connection for validating the OTA firmware. (see here)


/* Actual url will be 59 bytes. */
char ota_report_url[64];
snprintf(ota_report_url, 64, "%s/%s", NODE_API_ENDPOINT_BASE, NODE_API_ENDPOINT_SUFFIX_REPORT);
Copy link
Contributor

Choose a reason for hiding this comment

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

You pre-calculate the required size for ota_report_url.

Copy link
Author

Choose a reason for hiding this comment

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

Required size remains the same everytime(as URL does not change) - 59 characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Please add a define for this.

@@ -83,6 +103,8 @@ typedef struct {
char *priv;
/** OTA Metadata. Applicable only for OTA using Topics. Will be received (if applicable) from the backend, along with the OTA URL */
char *metadata;
/** The Function to be called for reporting OTA status. This can be used if needed to override transport of OTA*/
esp_rmaker_ota_report_fn_t report_fn;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change if I'm not mistaken. Please, increase the component version and update in the dependencies accordingly.
Please add a line in the CHANGES.md as well

Copy link
Author

Choose a reason for hiding this comment

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

Updated the changelog.
I'm not familiar with versioning scheme for esp-rainmaker, can you suggest version to increment the component to?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do increase the minor version in idf_component.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @shreyash-b regarding #339 (comment) please increase the esp-rainmaker's compont minor version.

Copy link
Author

Choose a reason for hiding this comment

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

done

@shreyash-b
Copy link
Author

I'm incorporating suggestions as additional commits, will squash/rebase once everything looks good.

Copy link
Contributor

@vikramdattu vikramdattu left a comment

Choose a reason for hiding this comment

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

Few other minor comments. LGTM.

@shreyash-b
Copy link
Author

Now it should be good
As per I asked in a previous reply, can you sugeest the version to increase esp-rainmaker component to?

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.

3 participants