Add support for saving the active configuration to flash

[ryan-summers]

This PR fixes #859 by adding a platform command to save the active running settings to flash.

Below is output during testing. CH0 is configured as a PID with Kp=2 via MQTT and then the save-active command was used to update the flash-based settings.

> list
/dual_iir/afe/0: "G1" [default: "G1"]
/dual_iir/afe/1: "G1" [default: "G1"]
/dual_iir/iir_ch/0/0: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0} [default: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0}]
/dual_iir/iir_ch/1/0: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0} [default: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0}]
/dual_iir/allow_hold: false [default: false]
/dual_iir/force_hold: false [default: false]
/dual_iir/telemetry_period: 10 [default: 10]
/dual_iir/stream_target: {"ip":[0,0,0,0],"port":0} [default: {"ip":[0,0,0,0],"port":0}]
/dual_iir/signal_generator/0/signal: "Cosine" [default: "Cosine"]
/dual_iir/signal_generator/0/frequency: 1000.0 [default: 1000.0]
/dual_iir/signal_generator/0/symmetry: 0.5 [default: 0.5]
/dual_iir/signal_generator/0/amplitude: 0.0 [default: 0.0]
/dual_iir/signal_generator/0/phase: 0.0 [default: 0.0]
/dual_iir/signal_generator/1/signal: "Cosine" [default: "Cosine"]
/dual_iir/signal_generator/1/frequency: 1000.0 [default: 1000.0]
/dual_iir/signal_generator/1/symmetry: 0.5 [default: 0.5]
/dual_iir/signal_generator/1/amplitude: 0.0 [default: 0.0]
/dual_iir/signal_generator/1/phase: 0.0 [default: 0.0]
/net/broker: "mqtt" [default: "mqtt"]
/net/id: "04-91-62-d2-a8-6f" [default: "04-91-62-d2-a8-6f"]
/net/ip: "0.0.0.0" [default: "0.0.0.0"]

> platform save-active
Currently active settings saved.

> list
/dual_iir/afe/0: "G1" [default: "G1"]
/dual_iir/afe/1: "G1" [default: "G1"]
/dual_iir/iir_ch/0/0: {"ba":[2.0,0.0,0.0,-0.0,-0.0],"u":0.0,"min":-32767.0,"max":32767.0} [default: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0}]
/dual_iir/iir_ch/1/0: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0} [default: {"ba":[1.0,0.0,0.0,0.0,0.0],"u":0.0,"min":-32767.0,"max":32767.0}]
/dual_iir/allow_hold: false [default: false]
/dual_iir/force_hold: false [default: false]
/dual_iir/telemetry_period: 10 [default: 10]
/dual_iir/stream_target: {"ip":[0,0,0,0],"port":0} [default: {"ip":[0,0,0,0],"port":0}]
/dual_iir/signal_generator/0/signal: "Cosine" [default: "Cosine"]
/dual_iir/signal_generator/0/frequency: 1000.0 [default: 1000.0]
/dual_iir/signal_generator/0/symmetry: 0.5 [default: 0.5]
/dual_iir/signal_generator/0/amplitude: 0.0 [default: 0.0]
/dual_iir/signal_generator/0/phase: 0.0 [default: 0.0]
/dual_iir/signal_generator/1/signal: "Cosine" [default: "Cosine"]
/dual_iir/signal_generator/1/frequency: 1000.0 [default: 1000.0]
/dual_iir/signal_generator/1/symmetry: 0.5 [default: 0.5]
/dual_iir/signal_generator/1/amplitude: 0.0 [default: 0.0]
/dual_iir/signal_generator/1/phase: 0.0 [default: 0.0]
/net/broker: "mqtt" [default: "mqtt"]
/net/id: "04-91-62-d2-a8-6f" [default: "04-91-62-d2-a8-6f"]
/net/ip: "0.0.0.0" [default: "0.0.0.0"]

[jordens]

I think we're hitting the limits of the design here.

What if we drop the AppSettings in serial settings, as well as the backing copy in the Miniconf MQTT client, as well as this active_settings. The original requirement for multiple settings copies (IIRC only) arose because serialization/deserialization could take a while and locking the settings for that time would have starved the low latency process task. Maybe that doesn't scale.

If we can split that into only two settings structs, we might cover all bases. One active low latency that has very short lock durations, and a single "slower" backing one for serialization/deserialization/update tasks in (but not owned by) miniconf and serial settings/flash. The latter would be passed into the slow poll calls as &mut and flipped onto the low latency active.

For serial settings that would mean the operations actually operate on the single backing buffer (and implicitly on the low latency one). We'd have to make save explicit. It would also remove lots of docs and explanations around what which op actually affects.

For the mqtt client, a wrinkle would be around wanting to republish a setting when changed via USB. Or foregoing that for now and instead documenting the behavior. It would affect the miniconf crate as well.

In general passing &mut Settings into the different "frontends" does sound better than having them each own theirs.

There is also an uncertainty around handled_update in miniconf-mqtt. I wonder if we actually use that feature (validating/aborting an update) anywhere. Alternatively we can validate in the app after the change and then on failure copy back from the low latency settings. That would only prevent nack-ing the update (on validation failure) through the frontend.

[jordens]

Meh. There are at least four places where we keep the setting now...

I'd rather have the poll call into serial settings return some sort of flag that the app then uses to push the settings. Or save that flag in the platform.

[ryan-summers]

If we can split that into only two settings structs, we might cover all bases. One active low latency that has very short lock durations, and a single "slower" backing one for serialization/deserialization/update tasks in (but not owned by) miniconf and serial settings/flash. The latter would be passed into the slow poll calls as &mut and flipped onto the low latency active.

One key detail is to mention that the settings stored in the serial client are not equivalent to those in the Miniconf client. Those in the serial client represent the flash-state of the device, whereas those in the Miniconf client represent those settings that are configured by the MQTT broker. These often differ during operation of the system.

We likely don't want them to be the same thing either, or it may not be clear to the operator what is actually used during initialization.

[jordens]

It makes sense to say:

• There are the currently active settings and the startup settings in flash.
• On boot the startup settings become active (override defaults)
• Both MQTT and USB can change active settings
• Saving copies active settings to startup settings where they differ from the defaults.

This is exactly the cisco behavior AFAICT.

In our case there are two sets of settings active in memory (i.e. in RAM): the low-latency settings for process and the backing store. We guarantee that they are functionally synchronized.
This is in addition to the implied startup settings consisting of defaults plus overrides in flash.

I think we can do this. It clears up a lot of potential confusion, simplifies the docs, and removes at least two copies of the settings.

[ryan-summers]

@jordens We previously decided against allowing the user to modify the currently-operating settings via USB because of complications with MQTT (see #822 (comment)), specifically around behavior with retained settings.

I think any change in this direction requires a bit more design and analysis before we opt to go that route - I'm happy to look into it if you'd be interested in that behavior, but we should definitely flesh it out before jumping into an implementation.

[jordens]

All ack! Let's have a look.

[jordens]

You are right. I'm flip-flopping on this. I think the previous analysis was too strict and limiting and we can design something more feature-rich, more consistent, and imprive user experience. To tear apart the post that you quote:

There would quite some explaining, misunderstanding and potential frustration about why broker/ip need a reboot and e.g. streaming target doesn't.

Solved nicely by your design with the struct encapsulation/NetSettings.

Imagine a retained setting in MQTT. Now you change that same setting over serial. Do you republish?

I'd say no republishing. Does need explicit docs though. Setting individual settings over USB will likely be the use case when there is no network/MQTT connection. Copying active to initial settings is something that I can see others wanting to do as well. And precisely in this case there is no need to republish as they are the same.

Do you do so retained? If yes, how do you decide what's retained and what's not?

Not applicable.

Then you reboot the device. Loads the setting from flash, but the retained mqtt setting quickly overrides the flash setting. Confusion ensues (why did my setting via serial work just fine a while ago but again now doesn't).

This is the case already now. We need to be clear that flash settings are initial settings and that set, get, list, as well as mqtt settings (retained or not) apply to active settings.

In the spec above we probably would also decide that there is no way to access initial settings other than through copying active to initial. Offering a dump (list-initial) of the initial settings would presumably be simple though, or even showing differing initial setting as part of the list as we do for default. A copy initial to active (without a reboot) would be problematic because of the NetSettings. get-initial, set-initial could probably be done but I suspect might be confusing and not needed.

[ryan-summers]

The rust ownership model complicates this approach.

1. Currently, the serial-settings module owns a Settings structure that is composed of the app runtime settings (DualIir, Lockin) and the NetSettings.
2. The MiniconfClient owns an app runtime settings structure (DualIir, Lockin)

As a result, there are the following problems we need to solve:

1. Both the miniconf and serial-settings clients own their respective settings structures in order to mutate them when user IO is processed. Rust ownership models prevent two entities having mutable access to the same object, so if we have a single settings structure accessed via the USB and MQTT interface, it can't be owned (or mutably borrowed) by both of them.
2. The serial-settings client has a larger settings structure than the miniconf client. The serial-settings structure is a super-set of those in the miniconf client, so the miniconf client essentially takes an individual member of the serial-settings structure.

Given these problems, what likely makes the most sense is moving the Settings structure to be owned by the application (i.e. stored as an individual RTIC resource). This settings structure would then be mutably borrowed out to the miniconf and serial-settings clients when those clients process IO to allow them to mutate the structure. This requires a minor redesign of both the crates to accomplish this.

[jordens]

Exactly. That was proposed in #862 (review)

[ryan-summers]

The above design discussion is implemented in #872

[ryan-summers]

Closing this in favor of #872