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

Fix for /config on 8285 based devices #2026

Merged
merged 2 commits into from Jan 2, 2023

Conversation

pkendall64
Copy link
Collaborator

8285 devices don't have enough memory (RAM) to allocate a 32k JSON object! And we don't need that much on an RX anyway.

@deadbytefpv
Copy link
Contributor

Tested with both an EP1 and an RP3 that wouldn't show any config parameters on previous master (1d58dca)

@CapnBry
Copy link
Member

CapnBry commented Dec 31, 2022

This works on:

  • ESP32 Vario
  • ESP EP2
  • ESP TX (export is 1827 bytes)

However, if everything fits in 2KB on all platforms, why is it allocating 32KB at all? Also the allocation of options is also 2KB, so can't that be reduced as well?

I missed where the model configurations export were added, but the button calls them "model configurations" and the import line below them call them "models configuration", so which is it? 😅 It would have been smarter at the time to not spell-out-every-parameter-in-the-longest-way-possible 64 times, to save RAM allocation and minor code space. Can we change this before 3.2? RAM is a limited resource and this is just shooting itself in the foot.

@pkendall64
Copy link
Collaborator Author

This works on:

  • ESP32 Vario
  • ESP EP2
  • ESP TX (export is 1827 bytes)

However, if everything fits in 2KB on all platforms, why is it allocating 32KB at all? Also the allocation of options is also 2KB, so can't that be reduced as well?

I missed where the model configurations export were added, but the button calls them "model configurations" and the import line below them call them "models configuration", so which is it? 😅 It would have been smarter at the time to not spell-out-every-parameter-in-the-longest-way-possible 64 times, to save RAM allocation and minor code space. Can we change this before 3.2? RAM is a limited resource and this is just shooting itself in the foot.

I'm not sure how you'd want to minimise the parameter names. JSON is a bit wordy, but I wanted to keep the file at least semi human readable.

The export code is only running on TX modules, which are ESP32 and they have 520kb of RAM, so plenty to go around.

Copy link
Member

@JyeSmith JyeSmith left a comment

Choose a reason for hiding this comment

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

I did a lot of 3.2 testing with this PR. No probs.

@CapnBry
Copy link
Member

CapnBry commented Jan 2, 2023

The export code is only running on TX modules, which are ESP32 and they have 520kb of RAM, so plenty to go around.

I just feel like there's always "plenty of RAM" until there isn't and someone needs to change the one thing that pushed it over the limit instead of optimizing along the way. Things like the power object having "power" included in the sub items is redundant, telemetry-ratio could just be "ratio" or even "tlmratio", dynamic-power and boost-channel being redundant (and we can't even store both of these independently), etc. Also there is DUPLETX_ESP which is a TX that isn't ESP32, which I know have been used in a few old controller upgrades / internal modules.

Anyway, that's all only tangential to this so, working is better than not working. Tested on a couple other rando things over the weekend too.

@JyeSmith JyeSmith merged commit 8fbf2a3 into ExpressLRS:master Jan 2, 2023
@pkendall64 pkendall64 deleted the fix-wifi branch January 3, 2023 21:22
JyeSmith pushed a commit that referenced this pull request Jan 15, 2023
* Fix for /config on 8285 based devices

* Fix naming of button/upload text
qqqlab pushed a commit to qqqlab/ExpressLRS that referenced this pull request Feb 18, 2023
* Fix for /config on 8285 based devices

* Fix naming of button/upload text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants