Refactor Cargo features #656

bradleyharden · 2022-12-31T03:22:28Z

Summary

Refactor the Cargo features to provide a better, more fine-grained structure.
Separate the features for selecting pins from the features for selecting peripherals.
Reorganize and better document the Cargo.toml file.

NOTE: Reviewers will probably want to read through the entirety of Cargo.toml, but the rest of the changes will probably be overwhelming to review. Everything builds, but none of this is tested on hardware, since I don't have any hardware to work with at the moment.

Checklist

~~CHANGELOG.md for the BSP or HAL updated~~
All new or modified code is well documented, especially public items
No new warnings or clippy suggestions have been introduced (see CI or check locally)

sajattack · 2022-12-31T04:43:15Z

hal/Cargo.toml

 description = "HAL and Peripheral access API for ATSAMD11, ATSAMD21, ATSAMD51, ATSAME51, ATSAME53 and ATSAME54 microcontrollers"
+documentation = "https://docs.rs/crate/atsamd-hal/"


should this point to github.io instead?

My first instinct is no, because the github.io docs are built from master, which is not on crates.io. But as you said yesterday, we should find a way to make those docs more visible.

I guess the bigger question is which set of docs should new users encounter first? A version tied to a specific crates.io release? Or a version built directly from master?

hal/Cargo.toml

sajattack · 2022-12-31T04:48:10Z

hal/src/lib.rs

-))]
-compile_error!("The 'usb' feature is enabled, but not a chip with USB support");
+#[cfg(all(feature = "usb", feature = "device", not(feature = "usbp")))]
+compile_error!("The 'usb' feature is enabled, but this chip does not support USB");


we should have one of these for sdmmc and other stuff too maybe?

Yes, absolutely. I'll see what other instances make sense.

bradleyharden · 2022-12-31T16:39:24Z

@sajattack had a suggestion on Matrix:

maybe some of the gpio features could be called min-pb00 instead of just pb00
just thought of that, since I was a bit confused until I read the comment

I responded, but I realized that discussion should really stay in the PR, so I'm copying my response here.

@sajattack, here was my thought process for feature naming:

I wanted to create feature "namespaces" using prefixes. That's how I came up with pins- and periph-.
However, using a periph- prefix for every single peripheral is extremely verbose, e.g. periph-sercom0, periph-tcc0, etc..
For peripherals with no possible confusion with a user feature, I just named it the most natural thing, e.g. sercom0, tcc0, etc.
For peripherals with a possible name collision, I appended a p to the user-selectable version, e.g. usb and usbp.
I originally had the pb00 feature within the pins- namespace as pins-pb00. But I thought a specific pin is akin to a specific peripheral, which does not have the namespace prefix, so I removed it.

Questions for you:

Do you have any broad feedback on this scheme?
What was the min- prefix supposed to convey in your suggestion?
Do you think I should change back to pins-pb00? Or use something else, like min-pb00?
If I change pb00, should I also change the peripheral features?

bradleyharden · 2022-12-31T17:13:43Z

I just had a philosophical realization. You say:

I was a bit confused until I read the comment

Several times recently, both at work and outside of work, people have effectively told me:

This wasn't intuitive to me, and it didn't make sense until I read the documentation.

While I agree that we should try to make things as intuitive as possible, sometimes things can't be made intuitive, either because the motivation is too complicated & subtle, or because people's intuitions don't match. At some point, I think the solution has to be "Read the documentation". But exactly where that happens is debatable.

Where do you think this situation falls? Would adding min- make things more intuitive to everyone? Or is it only intuitive if you're already familiar with our prior min-samxxx features?

sajattack · 2022-12-31T18:08:02Z

Questions for you:

* Do you have any broad feedback on this scheme?

Not really, it seems reasonable enough

* What was the min- prefix supposed to convey in your suggestion?

Similar to min-samd51g, it conveys that it includes pb00, and everything up to it

* Do you think I should change back to pins-pb00? Or use something else, like min-pb00?

Honestly I'm not too fussed about it, it was just an idea that came to me

* If I change pb00, should I also change the peripheral features?

The -p ones are a little unintuitive as well

sajattack · 2022-12-31T19:27:58Z

I just had a philosophical realization. You say:

I was a bit confused until I read the comment

Several times recently, both at work and outside of work, people have effectively told me:

This wasn't intuitive to me, and it didn't make sense until I read the documentation.

While I agree that we should try to make things as intuitive as possible, sometimes things can't be made intuitive, either because the motivation is too complicated & subtle, or because people's intuitions don't match. At some point, I think the solution has to be "Read the documentation". But exactly where that happens is debatable.

Part of the problem in this instance, is that, in doing a full code review, I see pb00 in multiple places, but the documentation is only in one of them. That's where an intuitive name has benefits over documentation.

bradleyharden · 2022-12-31T22:26:17Z

The -p ones are a little unintuitive as well

Yeah, I'm not a huge fan of it either, but I didn't come up with anything better at the time.

Similar to min-samd51g, it conveys that it includes pb00, and everything up to it

My hesitance comes from the fact that this statement isn't quite accurate. Pins aren't strictly additive. The best example is pin PA28, which I had to make special accommodations for. PA28 is included in the 48 and 64 pin packages for D21 chips, but it's not present in the same packages for D51 chips.

While thinking about your responses, I had an idea. What do you think of using a has- prefix for all individual pin & peripheral features, e.g. has-sercom0, has-tcc0, has-pb00, etc? It's more verbose, but I think it communicates exactly what is meant by the feature. Then, the pins- and periph- namespaces would exist to group has- features into reusable sets.

sajattack · 2022-12-31T23:04:52Z

The -p ones are a little unintuitive as well

Yeah, I'm not a huge fan of it either, but I didn't come up with anything better at the time.

Similar to min-samd51g, it conveys that it includes pb00, and everything up to it

My hesitance comes from the fact that this statement isn't quite accurate. Pins aren't strictly additive. The best example is pin PA28, which I had to make special accommodations for. PA28 is included in the 48 and 64 pin packages for D21 chips, but it's not present in the same packages for D51 chips.

While thinking about your responses, I had an idea. What do you think of using a has- prefix for all individual pin & peripheral features, e.g. has-sercom0, has-tcc0, has-pb00, etc? It's more verbose, but I think it communicates exactly what is meant by the feature. Then, the pins- and periph- namespaces would exist to group has- features into reusable sets.

Sounds like a good idea.

has- eliminates both problems in my mind, so I'm not sure why your last sentence mentions pins- and periph-

bradleyharden · 2023-01-01T21:34:27Z

has- eliminates both problems in my mind, so I'm not sure why your last sentence mentions pins- and periph-

The pins- and periph- prefixes would still exist to group together other features. The periph- features would only exist in the Cargo.toml file, so there shouldn't be any confusion there. But the pins- features would be in the code as well (as they are right now). Since you didn't comment on the pins- features, I take it they were self-explanatory enough.

I'll rename the features and push an update.

hal/Cargo.toml

Refactor the Cargo features to provide a better, more fine-grained structure. Separate the features for selecting pins from the features for selecting peripherals. Reorganize and better document the Cargo.toml file.

Add a `has-` prefix to the features for optional pins and peripherals. Remove the `p` suffix to avoid naming collisions, as it is no longer necessary.

The previous update scripts assumed a particular structure of the TOML files. This approach is extremely brittle in the face of formatting or comments. Instead, use the Python package `tomlkit` to parse, modify and dump TOML files while still preserving all formatting and commenting.

bradleyharden · 2023-01-14T16:21:06Z

@sajattack, the scripts I modified were definitely very brittle. They used something that was TOML-aware for reading fields, but they still modified the contents of the file by hand. I'm not quite sure why. My guess is that the Python toml library doesn't preserve style and comments, so a round-trip read & write would not yield the same file. The new TOML library I used is fully capable of a round-trip without modifications.

I guess we'll just see what happens. If we run into errors when trying to release, we can fix things then.

bradleyharden force-pushed the refactor_features branch 3 times, most recently from 0beb2ea to d8e8c1f Compare December 31, 2022 04:09

sajattack reviewed Dec 31, 2022

View reviewed changes

hal/Cargo.toml Outdated Show resolved Hide resolved

sajattack reviewed Dec 31, 2022

View reviewed changes

bradleyharden force-pushed the refactor_features branch 2 times, most recently from c999aa4 to 1b759d5 Compare January 1, 2023 22:23

sajattack approved these changes Jan 7, 2023

View reviewed changes

hal/Cargo.toml Outdated Show resolved Hide resolved

hal/Cargo.toml Outdated Show resolved Hide resolved

bradleyharden added 3 commits January 14, 2023 11:16

Refactor Cargo features

fcd6ded

Refactor the Cargo features to provide a better, more fine-grained structure. Separate the features for selecting pins from the features for selecting peripherals. Reorganize and better document the Cargo.toml file.

Improve naming of Cargo features

37516e1

Add a `has-` prefix to the features for optional pins and peripherals. Remove the `p` suffix to avoid naming collisions, as it is no longer necessary.

bradleyharden force-pushed the refactor_features branch from 6e94fa0 to d546024 Compare January 14, 2023 16:16

bradleyharden merged commit 61ffa08 into atsamd-rs:master Jan 14, 2023

bradleyharden mentioned this pull request Jan 15, 2023

Add support for SAMC2x ? #659

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Cargo features #656

Refactor Cargo features #656

bradleyharden commented Dec 31, 2022 •

edited

Loading

sajattack Dec 31, 2022

bradleyharden Dec 31, 2022

sajattack Dec 31, 2022

bradleyharden Dec 31, 2022

bradleyharden commented Dec 31, 2022

bradleyharden commented Dec 31, 2022

sajattack commented Dec 31, 2022

sajattack commented Dec 31, 2022

bradleyharden commented Dec 31, 2022 •

edited

Loading

sajattack commented Dec 31, 2022 •

edited

Loading

bradleyharden commented Jan 1, 2023

bradleyharden commented Jan 14, 2023

		description = "HAL and Peripheral access API for ATSAMD11, ATSAMD21, ATSAMD51, ATSAME51, ATSAME53 and ATSAME54 microcontrollers"
		documentation = "https://docs.rs/crate/atsamd-hal/"

Refactor Cargo features #656

Refactor Cargo features #656

Conversation

bradleyharden commented Dec 31, 2022 • edited Loading

Summary

Checklist

sajattack Dec 31, 2022

Choose a reason for hiding this comment

bradleyharden Dec 31, 2022

Choose a reason for hiding this comment

sajattack Dec 31, 2022

Choose a reason for hiding this comment

bradleyharden Dec 31, 2022

Choose a reason for hiding this comment

bradleyharden commented Dec 31, 2022

bradleyharden commented Dec 31, 2022

sajattack commented Dec 31, 2022

sajattack commented Dec 31, 2022

bradleyharden commented Dec 31, 2022 • edited Loading

sajattack commented Dec 31, 2022 • edited Loading

bradleyharden commented Jan 1, 2023

bradleyharden commented Jan 14, 2023

bradleyharden commented Dec 31, 2022 •

edited

Loading

bradleyharden commented Dec 31, 2022 •

edited

Loading

sajattack commented Dec 31, 2022 •

edited

Loading