-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: Maintain i.MX RT-specific drivers in separate repos #94
Comments
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. |
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
|
Updated the RFC to explicitly exclude HALs as a foundational, chip-specific driver. Sorry if that wasn't clear in the initial proposal! |
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. |
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 |
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. |
That's the thinking! Thanks for clarifying. Happy to separate the RAL before our next HAL releases. I'll add a task for it. |
We've split the foundational crates into their own repos, and completed all of the next steps (or defined them as separate efforts). |
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 maintainimxrt-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
imxrt-ral
, so that the user does not require the RALDrivers that meet this criteria include
imxrt-iomuxc
teensy4-pins
imxrt-ccm
ccm
module of async HAL, but considered for HAL integration in [WIP] RFC: construct drivers from RAL instances #92imxrt-dma
I do not consider HAL crates in the category of foundational, chip-specific drivers. HALs are
embedded-hal
. Foundational, chip-specific drivers expect you to deal with that complexity.Pros
My thoughts for separating the development of these crates from the HAL:
imxrt-iomuxc
crate appears to be an implementation detail of the HAL. However, I'm happy to support it for others who want to use it outside of our HALs, or to develop more convenience APIs for their systems (liketeensy4-pins
). I also intend to supportimxrt-ccm
andimxrt-dma
for similar users.imxrt-iomuxc
in the async HAL, just as we use it within the HAL. By separating the repositories, we can ensure that changes toimxrt-iomuxc
are not biased towards support of the HAL at the expense of the async HAL. This extends towards external users, too.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.imxrt-iomuxc
as a git dependency, it will ensure that the APIs work as expected. CI doesn't change.imxrt-iomuxc
, and the other foundation drivers, should be tested in isolation, without us relying on "it works in the HAL." Relates back to "dominate development for a single use-case."imxrt-iomuxc
before the HALs, which we already do.imxrt-iomuxc
and the async HAL support in today's model.imxrt-ral
maintenance, which happens in this repository.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
andimxrt-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'sdma
module to support async HAL prototyping. But, this results in code duplication, which could have been mitigated. Still need to figure out howimxrt-iomuxc
is consumed by async HAL.Next Steps
If accepted, I'll
imxrt-iomuxc
to a separate repoimxrt-iomuxc
imxrt-iomuxc
crate into a separate repo under theimxrt-rs
organizationimxrt-iomuxc
documentation, making it more useful for users and contributors (tracked in Revise documentation imxrt-iomuxc#2)imxrt-ccm
in animxrt-rs
org repoimxrt-dma
in animxrt-rs
org repoWe will maintain
imxrt-iomuxc
releases from the new repo. We can releaseimxrt-ccm
and-dma
once we explore the APIs in the HAL and async HAL.The text was updated successfully, but these errors were encountered: