esp-hal
esp-hal copied to clipboard
esp-rs
Remove `new_with_config` constructors where possible
I think it might just be cleaner to have new always take a config, users can always just pass Default::default().
Related to #2010 and #2321
IMHO we should instead do new().with_config() but the compiler will first apply a default config so it's slightly less efficient... But I think I prefer the code bloat over the "required optional" parameter.
IMHO we should instead do new().with_config() but the compiler will first apply a default config so it's slightly less efficient... But I think I prefer the code bloat over the "required optional" parameter.
I agree, but with a small tweak. It should be apply_config and it can be applied anytime. (Related: #2416 )
We should probably put this in the API-GUIDELINES, but in general I think with_X makes sense on construction, and apply_ or other verb-based methods make sense post creation.
re "the compiler will apply a default config" we can also choose to roll with hardware default initially. This point of view would better align with individual setters, where there are no default values defined by the HAL.
If we choose to go with individual setters, I'd probably argue for only keeping apply_X(&mut self) variants. While it's nice to chain-construct a peripheral, it's also pretty trivial to do that with mutating setters, which are more flexible.
We should also think about what to do with SPI: currently we don't have an SpiDevice equivalent, which means we can't automatically reconfigure the bus for the devices' individual needs. A configuring version of ExclusiveDevice would align well with config structs and apply_config. So I guess there are pros and cons for each approach, the question is whether keeping a small API surface here is a good enough motivator to pick between them?
We should also think about split peripherals: what does apply_config(uart::config::Config) mean for UartTx, what does it mean for UartRx? What does it mean to change the baud rate for half of a peripheral, for example?
We should also think about split peripherals: what does apply_config(uart::config::Config) mean for UartTx, what does it mean for UartRx? What does it mean to change the baud rate for half of a peripheral, for example?
This is a good question, in embassy they treat it like a full apply on the whole Uart. I think I would also go down this route. Having to do Uart::unsplit(rx, tx).apply_config() or something like that would be quite terrible, for a number of reasons.