Skip to content

Conversation

john-michaelburke
Copy link
Collaborator

@john-michaelburke john-michaelburke commented Dec 17, 2021

  • Use reqwest for downloading firmwareCPP-502
  • Fix backslashes for stings in UpdateTabCPP-505

@john-michaelburke john-michaelburke changed the title Use reqwest for downloading firmware[CPP-502][CPP-505] Use reqwest for downloading, standardize paths in UpdatePane[CPP-502][CPP-505] Dec 17, 2021
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/CPP-502 branch from d9f0d58 to 9aa41cd Compare December 17, 2021 21:43
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/CPP-502 branch from 9aa41cd to ee28bb5 Compare December 17, 2021 22:12
@silverjam
Copy link
Contributor

silverjam commented Dec 18, 2021

If the UpdateDownloader is allocated "statically" (as in, it sticks around for the whole app) it could account for the memory increase. We can switch to a dynamic allocation if that's the case (and drop or zero the downloader after using it) and it should resolve the memory issue.

@silverjam
Copy link
Contributor

This PR walks back some dep updates, not sure if that was intentional, fix proposed here: #303

@john-michaelburke john-michaelburke force-pushed the john-michaelburke/CPP-502 branch from c71731f to adde6a0 Compare December 19, 2021 03:18
@silverjam
Copy link
Contributor

silverjam commented Dec 19, 2021

Something else must be statically/globally allocating memory in the chain of new libraries pulled in by reqwest. Reqwest depends on Tokio, which is a new, large dependency for us. Maybe we cant try https://crates.io/crates/curl? We'd probably need the the "ssl" and "static-curl" features.

@silverjam
Copy link
Contributor

Attempt to use curl here: #304

@silverjam
Copy link
Contributor

silverjam commented Dec 19, 2021

Attempt to use curl here: #304

Same failure, so my theory doesn't seem to be correct, perhaps the increase was unrelated to the changes here, but still curious... but I don't think we should block this change over 9mb of memory though (and as long as this isn't indicative of a leak) I think we should just bump up the memory limit.

Also, if curl is still faster than minreq I think we should keep it, it's a much smaller dependency than reqwest (if we were already using tokio then it would not be an issue, but we've avoided tokio up to this point).

* try using curl to download piksi firmware

* bump max mem threshold
@john-michaelburke john-michaelburke changed the title Use reqwest for downloading, standardize paths in UpdatePane[CPP-502][CPP-505] Use curl for downloading, standardize paths in UpdatePane[CPP-502][CPP-505] Dec 20, 2021
@john-michaelburke john-michaelburke merged commit 958c8ae into main Dec 20, 2021
@john-michaelburke john-michaelburke deleted the john-michaelburke/CPP-502 branch December 20, 2021 03:34
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