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

Re-organize using a proc-macro to support more devices #728

Merged

Conversation

TethysSvensson
Copy link
Contributor

@TethysSvensson TethysSvensson commented Mar 5, 2024

Summary

This is a continuation an continuation of the work I did in #716 and #719. It uses the same basic approach as #719, but I changed the macros in response to the feedback I got on that PR.

Since this is quite a large PR, I have split it up by commits, to make it easier to review.

A quick overview of what this PR contains:

  • It adds a new proc-macro crate, which replaces all of the many-many features in the Cargo.toml
  • The primary macro of that crate is #[hal_cfg()], but it also contains a helper macro to allow using #[hal_cfg()] in more places, along with two specialized macros for creating conditional docstrings and conditional modules.
  • A lot of doc-comments are currently conditional depending on feature flags. This PR removes most of these, as they cannot easily be supported by the new proc-macro.
  • A lot of functionality were split up first by architecture in the thumbv6m and thumbv7em directories, and then by peripheral. This PR moves these files, so they are organized first by peripheral and then by architecture.
  • The code for reset_cause, serial_number and watchdog were sufficiently similar that I decided to merge the implementations.
  • It does not create a workspace, since I got weird crate unification errors when I did.

This PR also includes a change in sercom/pad/impl_pad_thumbv6m.rs should arguably be split into a separate PR. The is that while converting the pad_table, I found a mistake in the table and did not feel like carrying that mistake over.

However I wanted to review the rest of the table for mistakes as well, which I found almost impossible to do unless I split up the tables by datasheet.

If you want me to split out this change in a separate PR, please let me know whether you prefer me to carry over the bug in the conversion, or if you prefer to fix the bug before merging this PR.

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 - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

@TethysSvensson TethysSvensson force-pushed the tethys/proc-macros-reorganization2 branch from fe9a52d to 647ec50 Compare March 5, 2024 16:10
@TethysSvensson TethysSvensson force-pushed the tethys/proc-macros-reorganization2 branch 2 times, most recently from 3a75af0 to 4bd0788 Compare March 5, 2024 16:16
@TethysSvensson TethysSvensson force-pushed the tethys/proc-macros-reorganization2 branch 4 times, most recently from 4d12502 to 6975478 Compare March 5, 2024 17:39
@TethysSvensson TethysSvensson force-pushed the tethys/proc-macros-reorganization2 branch from 6975478 to d1e134d Compare March 5, 2024 18:24
Copy link
Contributor

@jbeaurivage jbeaurivage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR @TethysSvensson. After reading through all your changes, I can't seem to find anything I disagree with. I'd just be interested in knowing what the bugs were in the SERCOM pads tables?

hal/src/dmac/channel/mod.rs Show resolved Hide resolved
hal/src/sercom/i2c/flags.rs Show resolved Hide resolved
@TethysSvensson
Copy link
Contributor Author

I'd just be interested in knowing what the bugs were in the SERCOM pads tables?

I don't remember exactly how I first noticed it. I believe that the samd11 tables might have been different enough that I somehow spottet it?

In any case, the new tables are definitely different from the old ones, and I did compare them very carefully from the manual at some point, so I believe the new ones are right.

It was a while ago that I verified, so perhaps I should take another look to see if any mistakes snuck back in 🤔

@TethysSvensson
Copy link
Contributor Author

After re-checking, I didn't find a bug, as the new tables were mostly equivalent to the old ones. The only changes I found where these added pads to d11:

PA06
PA07
PA10
PA11
PA14/D
PA15/D
PA16
PA17
PA22
PA23
PA24/D
PA25/D
PA30/D
PA31/D

I think what I was as a "bug" was something like the PA14/D, where the pads are different between d11 and d21, but I misread the #[cfg()] to assume that it also applied to the d11.

If you want me to re-combine the tables I can do that.

@jbeaurivage
Copy link
Contributor

Nope it sounds good to me, I kinda like splitting the tables, it makes them more straightforward to understand. Anything else you want to review/discuss before we merge this?

@TethysSvensson
Copy link
Contributor Author

I'm good with merging now. As I said I haven't done any automated backwards compatible testing.

I don't know when I'll have time to do so either, so if you want that done before merging I'd love some help.

@jbeaurivage
Copy link
Contributor

Well, we rarely release patch updates anyway, so the next release will likely be a minor anyways, with no expectation of backwards compatibility. LGTM, thanks for the hard work!

@jbeaurivage jbeaurivage merged commit ecf2f58 into atsamd-rs:master Mar 8, 2024
107 checks passed
@TethysSvensson TethysSvensson deleted the tethys/proc-macros-reorganization2 branch March 9, 2024 12:00
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

Successfully merging this pull request may close these issues.

2 participants