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

Remove unsupported settings if a device doesn't support these settings / WIP PoC #1776

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ia
Copy link
Collaborator

@ia ia commented Aug 4, 2023

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?
    Remove "Profile Support" related data fully for unsupported hardware.

  • What is the current behavior?
    "Profile Support" related data populates internal structs which takes precious RAM & ROM.

  • What is the new behavior (if this is a feature change)?
    "Profile Support" related data removed & a bit more space available for more useful features.

  • Other information:

TL; DR: over-using of RAM & ROM for unused options should be addressed, in one way or another. Here is my rant...

Hello. I ended up with this as always by accident. I worked on adding one tiny additional boolean option into settings. As always, it doesn't fit for TS80P :(
I mean, without any managing code for a setting itself, just proper initialization in structs & "registration" in menu making a binary run out of size. Not knowing what else to optimize, I decided to make some kind of PoC. Yes, I know, it's horrible, awful, terrible, hacky, and so on and so forth. But the concept itself seems working: removing settings which are just physically not on a device allows to save not only flash but RAM as well.

As a proof for this proof-of-concept, here is all the successful builds with this PoC/WIP patch on top of discip/off-icon branch.

Tossing settings-related structs filled with unused data between RAM & ROM is getting a luxury on TS80P. However, I do realize that the way how this patch looks now it's barely possible to see it ending up in upstream. But I like to be a team player, I really do. Plus, the last thing I would want is to support some set of patches separately.

So, what are the technical realistic options?

  • add exceptions for every may-be-not-presented hardware setting (i.e., Hall Sensor, BLE, DC, etc.) in that mixture of python scripts & c/cpp headers (probably not an option)
  • holding for every device through configure.h their own pair of SettingsOptions / SettingsItemIndex (painful to debug in a case of problems)
  • generate the pair of SettingsOptions / SettingsItemIndex (half)dynamically (just like assertions inside Translation.LANG.cpp through python script but not sure on that as well)
  • ... something else which I'm probably missing?

Oh, and I have a couple of questions regarding those structs..:

  • is my understanding correct that while SettingsOptions is just a list of settings, physically values are located inside systemSettingsType struct? So, basically planar (as opposite to nested) allocation for settings stored on the flash is the following:
uint16_t versionMarker;
uint16_t value with length of settings
uint16_t value of soldering temp setting
uint16_t value of sleeping temp
uint16_t value of sleeping time
...
uint16_t value of Profile Cooldown Speed
uint16_t value with length of settings
uint32_t value with padding of FFFFFFFF
  • is my understanding correct that SettingsItemIndex mainly needed for translation purposes? Basically, it says that SettingsItemIndex::SleepTemperature holds long & short descriptions for sleeping temperature, right? But couldn't this struct has the same order of elements or maybe even share the same storage for indexes between SettingsItemIndex & SettingsOptions since the lists are identical and just representing IDs of settings elements?

Oh, and I know that recent builds from gui-dispatches reducing binary size for TS80P with very impressive numbers. But with this model we eventually going to the moment when every byte will be matter. And I just really like to make my hardware keep up & working as long as possible. Oh, and speaking about this and that "off icon" update. I like clear & obvious icons, notifications & messages in interfaces myself but if it will come to the choice between "eyecandy" interface and functionality I personally always prefer the latter. So, if adding "off icon" into source code takes so much space that now some builds are not fit, then maybe is it possible to make this change optional (i.e. on by default but possible to off at compile time to save space)?

As always, thanks for any feedback a lot. For the past few weeks I already learned so much!

/cc @Ralim & @discip

@Ralim
Copy link
Owner

Ralim commented Aug 4, 2023

Ah; yes this is on the todo for 2.23.

This design breaks the all settings shall always have same id's and space on all devices.

Longer term plan (after oled updates) is to end up with 3 sets of data that is pre-processed in python:

  • GUI & layouts
  • Translations and text
  • Settings

Since then we can tag out at build time if something is used or not; and then omit it entirely. As the bigger waste of space than this is the translation strings that are not used on the TS80P.

In terms of the pr, the index list doesnt end up in flash, those are just squashed to constant numbers during compile time; and we use them to ensure that in all the places we reference a setting its at the same offset.

In general its flash thats limited rather than rom; ergo why I dont trim or mess with the settings structure.

If I have an end goal its really to discontinue most of whats in configuration.h and extract it to yaml configs for each device to allow more specific building options. This is made messier by Miniware shiping accelerometers at random batch-to-batch.

@discip
Copy link
Collaborator

discip commented Oct 16, 2023

@ia
On this territory @Ralim is the one in the know!

@ia
Copy link
Collaborator Author

ia commented Dec 26, 2023

TL;DR: It's stalled but ongoing.

Sorry that I keep it open so long without any related activity. But to whom it may concern: if no one minds, I still would like to try & to test a couple of approaches addressing the issue in the future as soon as I will get enough of time to dig into this more deep & thoroughly. Thanks.

@ia ia closed this Jul 25, 2024
@ia
Copy link
Collaborator Author

ia commented Jul 25, 2024

Sorry, false alarm.

I still have very heavy motivation to implement this in one way or another, so I will try to find some time during next month. But currently I just royally messed up with git while was trying to revert the experimental changes in the branch for easiest sync with the current dev. :D

Now this branch should have "healthy" state. Therefore, I reopen this PR to avoid any further duplicates.

@ia ia reopened this Jul 25, 2024
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