-
Notifications
You must be signed in to change notification settings - Fork 27
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
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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.
src/hardware/serial_terminal.rs
Outdated
items: &[ | ||
&menu::Item { | ||
command: "reset", | ||
help: Some("Reset Stabilizer to force new settings to take effect."), |
There was a problem hiding this comment.
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 inminiconf
? I think there it'sitem
. - 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_args: &[&str], | ||
context: &mut Context, | ||
) { | ||
writeln!(context, "Available properties:").unwrap(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
This PR fixes #351 by storing the MQTT broker address and the stabilizer ID in the unused flash bank.
TODO: