Skip to content
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

Closed
10 tasks done
mciantyre opened this issue Oct 24, 2020 · 8 comments
Closed
10 tasks done

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

mciantyre opened this issue Oct 24, 2020 · 8 comments
Assignees

Comments

@mciantyre
Copy link
Member

mciantyre commented Oct 24, 2020

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

  • is specialized for i.MX RT processors
  • could be used across higher-level i.MX RT libraries within our organization, and outside our organization
  • fully replaces the equivalent API provided by imxrt-ral, so that the user does not require the RAL
  • requires a higher-level, safe interface before exposing to end-users

Drivers that meet this criteria include

  • IOMUXC, released as imxrt-iomuxc
    • API for pad configuration and multiplexing, specific for our processors
    • Used in HAL, async HAL; depended on by teensy4-pins
    • Does not depend on the RAL
  • CCM, in development as imxrt-ccm
  • DMA, in development as imxrt-dma
    • Unsafe API for allocating DMA channels, and scheduling DMA transfers. Re-exported by HAL and async HAL behind a safe interface.
    • Developed in HAL, then ported to async HAL with minimal changes. There's much code and documentation that could be shared.
    • Will not depend on the RAL

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

  • designed for safety and ergonomics. Foundational, chip-specific drivers may not provide a safe interface, or may require a deeper understanding of the peripheral.
  • intended to abstract processor complexities behind interfaces, like embedded-hal. Foundational, chip-specific drivers expect you to deal with that complexity.
  • dependent on foundational, chip-specific drivers, and may re-expose the interfaces of these drivers. Foundational, chip-specific drivers may be used by themselves, possibly with no required dependencies.

Pros

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

  • We signal that these crates are for public use. Today's 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 (like teensy4-pins). I also intend to support imxrt-ccm and imxrt-dma for similar users.
  • We share capabilities within our organization, while ensuring that one use-case doesn't dominate development. I'd like to keep using imxrt-iomuxc in the async HAL, just as we use it within the HAL. By separating the repositories, we can ensure that changes to imxrt-iomuxc are not biased towards support of the HAL at the expense of the async HAL. This extends towards external users, too.
  • We can more easily track changes, discuss issues, add documentation, and maintain releases independent of the HAL.
  • Consistency in our HAL and async HAL APIs. If [WIP] RFC: construct drivers from RAL instances #92 is accepted, our driver initialization HAL APIs will resemble the proposed async HAL APIs. This design consistency, based on a common set of crates, could be nice for users.

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.

  • Changes require two separate commits in two separate repositories.
    • No way around this. Workaround includes instructing contributors to use local path dependencies / git dependencies when prototyping, if they prefer.
    • (I consider this a feature in the vein of "easily tracking changes" and making sure a driver's development isn't dominated by one use case.)
  • Path dependencies are not portable across development environments.
    • Path dependencies are solved by git dependencies.
  • Continuous integration is less obvious.
    • When the HAL depends on 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."
  • Requires more release coordination
    • No more difficult than releasing imxrt-iomuxc before the HALs, which we already do.
    • Still need to figure this out for imxrt-iomuxc and the async HAL support in today's model.
  • Inconsistent with imxrt-ral maintenance, which happens in this repository.
    • A similar approach might have us maintain the RAL in a separate repo. Out of scope until we have a stable design for RFC: A Split HAL #56.

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

  • port imxrt-iomuxc to a separate repo
    • ensure that we have no pending changes to imxrt-iomuxc
    • move the imxrt-iomuxc crate into a separate repo under the imxrt-rs organization
    • integrate the crate into the HAL
    • integrate the crate into the async HAL
    • improve the imxrt-iomuxc documentation, making it more useful for users and contributors (tracked in Revise documentation imxrt-iomuxc#2)
    • transition any IOMUXC issues (#87) to the new repo
  • publish imxrt-ccm in an imxrt-rs org repo
  • publish imxrt-dma in an imxrt-rs org repo
    • integrate crate into HAL
    • integrate crate into async HAL

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
Copy link
Member

teburd commented Oct 25, 2020

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

  • imxrt-ral
  • imxrt106(0/2)-hal

I will create a few additional repos as well

  • ixmrt101(0/1)-hal
  • imxrt-usb

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

@mciantyre
Copy link
Member Author

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

  • the common HAL and the chip-specific HALs would have a very tight integration that would benefit from colocation; or
  • the chip-specific HAL(s) would be so thin that a separate repository overhead wouldn't be worthwhile

@mciantyre
Copy link
Member Author

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

@mciantyre
Copy link
Member Author

mciantyre commented Oct 25, 2020

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
Copy link
Member

teburd commented Oct 25, 2020

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
Copy link
Member

teburd commented Oct 25, 2020

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
Copy link
Member Author

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants