LPSPI: embedded-hal 1.0 rework

[Finomnis]

I briefly looked this over, the delays in the SPI transaction look very strange, is there a particular reason those are there? Won't this imply the need for a timer queue or scheduler of some kind?

That seems like a pretty big ask of what amounts to a hal library?

I'm uncertain what you mean, to be honest. Which delays?

If you are taking about the example, the delay there is not part of our driver code, it comes from embedded-hal 1.0

[Finomnis]

I may be in the minority about the "give users low-level control" design decisions

I personally don't like this, because it introduces ways for the user to break the internal state of the driver. I kind of consider this a functional unsoundness. I found the existing driver very hard to use because it requires me to understand the peripheral, which I as a user don't want to have to understand. I felt I would have to write a driver for the driver to actually use it in a project.

I would personally rather go the other route - closing it off completely and then step by step adding features as required. But that's just my personal opinion.

[Finomnis]

Here's a rough outline of what it might look like:

Let me think about this. You might be onto something.

Be aware that:

• not every u8 supports dma. It only supports full dma if both the read and write buffer are of the same u32 alignment. Otherwise it will always be the case that we have to decide whether to dma read or write, both aren't possible if they are misaligned.
• there will always be a benefit from using interrupts, even in the dma case. For error handling and for the flush call.
• the few unaligned bytes before and after a transfer also benefit from interrupts, and would still be required on the dma version.

[teburd]

I think it’s cool that you are adding support for eh 1.0. I’d note that interrupts are not free of cost and sometimes it really is faster to poll. I saw this in a few spi peripherals in Zephyr that I’ve modified/reviewed. At times interrupts can cause slowdowns. Especially true when doing XIP or on cores with longer pipelines with icache/dcache involved like the M7

[Finomnis]

That might be true!

If we

• drop dma support for u8/u16
• assume busy waiting for hal-async's flush call is acceptable

Then we can go with @mciantyre's architecture proposal.

[Finomnis]

The biggest open question I still have is error handling. But I have to clarify:

• errors can only occur if NOSTALL is activated. Otherwise it's guaranteed that the overflow errors can never occur. So the entire error discussion is irrelevant if we don't allow this flag to be set. I would like to keep the possibility, though.
• without interrupts, errors would have to be polled repeatedly. According to the RM, if an error occurs, the transmission would have to be aborted immediately. If we relax this requirement for the dma case, this might be possible.

[mciantyre]

I found the existing driver very hard to use because it requires me to understand the peripheral, which I as a user don't want to have to understand.

We support embedded-hal for hiding the peripheral's complexity. We still need to give the user APIs for driver configurations, since embedded-hal doesn't help us here. But after you configure your driver and pass ownership into your embedded-hal-using component, it should become difficult to break the driver's internal state.

I'm sure there's use-cases I'm not considering, but that's the thinking for giving users the lower-level control.

there will always be a benefit from using interrupts, even in the dma case. For error handling and for the flush call.

We could require the user to transform InterruptTransmit into DmaTransmit, for example. Once DmaTransmit takes ownership of InterruptTransmit, it could select the I/O behaviors, wait for flush, check errors.

I’d note that interrupts are not free of cost and sometimes it really is faster to poll.

Good point. We're free to spin in async code if we find it profitable. We block the executor and other tasks.

[Finomnis]

But after you configure your driver and pass ownership into your embedded-hal-using component, it should become difficult to break the driver's internal state.

I agree. So we should maybe look into something like a builder pattern?

[Finomnis]

@mciantyre Might have to take a step back over the next couple of days/weeks. If someone has an idea, be free to use my code as a base/reference. There are a couple of things in there that took a while to figure out, like the proper settings of the timing registers and the clock multiplier calculation, which I think are improvements over the original version in a couple of details.

If I do continue (which I will if nobody else wants to take over), I might start incorporating a couple of improvements into the existing driver if you think it would be better to start with the existing one as a low-level backend. We could then add a high-level driver which takes ownership of the low-level one that then can do Embedded Hal Async stuff. Although @mciantyre I would need some guidance on the naming of things, because I feel like both the low level and the high level driver deserve the name Lpspi.

[Finomnis]

Moved discussions over to #147.