Move more things to Peripherals

[MabezDev]

We have a bunch of structs which emulate the singletons:

• DMA Channels - [DMA 7/8]: Remove Dma, move channels to Peripherals #2545
• SYSTIMER alarms
• TimerGroup timers
• MCPWN channels
• LED channels
• ?

With the merge of #2544, we can move a bunch of init code out of the top level driver and instead move these fake "singletons" into the real singleton block of Peripherals

[Dominaezzz]

How do you know when something in the top level driver is a fake singleton?

Are you gonna have a peripherals.UART_TX and peripherals.UART_RX?
Or peripherals.PARL_IO_TX and peripherals.PARL_IO_RX?
Or peripherals.DMA_CHANNEL_0_IN and peripherals.DMA_CHANNEL_0_OUT?
Or peripherals.GPIO4_IN_SIGNAL and peripherals.GPIO4_OUT_SIGNAL?

What about the shared state between these "singletons" you've listed? Is everyone going have to pay for locks now?

we can move a bunch of init code out of the top level driver

Where is this init code going to go? Sometimes you need to do some setup before splitting the driver up.

[MabezDev]

How do you know when something in the top level driver is a fake singleton?

It's not an exact science, but in general most things that have some form of "channel" where the actual part of the peripheral you want to use is split into channels. The systimer peripheral as a whole is not really that useful, but its comparators are, they're the "real" peripheral here.

What about the shared state between these "singletons" you've listed? Is everyone going have to pay for locks now?

They have to pay for that regardless, of whether the "singleton" is created in-driver or at the peripheral level.

Where is this init code going to go? Sometimes you need to do some setup before splitting the driver up.

That was poorly phrased on my part, I'm mostly talking about enabling the clock and resetting the peripheral

[bugadani]

Where is this init code going to go? Sometimes you need to do some setup before splitting the driver up.

You'll be seeing an idea of mine implemented for DMA, once we are merging PRs again.

[Dominaezzz]

They have to pay for that regardless, of whether the "singleton" is created in-driver or at the peripheral level.

No this depends on how the driver has been written. The driver could choose to hide the shared state with locks, or expose the shared state and have the user manage it.

The current drivers in the hal tend to do the former, which is fine for simplicity as there was always the option of writing a 3rd party driver that skipped the locks.

By splitting up the peripherals upfront like this, everyone has to pay for these locks.
It also makes it really awkward to write 3rd party drivers as they now have to somehow play nice with the hal in terms of sharing this state.

It also makes it harder to reason about what's going on with the hardware when you code.

Makes it harder to reason about what happens when you steal a peripheral.

Makes it harder to reason about when your peripheral is reset. All of a sudden, dropping and recreating my driver doesn't seem to reset the state of my peripheral. And by splitting the peripherals upfront users won't immediately be able to grok what's going on.

It's not obvious to me what upsides there are. Maybe shaving off a tiny bit of "boilerplate" but losing the current semantics is too high a price to pay for just that.

Please reconsider this upfront splitting. At least for the peripherals that that can be enabled/reset. I can only see it making life more difficult.

That was poorly phrased on my part, I'm mostly talking about enabling the clock and resetting the peripheral

Sure but where is that going to go? Is it going be called in esp_hal::init, then these peripherals are always on?

[MabezDev]

No this depends on how the driver has been written. The driver could choose to hide the shared state with locks, or expose the shared state and have the user manage it.

This is a bit of a strawman argument, I don't see any future where we leave the shared state to the end user. These locks already exist in many of the drivers listen above already, that's not going to change. 3rd party drivers can use the pac directly, so I don't see how this impacts esp-hal.

It also makes it harder to reason about what's going on with the hardware when you code.
Makes it harder to reason about when your peripheral is reset. All of a sudden, dropping and recreating my driver doesn't seem to reset the state of my peripheral. And by splitting the peripherals upfront users won't immediately be able to grok what's going on.

I don't really see the difference between the proposal, and how things are now. These fake singletons suffer all the same issues. If we had a drop impl on Alarm, it wouldn't reset the whole systimer peripheral (unless it was the last one to be dropped, see below), so whats the difference here?

Makes it harder to reason about what happens when you steal a peripheral

I suppose this one has some merit, but it again isn't all that different to how things currently work, its just that each driver has it's own way to "steal" one of these resources, usually an unsafe conjure method.

Sure but where is that going to go? Is it going be called in esp_hal::init, then these peripherals are always on?

No, refcounting in each channel driver (See #2544). This is required even if we don't go through with this proposal, we're just not doing it at all right now. Almost all the peripherals in that list once initialized are on. Adding a drop impl for the main struct that holds these fake singletons also does nothing to help us because the singletons can be moved out.

[Dominaezzz]

No this depends on how the driver has been written. The driver could choose to hide the shared state with locks, or expose the shared state and have the user manage it.

This is a bit of a strawman argument, I don't see any future where we leave the shared state to the end user. These locks already exist in many of the drivers listen above already, that's not going to change. 3rd party drivers can use the pac directly, so I don't see how this impacts esp-hal.

It doesn't impact esp-hal directly, it impacts interop with 3rd party drivers.
Perhaps I've misunderstood the point of the Peripherals struct. You use it to track ownership of peripherals right?
Sure, 3rd party drivers would use the PAC but how do they get ownership of the peripheral when users use them?

let ledc = CustomLedc::new(peripherals.LEDC);

Like that right? (Or is the thinking that 3rd party drivers must steal the peripheral? There's no docs about how this should work)

Assuming that's how it should work, then splitting the LEDC into multiple channels impacts this workflow.
How is the 3rd party driver supposed to safely take ownership of the whole peripheral now?

I don't really see the difference between the proposal, and how things are now. These fake singletons suffer all the same issues. If we had a drop impl on Alarm, it wouldn't reset the whole systimer peripheral (unless it was the last one to be dropped, see below), so whats the difference here?

The difference is, it's more obvious and intuitive to the user that reset happens when the peripheral is constructed.

let driver = Driver::new(peripherals.DRIVER_PERIPHERAL); // Reset happens where where the peripheral.* is provided.

let utilitya = UtilityA::new(driver.part1); // This is just a wrapper with a nicer interface, no reset here.
let utilityb = UtilityB::new(driver.part2); // Same deal here.

I suppose this one has some merit, but it again isn't all that different to how things currently work, its just that each driver has it's own way to "steal" one of these resources, usually an unsafe conjure method.

Same deal here, the driver decides the semantics of what conjureing does. The unsafe method will have some documentation describing what must be done to ensure conjuring is sound/safe. Stuff like "this channel shouldn't be used by anything else", "this channel should already be enabled/reset", "you must not be touching these subset of registers". These are things that may vary between drivers, and by splitting the peripherals upfront here means 3rd party drivers can't use different semantics. They're stuck with the choices made here if they want to avoid forcing their users to do unsafe to acquire the peripheral.

No, refcounting in each channel driver

Right. And for the init code (that happens after the enable/reset) like this, I'm assuming the thinking here is that when the first channel is created, it will be ran. Then when all the channels are dropped, and another "first" channel is created, it will be ran again. Then wrap the init in a lock so the two cores don't race.
I'm curious how interrupt handlers will fit in this situation, where multiple peripherals will now be sharing a handler...

Perhaps I was too focused on trying to justify my point that I didn't explain it at a high level.

Splitting up the peripherals like this gives 3rd party drivers are harder time.
There is something to be gained from safely taking ownership of a whole peripheral rather than parts.
There should be a doc/guidelines for how 3rd party drivers are expected to play nice with esp-hal.

Hopefully I've gotten my point across. Though I'm not optimistic since only one sentence had some merit.

[MabezDev]

To be frank, 3rd party drivers are the least of my concern right now. They can always just use the PAC directly. Anyone writing there own driver, will have the know how to get around the HAL and figure out how to make it work.

With that said, I've been playing with this a bit locally and it doesn't actually work that well for many of the driver listed above. GPIO worked well, and I still think there is merit in doing it for DMA, but the other drivers I'm not fully convinced of just yet.

Thanks for the input, I'm going to close this for now. We may revisit this discussion in the future.