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

Feature/serial settings #350

Merged
merged 12 commits into from
Dec 5, 2023
Merged

Feature/serial settings #350

merged 12 commits into from
Dec 5, 2023

Conversation

ryan-summers
Copy link
Member

This is a proof-of-concept using the serial-settings crate to see how well it works with the API.

cc @jordens

@ryan-summers ryan-summers requested a review from jordens December 4, 2023 13:14
@ryan-summers
Copy link
Member Author

@jordens I had to refactor to the latest versions in Stabilizer. Mind taking a second look here?

Copy link
Member

@jordens jordens left a comment

Choose a reason for hiding this comment

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

I'm confused. Don't we store the main board settings (broker etc) already in the eeprom? And with this now also in flash?
Could you document somewhere the overall architecture (what's stored where in which format, what's the precedence etc)?

src/hardware/serial_terminal.rs Show resolved Hide resolved
@ryan-summers
Copy link
Member Author

ryan-summers commented Dec 4, 2023

I'm confused. Don't we store the main board settings (broker etc) already in the eeprom?

Correct, we did store it in EEPROM, but the EEPROM was too short to contain i.e. a DNS-named broker, so this PR is migrating existing settings over to flash.

And with this now also in flash?

The intent of the design is to:

  1. Load initial settings from EEPROM (if present)
  2. Update settings from any flash-based values

That way, we don't cause users to lose their existing configs when switching from earlier booster firmware to latest. However, it does result in us carrying over a bunch of legacy EEPROM code for settings management and makes the whole process more complicated.

All new settings will be written to flash and EEPROM will essentially be "frozen" after this merge.

We could just entirely deprecate the old EEPROM settings and force users to reconfigure booster during the firmware update if we want to simplify.

Could you document somewhere the overall architecture (what's stored where in which format, what's the precedence etc)?

Yeah, this is a good idea. I'll add it to one of the settings modules.

@ryan-summers ryan-summers requested a review from jordens December 4, 2023 17:32
Copy link
Member

@jordens jordens left a comment

Choose a reason for hiding this comment

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

LGTM with these slight changes.

src/settings/global_settings.rs Show resolved Hide resolved
src/settings/global_settings.rs Show resolved Hide resolved
src/settings/global_settings.rs Outdated Show resolved Hide resolved
src/settings/global_settings.rs Outdated Show resolved Hide resolved
ryan-summers and others added 2 commits December 5, 2023 09:52
@ryan-summers ryan-summers added this pull request to the merge queue Dec 5, 2023
Merged via the queue into main with commit c58831b Dec 5, 2023
5 checks passed
@ryan-summers ryan-summers deleted the feature/serial-settings branch December 5, 2023 11:24
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