imxrt-rs / imxrt-hal

Rust for NXP i.MX RT
Apache License 2.0
131 stars 32 forks source link

RFC: Maintain i.MX RT-specific drivers in separate repos #94

Closed mciantyre closed 3 years ago

mciantyre commented 3 years ago

In #56, we discuss a split HAL, which will have a single HAL crate per i.MX RT processor family. The HALs will probably depend on imxrt-iomuxc, our custom driver for pad multiplexing and configuration.

We maintain imxrt-iomuxc in this repository. However, this issue proposes that we maintain imxrt-iomuxc, along with other foundational, chip-specific drivers, in separate repositories. The approach will have us maintain only the split HALs, the common HAL, and the RAL in this repository.

I define a "foundational, chip-specific driver" as a library that

Drivers that meet this criteria include

I do not consider HAL crates in the category of foundational, chip-specific drivers. HALs are

Pros

My thoughts for separating the development of these crates from the HAL:

Cons

This comes with some downsides. Namely, it's more difficult to integrate work into the HAL(s). However, after prototyping the async HAL, which depends on imxrt-iomuxc as a git dependency, I don't consider these to be difficult challenges. I've listed my thoughts on these downsides below the problem.

Alternatives

An alternative approach would have us maintain the foundational, chip-specific drivers in this repository. However, these drivers still be shared with the async HAL in the manner described above, which may relegate the async HAL to a "less important" project in our organization. Once we're sharing the crates across both HALs, there's no reason why the foundational crates should be maintained here, versus in the async HAL repository. (A further step might including moving the async HAL into this repository; however, I feel the projects are different enough to warrant a repository boundary and separate issue tracker.)

Another approach would do nothing. We continue maintaining imxrt-ccm and imxrt-dma as separate modules within each of the HAL and async HAL projects. I'm not opposed to this approach; I did this with the HAL's dma module to support async HAL prototyping. But, this results in code duplication, which could have been mitigated. Still need to figure out how imxrt-iomuxc is consumed by async HAL.

Next Steps

If accepted, I'll

We will maintain imxrt-iomuxc releases from the new repo. We can release imxrt-ccm and -dma once we explore the APIs in the HAL and async HAL.

teburd commented 3 years ago

I'm good with the above proposal, and as an additional set of steps I would say we go ahead and split this repo

I will create a few additional repos as well

I'm fine naming the hal's after the ref manual ids.

mciantyre commented 3 years ago

I support separating the RAL from the HAL repository, eventually. I'm not considering that in scope of this effort. I'm happy to wait until we deliver a unified RAL and / or split HAL.

What will be the value of separating the chip-specific HALs by repository? In #56, we note that we're modeling our split HALs on the nrf-hal model. The nrf-hal team maintains the common HAL, and the chip-specific HALs, in the same repository. I'm partial to that model. I'd think that

mciantyre commented 3 years ago

Updated the RFC to explicitly exclude HALs as a foundational, chip-specific driver. Sorry if that wasn't clear in the initial proposal!

mciantyre commented 3 years ago

Another way I'm thinking of foundational, chip-specific drivers is to ask if the common user would directly depend on the crate. It's unlikely that a common user would depend on the IOMUXC, CCM, and DMA drivers, unless they were designing a capability that required those chip-specific details.

The common user would indirectly depend on those three libraries, since they're needed for -- and possibly re-exported by -- the HAL. The APIs help the user reach a goal that the HAL supports, but the user doesn't care if they're a separate crate, or if they're just a HAL implementation detail.

teburd commented 3 years ago

I think those are all fair points. I might be reiterating below, but I do that to make sure I understand.

So then core (iomux, ccm, dma) and probably complex drivers (usb, eth, audio) have their own repos, while the aggregate HAL crates remain here with a common hal containing the simpler peripheral drivers (uart/i2c/spi/pwm/timers/adc) where colocation could be more beneficial for the time being, modeling nrf52-hal repo structure. Does that seem like a fair assessment?

I think that models what stm32-rs and nrf52-rs are mostly doing as well.

At some point in the future the imxrt-ral perhaps goes to its own repo as well, but that's not in this initial proposal.

I agree with all of the above

teburd commented 3 years ago

I do think given that other repos depend on the ral potentially, it makes sense to get that into its own repo soonish, perhaps before the next release? It's pretty stable in its current form, and even if we unify it, the way its being used currently isn't broken at all.

mciantyre commented 3 years ago

That's the thinking! Thanks for clarifying.

Happy to separate the RAL before our next HAL releases. I'll add a task for it.

mciantyre commented 3 years ago

We've split the foundational crates into their own repos, and completed all of the next steps (or defined them as separate efforts).