RFC: Provide consistent APIs for configuring clock sources for peripheral drivers

[jessebraham]

From this project's inception we've just been hard-coding the clock source for peripheral drivers (usually just using the default clock source). This has been done just to keep things simple. We've reached the point that I feel we should start thinking more about this. I think it should not be too difficult to provide consistent APIs for configuring clock sources.

(I have not thought about this much (or at all, really) yet but I will update this comment once I have a better idea of what we should actually do about this)

[playfulFence]

I guess conceptually it should be something like #1416, right?

[sunsoan]

I guess conceptually it should be something like #1416, right?

yes,some other peripheral also need a method to config clock source

[MabezDev]

I changed my mind, this isn't a beta blocker but a 1.0 blocker - I've added a note in #2495 (the only driver where we set the clock source) to mark it as unstable until we figure out a full solution for this.

[bjoernQ]

I'd say since drivers should take a config the clock source should be part of the config - that's also future proof since if drivers take a config but don't include a clock source yet we can add that later (since configs need to be non-exhaustive)

[MabezDev]

I'd say since drivers should take a config the clock source should be part of the config - that's also future proof since if drivers take a config but don't include a clock source yet we can add that later (since configs need to be non-exhaustive)

My concerns with this are:

• Is changing clock sources at runtime without resetting the peripheral:
  • Possible?
  • A good idea?
• If we agree on the above, how do we handle it in the case of split peripherals?
  • Should it just be ignored when applying config's to split halves?

[bugadani]

Is changing clock sources at runtime without resetting the peripheral:

• Possible?
• A good idea?

Where it isn't, we'll have to return a ConfigError. But generally it should probably be fine, at least when the peripheral is idle.

If we agree on the above, how do we handle it in the case of split peripherals?

I still think any kind of configurability that affects both halves at the same time is just a convenient way to write bugs. We can make it work by essentially locking in each operation, but I'm fairly sure we can just end up deadlocking that way. Ignoring "random" config options is simply confusing and essentially turns the config functions into some nondeterministic mess ("This function may sometimes not do what you want" is not the best UX we can provide).

[MabezDev]

I still think any kind of configurability that affects both halves at the same time is just a convenient way to write bugs

Maybe, but I can't think of a better way to do it.

I was thinking about structuring the Config a bit differently:

pub struct Config {
	baud: u32,
	rx: RxConfig {
		rx_fifo_full_threshold: u16,
		...
	},
	tx: TxConfig {
		...
	}
}

Where apply_config for the halves would just take the rx/tx side of the config. This still doesn't solve the problem of the shared config things, like baud etc. Any ideas?

Where it isn't, we'll have to return a ConfigError. But generally it should probably be fine, at least when the peripheral is idle.

I suppose this could work. I was also thinking about having a config that's slightly different to the normal config, that is only taken in the driver constructor with the various "one time settable" configs, like clock source - but maybe that's not a good idea.

[bugadani]

I think the most safe option is to not provide a stable way to configure split halves just now. Although I also think we shouldn't provide stable split halves in the first place but that may be stretching it a bit :)

[Dominaezzz]

For the shared config, the driver could be split in three parts. Tx, Rx and control. Then the user could update shared config using the control object and wrap it in a mutex if necessary.

[bugadani]

That is a good idea, which we can actually add later (by providing a three-way split alternative for those who need it). Together with #2904 I think we can even remove the UartTx/UartRx::new functions so that we don't have to think about potentially half-peripheral relevant configurations.

[Dominaezzz]

(by providing a three-way split alternative for those who need it)

This might end up looking like an eye sore when it's added. Having two split methods.
The rusty way would be to have one split method and use an underscore to ignore the 3rd item in the tuple.

[bugadani]

Hmm a three-way split doesn't solve the problem that we have with spooky action in the distance. While we don't have to split the config structure up, we still can affect the peripheral while it's busy.

[MabezDev]

I agree with @bugadani that split probably isn't ready let's mark it (and the halves) as unstable. I also like @Dominaezzz's idea of split also giving a control block for the "shared" stuff. This is somewhat getting away from the original issue here, so I've filed #2931.

[MabezDev]

I'm still a bit concerned with clock source being runtime settable via config.

Whilst we can return a config error, I can't help but wonder if the clock source should be taken with the constructor of the driver? This means clock source can only be set during initialization, and it means the peripheral is reset prior to the clock source (possibly) changing. We could even stub this out with an empty enum ClockSource that just use the default as it is now and punt the changing of sources to post 1.0.

Another option, which I'm less fond of, but I still think might be better than the config is to provide a with_clock_source builder lite method to the driver. This still might end in a user trying to change the clock source at runtime, but we can at least punt adding this (or mark it as unstable) to post 1.0.

Thoughts?

[bugadani]

I don't understand why we need to special-case the clock source config.

[Dominaezzz]

Afaia, there are two things to take into account when choosing a clock source.

1. Picking the right frequency that can be divided just right or as close as possible to the actual desired baud rate. (The LCD cam driver does this atm)
2. Some clocks are not available/running in some power states.

Due to number 2, runtime clock source changes are necessary I think, when transitioning in and out of different power states. Perhaps this decision can be made per driver.