[draft] Re-organizing using a proc-macro to support more devices

[TethysSvensson]

Summary

This is a different variant of #716 with the same goals. Instead of introducing more cargo features, this one uses a proc-macro. I have ported the same two modules as in #716. If this way of organization is accepted, more modules could be ported in follow-up PRs.

This was partially inspired by the proc-macro branch made by @jbeaurivage, but works slightly differently.

A few implementation details of note:

• I decided not to use cargo features in the proc-macro crate. This was both to prevent recompilation, and to simplify its inclusion in the hal.
• I used a build.rs file inside proc-macro to prevent having to read config files for each invocation of the macro.
• I made the hal_module_mapping!() macro to get around the problem of proc-macro-attributes not usable in as many locations as the #[cfg()] macro.
• I would probably also make a hal_code_mapping!() macro that works in the same way as the hal_module_mapping!(), but it was not needed for this example.

[jbeaurivage]

@TethysSvensson, I took the time to review more in depth, I really like the approach you took here. Some more detailed comments, most of which are more or less suggestions:

• I think a hal_code_mapping!() macro will absolutely be necessary. There are a few places where we use #[cfg]s inside functions.

• Now that we have two top-level crates (ie, not BSP or PACs), I would suggest creating a cargo workspace for them to avoid target dir duplication

• If you think it would simplify some things in devices.yaml, would it make sense to support a except key alongside only in peripheral declarations?

• This is up for discussion, but when declaring variants in hal_module_mapping!() or #[hal_cfgs], the current way of declaring multiple variants is as following: "adc-d11 adc-d21". To me, a more obvious (idiomatic?) way declaring them could be:

  atsamd_hal_macros::hal_module_mapping!(
  pub adc,
  ["adc-d11", "adc-d21"] => "adc/d11.rs",
  ["adc-d5x"] => "adc/d5x.rs",
  );

and

#[hal_cfgs("serial-numbers-d11", "serial-numbers-d21")]

• Pros:
  • Easier to distinguish between variants when reading cod
  • Cons:
  • A bit of added complexity in the parsing code
  • A bit more typing when writing

My rationale is that we'll usually write these macro declarations only once, but many people will likely read them often. So it makes sense to make the code more readable at the expense of a few extra keystrokes. Thoughts?

[TethysSvensson]

• I think a hal_code_mapping!() macro will absolutely be necessary. There are a few places where we use #[cfg]s inside functions.

I agree. After porting most of the crate to use the proc-macros, I found it that even this macro was not enough. I also had to create a #[hal_macro_helper] to allow using #[hal_cfgs()] and #[hal_cfgs_all()] in places where proc-macro attributes are not normally allowed.

• Now that we have two top-level crates (ie, not BSP or PACs), I would suggest creating a cargo workspace for them to avoid target dir duplication

Sounds good, I will do this next time I work on this. :+1:

• If you think it would simplify some things in devices.yaml, would it make sense to support a except key alongside only in peripheral declarations?

I think there are a few cases where this would make the code nicer, such as for samd51g being the only device in the d5x family that does not have I²S.

• This is up for discussion, but when declaring variants in hal_module_mapping!() or #[hal_cfgs], the current way of declaring multiple variants is as following: "adc-d11 adc-d21". To me, a more obvious (idiomatic?) way declaring them could be:

atsamd_hal_macros::hal_module_mapping!(
    pub adc,
    ["adc-d11", "adc-d21"] => "adc/d11.rs",
    ["adc-d5x"] => "adc/d5x.rs",
);

and

#[hal_cfgs("serial-numbers-d11", "serial-numbers-d21")]

• Pros:

  • Easier to distinguish between variants when reading cod

• Cons:

  • A bit of added complexity in the parsing code
  • A bit more typing when writing

My rationale is that we'll usually write these macro declarations only once, but many people will likely read them often. So it makes sense to make the code more readable at the expense of a few extra keystrokes. Thoughts?

I agree completely.

I originally did it like that, because I wanted to keep the proc-macro as small and simple as possible. However, after porting most of the crate, I realized that I needed more functionality in the proc-macro anyways, but I never re-examined the decision to keep it in a single string. I will definitely change this, the next time I work on this.

[jbeaurivage]

After attempting to diff the before/after of introducing the macros, I found the following potential bug:

#[hal_cfgs("pin-group-b")]
    B,

expands to

#[cfg(any(
    feature = "samd21el",
    feature = "samd21g",
    feature = "samd21gl",
    feature = "samd21j",
    feature = "samd51g",
    feature = "samd51j",
    feature = "samd51n",
    feature = "samd51p",
    feature = "same51g",
    feature = "same51j",
    feature = "same51n",
    feature = "same53j",
    feature = "same53n",
    feature = "same54n",
    feature = "same54p"
))]
B,

Unless I misunderstand how pin groups work, shouldn't all chips have a pin group B? As it is now, pin group B starts at SAMD21EL.

[TethysSvensson]

Hi and sorry for the lack of communication. I am still planning to clean this up a bit and turn it into a series of PRs (or alternatively one big PR if you prefer that). I've been quite busy the last few weeks with a new job, and I expect it to be at least another week or two until things calm down enough for me to take another look.

If you're itching to get this merged to the main branch, feel free to take a look at my reorganization branch. If I remember correct, I have ported all of the code except for conditional docstrings to use macros instead of feature flags.

[TethysSvensson]

And to answer your question: The samd11c, samd11d and samd21e only have PAxx pins, though the samd21el also has PBxx pins.

[jbeaurivage]

Duh, I should've double checked the datasheet before talking too fast! Nothing to see here, carry on 😁

I've been thinking about how you added a hal_cfg_all macro. Should we consider renaming hal_cfg to hal_cfg_any for consistency's sake? Also, even though the macros are for internal use, they should probably be documented for the benefit of other maintainers.

[TethysSvensson]

I could also combine them, so you would have to do #[hal_cfg(any("a", "b"))] or #[hal_cfg(all("a", "b"))] if you wanted to deal with more than one device.

This would also allow you to do nested expressions if that ever turns out to be useful.

[jbeaurivage]

Superseded by #728.