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: A Split HAL #56

Open
teburd opened this issue May 4, 2020 · 17 comments
Open

RFC: A Split HAL #56

teburd opened this issue May 4, 2020 · 17 comments

Comments

@teburd
Copy link
Member

teburd commented May 4, 2020

I've been mulling this over for awhile now, and really I'm starting to think...

We should have a HAL crate per chip, that depends on a common HAL (imxrt-hal) still.

My reasoning really comes back to the documentation part of the HAL, along with perhaps not wanting to rely too much on conditional compiles after all, at least at the end user supplied HAL crate.

So imagine the current HAL crate remains, but the Peripherals object that is built ends up in its own imxrt1062-hal crate which hides the need to set the feature flags up from end users.

@mciantyre
Copy link
Member

How granular should the new HAL crates be? Today's RAL and HAL have a feature-flag per chip variant; "imxrt1061" and "imxrt1062" specify unique targets. But, they're nearly identical, and they share a reference manual. Should we consider an imxrt106x-hal crate that can be used by both variants? We could handle discrepancies with HAL-specific feature flags. On a quick-pass through the datasheet, the 61 doesn't have the display and camera components (LCD, CSI, PXP) that the 62 supports. Those could be gated behind an optional feature-flag in the imxrt106x-hal crate. The approach still needs feature-flags, but they're truly for optional features, rather than a required flag for a hardware target.

Alternatively, we could do something similar to the stm32-rs project. The team has a HAL per family and Cortex-M types. Then, they require feature-flags to describe the specific chip they're using. The approach is similar to our current approach in that they require feature-flags to describe devices. But, they have smaller, more granular HAL crates for different chip lines. It might translate to imxrt101x-hal and imxrt106x-hal crates in our project. We could reach out to the team to see if they like their current approach, or if they have any regrets.

Any thoughts on project organization? Regardless on how we require feature-flags, I'm partial to a single repo with a top-level workspace, rather than multiple repos. The HAL crates will probably be developed together, so having them in the one repo will help us coordinate changes.

@teburd
Copy link
Member Author

teburd commented May 11, 2020

I like the idea of grouping things together when possible. The 1064 is seemingly a different beast than the 1061/1062, its probably the oddest duck in th the 106x set. There's also the really odd one part that has a DSP alongside it, which I don't really intend on supporting myself. The RAL script does some family segmentation already, but it could probably be changed to be more specific to the imxrt series as we know better what the splits are.

I'm all for the mono-repo with workspace as well. I think it helps really keep things in sync, especially since I believe we will continue to create common drivers for all these chips, with different instances per chip. I still want to try to maximize code sharing.

@teburd
Copy link
Member Author

teburd commented Jun 30, 2020

I think a first step is making the current imxrt-hal crate the imxrt106x-hal crate with feature flag gates for the mentioned components when/if they get added. The second step would be creating a imxrt101x-hal (my task) and moving various common macros/drivers/etc to a common imxrt-hal which they both would depend on.

@teburd
Copy link
Member Author

teburd commented Jul 13, 2020

Seeing how the new new is to have drivers in crates ala imxrt-iomuxc, I'm thinking it makes sense to have a driver per crate, which inevitably all depend on the associated Pin types provided by iomuxc. I really like the new direction @mciantyre has pushed this!

@teburd
Copy link
Member Author

teburd commented Oct 6, 2020

I started this since I finally got probe-rs working with my imxrt101x board, $10 for 500MHz is hard to beat! They have since raised the price to $40, still not too terrible considering it has a builtin debug probe

@teburd teburd closed this as completed Oct 7, 2020
@teburd teburd reopened this Oct 7, 2020
@teburd teburd mentioned this issue Oct 10, 2020
@teburd
Copy link
Member Author

teburd commented Oct 12, 2020

So my first goal is to have a imxrt101x-hal and imxrt106x-hal, where the only things I care about for imxt101x-hal are iomuxc, ccm, and lpuart working.

I'm experimenting with a few ideas, one of which is to stop the imxrt-ral split between chips such that...

  • Common Instance/RegisterBlock types are generated for all chips.
  • Only the unique per chip Instance/RegisterBlock types are per chip feature flagged in.
  • This lets us build drivers against the common Instance/RegisterBlock types.
  • The drivers do not need any conditional compilation to be built.

Some interesting side notes in attempting this so far...

  • A single bitflag difference in LPUART, RIDMA, exists for some but not all chips in the ixmrt line, with no real rhyme or reason.
    • Is this an SVD issue?
    • Something we can deal with by using a feature flag seeing as its additive?
    • Is this sort of additive bitflag/field common in other peripheral Instance definitions causing _v2 when I remove the family
      split in imxrtral.py?

@teburd teburd closed this as completed Oct 12, 2020
@teburd teburd reopened this Oct 12, 2020
@mciantyre
Copy link
Member

Sounds reasonable! With goals for common register blocks and optional feature flags, it sounds like we're moving away from a "split HAL," and more towards a "unified RAL." Is that the thinking?

Not requiring feature-flags would be a boon for library development. Then, when users are ready to compile their binaries for their system, they can enable a chip-specific feature without too much effort.

A single bitflag difference in LPUART, RIDMA, exists for some but not all chips in the ixmrt line, with no real rhyme or reason.

Looks like it's missing from the 1021 SVD. I confirmed that the RIDMAE bit exists in the LPUART BAUD register, offset 20, in each 1010, 1015, 1020, 1050, 1060, and 1064 reference manual.

@teburd
Copy link
Member Author

teburd commented Oct 13, 2020

I'm still thinking the end hal is a split, but the end hal effectively needs to do only a few things, define the number and kind of peripherals into a struct Peripherals and possibly deal with some of the clocking diffences. We'll see though!

@teburd
Copy link
Member Author

teburd commented Oct 16, 2020

Reason for the Split

  • Git repo becomes git clone && cargo build for (imxrt-rs) unless you need imxrt-ral updates, this would build all chip hals and all feature flag variants actually used by chip hals by default
  • No conditional compiling needed in the chip crate
  • Some conditional compiling needed in imxrt-hal-common, specified by the chip crate on behalf of the end crate user
  • Some chips vary enough to warrant it, when unifying the imxrt-ral to one family there are some significant differences between peripherals that need to be accounted for somehow
  • Clearer generated docs, one doc.rs crate page per chip

TODO

  • Fixup workspace to match https://github.com/nrf-rs/nrf-hal layout, with HAL per chip and a common hal (Removes conditional feature compilation for HAL #89)
  • Create a common clock and power management library in imxrt-clock matching imxrt-iomuxc, usable on 1010 and 1060 series chips and used by imxrt1060-hal
  • Create a common LPUART library in imxrt-lpuart (or imxrt-hal-common if it makes more sense since its going to be smallish).
  • Move most code from imxrt1060-hal to imxrt-hal-common or perhaps peripheral driver crates (see imxrt-iomuxc) when sensible.
  • Create a imxrt1010-hal (supports imxrt 1011)

Questions and Answers

Q: Should we change imxrt-ral so there's only one device family, but multiple RegisterBlock types when differing devices exist?
A: Up in the air, this means you still need a feature flag to get the actual peripheral instances for a specific chip, but the type definitions and the memory layout of a RegisterBlock is reused throughout. If we remove the pub use portions pushing up the chip specific Instances, the feature flags become entirely additive. That's perhaps feasible with a split hal. It's also clearer the number of variants each peripheral has, for example the clock configuration module there were 3 when I did this locally. I think this makes it easier to then write a driver against, as its clear there are 3 variants and what the differences are.

Q: How should we shared doc comments to avoid copy/paste insanity?
A: It's possible that we document each Peripheral API with doc comments and then simply refer to those examples (with links hopefully) when describing how to use a particular device's lpuart, lpspi, or any other peripheral. Some devices have specific peripheral functionality so no matter what we will need device specific doc comments somewhere.

Q: How do we write a common crate used against a variety of hal's?
A: I would recommend we select the correct hal with a feature flag for each variant supported. So for imxrt-uart-log we'd have a feature flag imxrt1062 which selects the imxrt1062-hal, and then does #[cfg(feature=imxrt1062)] pub use imxrt1062_hal::uart as uart for example. Also possible to write against imxrt-hal-common but I'd like to keep imxrt-hal-common an implementation detail, its our way of sharing code among the imxrtXXXX-hal crates is how I see it.

@mciantyre
Copy link
Member

mciantyre commented Oct 16, 2020

One of the previously stated values of a spit HAL was to support documentation, so I'd like to get your thoughts on how that will work.

In #89, we're changing documentation examples from imxrt_hal to imxrt106x_hal. As we consolidate drivers into something like imxrt_hal_common, we'll re-export those drivers from the imxrt106x_hal and other chip-specific HALs. I see documentation working a few different ways:

  1. When we move the driver code into the common HAL, we also move the driver documentation. Those imxrt106x_hal namespaces become something like imxrt_hal_common. Since the chip-specific HAL's re-export the driver as-is, the end-user sees namespaces with imxrt_hal_common in the imxrt106x_hal documentation. This is inconsistent for the end-user, but can be taught by a top-level statement resembling "assume that all documentation implicitly includes use imxrt106x_hal as imxrt_hal_common."
  2. When we move the driver code into the common HAL, we do not move the driver documentation. Instead, we only document the re-exported driver in the chip-specific HAL. This requires us to apply documentation examples across all chip-specific HALs. However, the documentation for the imxrt106x_hal is consistent with its name. We're OK with having little documentation on the common HAL.

Are there other approaches we could take that don't have these trade-offs? It doesn't look like the nrf-hal has an approach for documentation examples, as the crates do not yet have documentation examples.


Suppose I'm a user who wants to design a higher-level crate that works across multiple i.MX RT chips. The imxrt-uart-log crate falls into this category (let's pretend that it's maintained outside of the imxrt-rs project). What will be our policy for supporting users who want to design these libraries?

  1. We let users consume the imxrt_hal_common crate, so that their API works easily for all chip-specific HALs. This is similar to where we are today. To support this, we need to keep maintaining the feature-flag documentation. This goes against the nrf-hal approach, which says "Don't use this directly.".
  2. We adopt the same approach as nrf-hal: the imxrt_hal_common is an implementation detail, and we're free to break users to satisfy our chip-specific HALs. As an imxrt-uart-log developer, I need to either mimic the structure of the public HALs, and implement crates lik imxrt106x-uart-log, imxrt101x-uart-log, etc; or, I need to adopt feature flags that selectively choose a chip-specific HAL.

Is there another way to support external users who want to create libraries for multiple chips?


Should we change imxrt-ral so there's only one device family, but multiple RegisterBlock types when differening devices exist?

Concretely, this would mean imxrt-ral has a feature called "imxrt106x" which consolidates the two "imxrt1061"1 and "imxrt1062"` features? If so, I'd say no, we don't need to have multiple register blocks. In some previous studies, it looked like the 1061 and 1062 chips were so similar they wouldn't warrant different register blocks. If we need to support multiple register blocks, they should be behind a different RAL feature.

Semi-related: we may want to change our chip naming schema to more closely match the reference manuals and product identifiers. I proposed the "x" suffix back when we started discussing split HAL, and then I used it in the imxrt-iomuxc crate. I'm concerned that was the wrong call, since the "x" suffix doesn't distinguish common support for the 1010 and 1015 families, which seem to be pretty different.

@mciantyre mciantyre pinned this issue Oct 16, 2020
@teburd
Copy link
Member Author

teburd commented Oct 16, 2020

After better understanding the differences, I'm with you on ditching "x" suffix, as each chip really is different enough to warrant its own feature flag I believe.

So I'm looking to the nrf-rs project and their nrf-hal repo in particular as the model to follow

I've updated the Q/A section with answers I think help resolve most of the questions, if I missed something let me know

@mciantyre
Copy link
Member

mciantyre commented Oct 17, 2020

[...] simply refer to those examples (with links hopefully) [...]

Links back to examples works for me. We lose the benefit of documentation tests that make sure our examples are relevant. It's on us to make sure these links don't point to stale or unreleased examples; a crate release from 5 years ago will need to keep pointing to 5 year old examples.

I would recommend we select the correct hal with a feature flag for each variant supported.

We've cited reasons why required, narrowly-scoped feature flags aren't the best. This sounds like we're putting our current problem on users who want to do the same thing we're doing: create drivers that work across chips. If we're all trying to build drivers that work across these systems, shouldn't we help the community figure this out, without having them adopt an approach that we tried and didn't like?

@teburd
Copy link
Member Author

teburd commented Oct 18, 2020

If we expose the common crate as a published crate, does that solve most of the issues?

imxrt-uart-log would depend on imxrt-hal-common, and a feature flag would need to be set so the types match those in whatever chip hal your using.

Is that workable?

@mciantyre
Copy link
Member

I think it would be workable.

Sorry, I might be confused about the goals of the common HAL. When I see a reason for splitting up our crates, like

Some conditional compiling needed in imxrt-hal-common, specified by the chip crate on behalf of the end crate user

I'm interpreting that as "we only need conditional compiles when there's a drastic difference between chips." For UART and DMA HAL drivers -- the peripherals needed by imxrt-uart-log -- I'm not expecting to need feature flags in the common HAL.

and a feature flag would need to be set so the types match those in whatever chip hal your using.

Can you clarify why I need feature flags when designing against the common HAL, particularly when consuming a UART and DMA driver API?

@teburd
Copy link
Member Author

teburd commented Oct 19, 2020

Unless you are only depending on trait types that are always available from the HAL without a feature flag involved, the concrete types depend on RAL types which vary over the chips.

@mciantyre
Copy link
Member

mciantyre commented Oct 20, 2020

the concrete types depend on RAL types which vary over the chips.

It sounded like a unified RAL could solve that.

I want to make sure we're on the same page with what a unified RAL, and the common HAL, might look like. I linked to my experiments below.

https://github.com/mciantyre/proto-hal

Could you let me know if this meets your thoughts on how this could work? The experiment seems to show that, with some restructuring to our dependencies, we should be able to create general crates that use the common HAL without required feature selection.

@teburd
Copy link
Member Author

teburd commented Oct 21, 2020

A unified ral can solve most of the problem I agree, in looking at the proto-hal you have, that is indeed what I was hoping to accomplish!

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