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

Fix CI: source of PACs matrix + clippy satisfaction #71

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

martinmortsell
Copy link
Contributor

The CI was broken due to the pac folder being removed.
This fix extracts the pac features from hal/Cargo.toml instead.

@michalfita michalfita added bug Something isn't working build system Related to build system or CI support labels Dec 18, 2023
@martinmortsell
Copy link
Contributor Author

That required a good number of changes to satisfy the doctests and clippy. I suspect the bump in version is to blame.
I blindly listened to clippy, though the changes seemed to make sense. For the doc tests essentially every test got the same error message:

error[E0599]: `Peripherals` is not an iterator
  --> hal/src/tc/mod.rs:25:34
   |
11 | let pac = hal::pac::Peripherals::take().unwrap();
   |                                  ^^^^ `Peripherals` is not an iterator
   |
  ::: /mnt/c/Users/martin.mortsell/Documents/atsams70-rust/target/x86_64-unknown-linux-gnu/debug/build/atsamv71q21b-90d0014a7c81a20a/out/lib.rs:1:55241
   |
1  | ...All the peripherals."] # [allow (non_snake_case)] pub struct Peripherals { # [doc = "ACC"] pub ACC : ACC , # [doc = "AES"] pub AES : AES , # [doc = "AFEC0"] pub AF...
   |                                                      ---------------------- doesn't satisfy `Peripherals: Iterator`
   |
   = note: the following trait bounds were not satisfied:
           `Peripherals: Iterator`
           which is required by `&mut Peripherals: Iterator`

error: aborting due to previous error

I don't understand what the issue is, but solved it by changing all occurrences of Peripherals::take() to Peripherals::steal(). This change is not visible in the docs as those lines are hidden anyway.

@michalfita michalfita changed the title Fix CI Fix CI: source of PACs matrix + clippy satisfaction Dec 18, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@@ -28,7 +28,7 @@ For example, if we want to configure the [`MainClock`]:
use atsamx7x_hal as hal;
use hal::fugit::RateExtU32;

let pac = hal::pac::Peripherals::take().unwrap();
let pac = unsafe{hal::pac::Peripherals::steal()};
Copy link
Collaborator

@michalfita michalfita Dec 18, 2023

Choose a reason for hiding this comment

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

It's unsafe again??? 🤔

Looks like it depends on critical-section implementation: https://docs.rs/svd2rust/latest/svd2rust/#peripheral-api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

steal() is unsafe as it returns a peripheral struct without checking if the peripherals have been generated before, potentially violating the singleton assumption.

It seems take() is a method implemented for iterators which confuses the doc test since Peripherals is not an iterator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, I believe you are correct that critical-section is the root cause here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking into this a bit more and rolling back to an older version of the pac makes this error go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems Peripherals does not have the take() method anymore. Very unclear, investigating further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is interesting. I should have some time tomorrow to branch and try next version of svd2rust to see if it's intermittent problem. There's 0.30.1 and two other point releases to try with.

I'm not going straight to last version as there are breaking changes and invocation changes + I want something to work on release automation (I wish I have like two weeks full time to work on that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I will keep investigating in the meantime.
I can't seem to build the pacs locally. python3 tools/pacs.py svd should work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rust-embedded/svd2rust#704 Found the relevant issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't catch this in our examples as they all utse RTIC and RTIC does a steal() internally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

tools/pacs.py requires mapping JSON file as argument... I forgot to commit it - harvester generates it when downloads atpacks and extract SVDs.
For now you can run harvester yourself to redownload and obtain the file. It's used to fill up ATPACKs version numbers in README.mds of each PAC.

Co-authored-by: Michał Fita <[email protected]>
@michalfita
Copy link
Collaborator

@martinmortsell I can't commit to PRs raised from GrepitAB's forks. I'm merging this as is and I'm going to work on more fixes for #72 on another branch.

@michalfita michalfita merged commit a496153 into atsams-rs:development Dec 22, 2023
77 checks passed
@michalfita michalfita added this to the HAL Release 0.5.0 milestone Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build system Related to build system or CI support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants