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

arm: Use AP specific types & enumerate Addresses rather than APs #2683

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

ithinuel
Copy link
Contributor

@ithinuel ithinuel commented Jul 15, 2024

  • Enumerate Memory AP addresses instead of giving the number of valid
    (Generic and Memory) APs.
    This will enable future development where APv2’s live in disjoint memory
    spaces.
  • Use AP specific types
    This should allow future developments to enabled things like secure
    debug, memory tagging, etc
  • Drop ApInformation and MemoryApInformation
    The AP specific types carry these information internally so these types
    are no longer required.

Follows:

@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from 7961632 to 693b4b0 Compare July 16, 2024 21:32
Copy link

The following changelog fragments do not match the expected pattern:

  • changelog/draft.md

Files should start with one of the categories followed by a dash, and end with '.md'
For example: 'added-foo-bar.md'

Valid categories are:

  • added
  • changed
  • fixed
  • removed

@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch 5 times, most recently from 4571ff7 to e829bff Compare July 21, 2024 21:37
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch 7 times, most recently from c7b6def to c33729a Compare July 23, 2024 21:07
@ithinuel ithinuel marked this pull request as ready for review July 23, 2024 21:07
@ithinuel
Copy link
Contributor Author

@bugadani I’m sorry the last commit is bigger, I didn’t really now how to detangle the various parts.

probe-rs/Cargo.toml Outdated Show resolved Hide resolved
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from c33729a to dcee752 Compare July 24, 2024 06:21
@ithinuel
Copy link
Contributor Author

There are a few things that I’m not fully satisfied in this PR but I had to stop somewhere :P
Just to list a few:

  • The methods remaining in ArmMemoryInterface.
    • The way we have to return Result<&mut _, _> etc. Something feels off about it.
    • The newly introduced base_address.
  • The mocks that seem to implement the same trait in slightly different ways (based on the tests needs).
    This adds lots of noise and require changes anytime there’s an unrelated change in the trait.
    It’d be easier with 1 well features mock for the other tests to use.

There are more refactor coming with the introduction of APv2 & ADIv6 (mostly moving some modules around to make place for an "apv2" module.

@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from dcee752 to 2a5d786 Compare July 24, 2024 18:26
probe-rs/src/memory/mod.rs Outdated Show resolved Hide resolved
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 00b02b0 to 74233de Compare July 24, 2024 23:10
@bugadani
Copy link
Contributor

NGL this is very hard to review. I don't have experience with the code, and even though I assume most of this is just pushing stuff around, I can't track what goes where. The size of the whole diff makes my computer cry, and the online vscode is just not optimal to read diffs either :)

Because we squash PRs into single commits, I would recommend to split out the independent parts into separate PRs. That would make them easier to review and approve, as well as change tracking/blaming/reverting independent parts becomes about an order of magnitude easier.

I would recommend submitting the initial refactors in one go, the bugfix in a second phase, and the last commit's madness last. As you said the last commit is hard to split up, I guess I'll have to bite the bullet there, but it will take time and it would be unfortunate if the PR gathered conflicts in the meantime - merging the rest should somewhat reduce the diff surface to help with that.

@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from 74233de to bac99d0 Compare July 25, 2024 18:53
@ithinuel ithinuel marked this pull request as draft July 25, 2024 18:53
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 94a6415 to 3685203 Compare July 25, 2024 19:12
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from 3685203 to 21ef060 Compare July 28, 2024 08:09
@ithinuel ithinuel marked this pull request as ready for review July 28, 2024 08:10
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 1eea56d to a7b8185 Compare July 28, 2024 23:42
@ithinuel ithinuel changed the title arm: changes a bunch of things in prep for ADIv6 arm: Use AP specific types & enumerate Addresses rather than APs Jul 29, 2024
Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

I'm not done reviewing yet, completing this will take time.

probe-rs/src/architecture/arm/ap/memory_ap/mod.rs Outdated Show resolved Hide resolved
probe-rs/src/probe/stlink/mod.rs Show resolved Hide resolved
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from a7b8185 to f9ea724 Compare July 29, 2024 16:55
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from f9ea724 to f57c24c Compare July 30, 2024 19:49
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch 2 times, most recently from 8b99a1e to 083c6b1 Compare July 31, 2024 06:56
- Enumerate Memory AP addresses instead of giving the number of valid
  (Generic and Memory) APs.
  This will enable future development where APv2’s live in disjoint memory
  spaces.
- Use AP specific types
  This should allow future developments to enabled things like secure
  debug, memory tagging, etc
- Drop `ApInformation` and `MemoryApInformation`
  The AP specific types carry these information internally so these types
  are no longer required.
@ithinuel ithinuel force-pushed the add-adiv6-support-step-2 branch from 083c6b1 to 93fe5ed Compare July 31, 2024 07:25
@bugadani
Copy link
Contributor

@Tiwalun should I leave this PR unmerged for a short while, or you'll only have comments after 5 minutes after it gets merged? 🫠

@Tiwalun
Copy link
Member

Tiwalun commented Jul 31, 2024

I think my interpretation of a short while is different than yours 😄

Feel free to merge this, I don't think I have time to look at this before the weekend.

@bugadani bugadani added this pull request to the merge queue Jul 31, 2024
Merged via the queue into probe-rs:master with commit 9eb1c2f Jul 31, 2024
20 checks passed
@ithinuel ithinuel deleted the add-adiv6-support-step-2 branch July 31, 2024 08:11
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.

3 participants