[RFC] How to apply configuration

[MabezDev]

• Uart::new() should be infallible, only applying some invalid config should be invalid
• Remove the independent change_x mutator methods in favour of apply_config(&mut self, config: &Config) Flatten Uart module, remove unnecessary data, replace methods with apply_config #2449

[bugadani]

Should all other drivers be reworked to use a config struct? My main worry is that clock speed calculations on SPI aren't trivial and changing any configuration means changing everything with this approach.

Should we keep with_config that moves self, along with apply_config?

[bugadani]

Considering #2389 and the cost of a full reconfig, do we even want config structs? What if we got rid of them in favour of single option setters?

[MabezDev]

Considering #2389 and the cost of a full reconfig, do we even want config structs? What if we got rid of them in favour of single option setters?

Hmm, I quite like the idea of a config struct because it neatly packages everything in one place. In the case of Uart for example, we have to forward methods from UartRx to Uart and the same for UartTx. All three could just have one apply_config method and each will update there respective parts of the config.

Regarding the cost, we can actually be quite smart about this in the future. We can keep a copy of the current config and then only update the options that have changed. If we didn't want to pay the memory cost of storing the config in the driver, we could also just check the registers themselves (though this wouldn't work for registers that require some calculation from config inputs).

[bugadani]

Checking the registers before writing the same or different value into them doesn't make much sense :) The read-and-compare is probably more costly than a blind write/update.

On the other hand, if you only want to update a single field, you have to lug around all your previous configuration anyway, so why pay the memory cost twice? We can store the config in the driver, and read from there, but that might be wasted memory also if config update isn't needed.

[Dominaezzz]

My main worry is that clock speed calculations on SPI aren't trivial

There's a solution for this one. #1666 (comment)

[bugadani]

I would recommend always constructing a peripheral using Uart, disallow creating UartRx and UartTx directly. UartTx and UartRx have limited configurability, some of which don't make sense when the peripheral is split, and so disallowing constructors would prevent questions like "Why can I only configure UartTx baud rate using the constructor, but not setters mid-operation?".

[bugadani]

I'd like to batch #1919 into this issue. Not necessarily for UART, but mainly for SPI - we get per-device configuration for free using https://docs.embassy.dev/embassy-embedded-hal/git/default/shared_bus/asynch/spi/struct.SpiDeviceWithConfig.html

[bugadani]

Uart::new() should be infallible, only applying some invalid config should be invalid

We have a few options on how to apply initial configuration, but we'll have to pick which one we'll do:

1. new() -> Driver; and new_with_config(config) -> Result<Driver, Error> - default config is rarely what the user needs, we don't have to optimize for that
2. Only have new() -> Driver and apply_config - forces an extra call, which is probably fine for SPI shared bus but not really good elsewhere
3. new(config) -> Driver. We could panic if the driver is initialized with an invalid config as it's likely the user will just unwrap it and get the panic anyway.
4. new(config) -> Result<...> - probably the least offensive option. The compiler is often smart enough to remove the panic, but new().unwrap() isn't terribly elegant.

[bjoernQ]

My gut feeling is 4 but my common sense would prefer 3

I assume that people will use fixed configurations so they will see the panic early in development and not have devices crash randomly in the field weeks later

[bugadani]

I guess if we choose 3 and someone absolutely needs the driver to not panic but provide an error, they can create the driver with Config::default() and call apply_config with custom configuration. I expect in case bus configuration isn't known at build time (is read from some other source, like persistent storage) this might see some use. I consider this a special enough case to not actively optimize for (which would be option 4).

[JurajSadel]

I like the 3 the most.

[Dominaezzz]

I also like 3 most.

I sorta like 4 but I'd prefer if the error handling happened before creating the driver. (i.e. make it impossible to create an invalid config) which just makes it look like 3 anyway.

[bugadani]

(i.e. make it impossible to create an invalid config)

Most of the difficulty comes from baud rates, especially where the clock source is configurable. In that case, it is unfortunately rather difficult to check a config in compile time. (or even in runtime, pre-calculating values and changing clocks might end up causing surprises).

[Dominaezzz]

I mean something along the lines of Config::set_frequency returning a Result.

[playfulFence]

I also agree with the 3rd option the most.

[MabezDev]

I'm leaning towards 3, but I'm wondering about the case where driver initialization might happen later in the program. I see the value in panicking early if it's initialized early, but is it acceptable to panic later?

Another consideration, what if the config is stored on an eeprom at runtime, the next reboot the settings are loaded from there. If someone somehow stored an invalid config we'll essentially bootloop. The solution to this in this case is to always use apply and pass Default::default into the constructor, but it's worth thinking about.

[MabezDev]

Quoting our API-GUIDELINES

Prefer compile-time checks over runtime checks where possible, prefer a fallible API over panics.

I feel like this pushes us closer to 4 or 2, unless we want to rethink this guideline.

[bugadani]

We also have the following which conflicts a bit:

Design APIs in a way that they are easy to use.

Also note the two weakening conditions:

• Where possible
  Not every Config can be checked in compile time.
• Prefer
  This one is causing the conflict. I prefer ease of use over fallibility, but we also support fallible API (with apply_config here) if the user needs them, so I think we're set here.

[bugadani]

Just a reminder that we should spell out the decision taken here, especially since the last comment is essentially an objection to (3). I'm not confident whether I should treat this as a vote and move forward implementing (3).

[MabezDev]

Whilst 3 is my favorite ergonomically, I think to support a wide range of use cases we should go with option 4.

Option 3 relies to heavily on the fact a driver is created at the beginning of the program and lives the entire time. This doesn't play nicely with the peripheral pattern for example, and may end up with unexpected panics.

Option 3 also pushes users to emulate option 2 for the case where they don't want to deal with those panics, meaning they call with default config and use apply_config (applies config twice, not great.)

I sorta like 4 but I'd prefer if the error handling happened before creating the driver. (i.e. make it impossible to create an invalid config) which just makes it look like 3 anyway.

This would be nice, but I don't think it's possible to implement this for every driver, as it can depend on a lot of variables outside of what the config can see. One example would be to choosing a clock source that's not enabled, should the config be checking this and returning an error, or the driver? I would argue a config shouldn't be poking around in clock registers.

I think if we go with option 4 now, we can come up with some more ergonomic helpers to alleviate the pain of unwrap later down the line.