Add separate config for Rx and Tx (UART)

[playfulFence]

Description

Proof of concept for #2931

[bugadani]

We won't be ableto implement embassy_embedded_hal::SetConfig for this :(

[playfulFence]

Yes, that's true. It exists for now because apply_config for UartRx wants symbol_length calculated. It's also possible to just pass the whole Config to that function, which is also ugly...

[bugadani]

Can't we read the symbol length from some register?

[playfulFence]

But does it make sense? I thought we're calculating symbol length ourselves only in symbol_length() function. So yes, during the peripheral initialisation we write the value into rx_tout_thrhd register, but afterwards, if we need to change Shared configuration, numbers will be different, so the value in the register will be wrong

[MabezDev]

I think that any change to Shared should result in a reload of tx/rx too, for this exact reason where symbol length (or other settings) may change based on the global config.

This might be hard to structure, but one idea I had could be:

All the apply_config logic, for shared, rx, and tx lives in the control struct. Any call to apply_config there would reload shared, and then rx/tx.

Inside the split halves, when apply_config is called it internally (this is the fuzzy bit that needs solving) calls the control struct rx/tx reload.

struct Control;

impl Control {
    pub fn apply_config(&mut self, config: Config) -> Result<(), Error> {
		/* TODO: shared config applied here */
		Control::apply_rx(config.rx)?;
        Control::apply_tx(config.tx)?;
        Ok(())
	} 

   /* Note: free standing function, not sure if this will be possible in reality? Might need some other solutions here 
   Probably just need to tell this function which uart the config is being applied to?
*/
	fn apply_rx(config: RxConfig) -> Result<(), Error> {}

    fn apply_tx(config: TxConfig) -> Result<(), Error> {}
}

pub struct UartRx;

impl UartRx {
    pub fn apply_config(config: RxConfig) -> Result<(), Error> {
        Control::apply_rx(config)?;
        Ok(())
    }
}

There are likely a number of challenges with this approach, and this doesn't even mention some form of internal locking (though maybe we don't need it if we disconnect the various pins as @bugadani suggested somewhere?).

[MabezDev]

I think that any change to Shared should result in a reload of tx/rx too, for this exact reason where symbol length (or other settings) may change based on the global config.

With this in mind, I don't think we need the Shared config, because any change to Shared will result in a full config change anyways? So I think we can drop the seperate Shared part and just make the fields part of Config.

[playfulFence]

I am a little confused now 🤔
I guess that Control struct should be public, right? Otherwise I guess it makes no sense. Then, in order to be able to edit uart registers it needs to have an access to Uart peripheral, otherwise raw access to its registers is dangerous.

Given that Control has no items, we can (?) pass UartTx/Rx to Control::apply_tx/rx/config (update: no we can't, UartTx and UartRx needs generics (mode) passed, which we can't have for unit struct Control). It should know which uart it will apply config to.

[MabezDev]

We need to take a step back here, the whole purpose of the parent issue, and this experimentation is to figure out how the config should look like. We're not actually implemented the 3 way split here, we haven't even decided if it's something we will do, it was just a suggestion from #1668 (comment) which prompted further discussion about how to structure the config.

I probably shouldn't have written the pseudo code above, I think that just confused things more than it needed to because it's all hypothetical right now. Sorry about that.

The discussion from #2965 (comment) makes it clear to me that Shared config is not needed because any change to those must result in rx/tx reloading it's config as well.

Therefore I think we should implement the original idea in the issue of just splitting the rx/tx configs and keeping the shared stuff inside the top level config. E.g:

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

[Dominaezzz]

Another idea. In addition to split, let's have an unsplit too, which users can use to set shared config.

Shared config aside, it's still worth having I think

[bugadani]

let's have an unsplit too, which users can use to set shared config.

This is a separate issue from this PR, but we could just as well implement a (&mut RX, &mut TX).apply_configuration(), so the users wouldn't need to unsplit, then split again.

[bugadani]

Wow this is probably more confusing to read than it should be - is this how rustfmt formats the code?

Scratch that, I don't like rx.apply_config(&Config)

[playfulFence]

@bugadani
Yes, was just about to write about rx.apply_config(&Config) too.

Now we can actually make it be rx.apply_config(&RxConfig, &Config) and this still will implement embassy_embedded_hal::SetConfig;.
PS: that's ugly
PPS: and doesn't make sense because RxConfig is already inside of Config 😆

[playfulFence]

Maybe that'll be better just so it's at least consistent? 🤷🏼

[bugadani]

Yup, and "consistent" would be to use the split configs for the split halves. I'll pass on the opportunity on this review, I am not sure how relevant my biases are in this case.