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

Adding initial support for configuring parameters via USB #805

Merged
merged 18 commits into from
Nov 14, 2023

Conversation

ryan-summers
Copy link
Member

@ryan-summers ryan-summers commented Nov 10, 2023

This PR fixes #351 by storing the MQTT broker address and the stabilizer ID in the unused flash bank.

TODO:

  • Update code to use parameters stored in flash on startup
  • Verify parameters are not lost during flashing

src/hardware/flash.rs Outdated Show resolved Hide resolved
Copy link
Member Author

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

@jordens This is now a functional implementation. A lot of this could easily be abstracted out into a separate crate that we could share between stabilizer/booster/thermostat as well. My only current question is figuring out how to be generic over the flash settings structure. It's a similar problem to miniconf, but subtley different.

Alternatively, if we don't really care about losing settings when the settings structure is modified, we could just serialize the whole structure directly into flash, or we could implement update management directly in firmware.

src/hardware/flash.rs Outdated Show resolved Hide resolved
@ryan-summers ryan-summers marked this pull request as ready for review November 13, 2023 13:56
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.

Feel free to defer things and convert into future issues where you see fit.

items: &[
&menu::Item {
command: "reset",
help: Some("Reset Stabilizer to force new settings to take effect."),
Copy link
Member

Choose a reason for hiding this comment

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

  • Let's make this already device and application agnostic. s/Stabilizer/device.
  • Let's also add a command that resets the settings to their default (and distinguish clearly which command resets the CPU and which resets the settings).
  • Does the term property match the term we use in miniconf? I think there it's item.
  • Let's also state clearly here that these settings here are distinct from the application mqtt settings. Let's open an issue to discuss how we can use/expose the application settings through usb and save/load them in flash to be overriden by mqtt.
  • Also I'd like to consider making this menu interface to miniconf (plus maybe persistent storage) a agnostic thing (crate).

Copy link
Member Author

Choose a reason for hiding this comment

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

Export to a new crate is tracked in #808. All the other issues are addressed in 6f17197

src/hardware/serial_terminal.rs Outdated Show resolved Hide resolved
_args: &[&str],
context: &mut Context,
) {
writeln!(context, "Available properties:").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Can this also print the docstring (or some other struct-associated string) of the settings struct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, no. I think we could expand the miniconf macro to expose the docstrings as metadata as well, but that would be a modification of miniconf. I'll open an issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added quartiq/miniconf#187 for this functionality

Copy link
Member

Choose a reason for hiding this comment

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

I meant just the docstring of the struct, not of the fields. That would already cover a lot.

src/hardware/serial_terminal.rs Outdated Show resolved Hide resolved
src/hardware/serial_terminal.rs Outdated Show resolved Hide resolved
src/hardware/serial_terminal.rs Outdated Show resolved Hide resolved
jordens
jordens previously approved these changes Nov 14, 2023
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.

Nice.

@jordens jordens added this pull request to the merge queue Nov 14, 2023
@jordens jordens removed this pull request from the merge queue due to a manual request Nov 14, 2023
@jordens jordens added this pull request to the merge queue Nov 14, 2023
Merged via the queue into main with commit fa34e52 Nov 14, 2023
8 of 9 checks passed
@ryan-summers ryan-summers deleted the feature/flash-storage branch November 14, 2023 11:14
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.

Store device identifier and broker in non-volatile memory
2 participants