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

Remove the SystemExt trait and rename SystemParts to SystemControl #1495

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

jessebraham
Copy link
Member

@jessebraham jessebraham commented Apr 22, 2024

Further work towards #1401, finally removes the SystemExt trait entirely. The SystemParts struct has been renamed to SystemControl (open to suggestions on naming here, but this seemed to fit with the other structs we've defined) which now has a constructor which takes the SYSTEM peripheral.

@MabezDev I'm still not 100% sure if we're on the same page regarding these changes or not, so would appreciate your feedback here. As usual happy to make any changes.

While I was in here I also noticed that the SoftwareInterrupt struct had no fields, so converted it to a unit struct instead.

I also moved some CHANGELOG.md entries which were mistakenly added to the wrong release in another PR.

@jessebraham jessebraham requested a review from MabezDev April 22, 2024 16:14
@jessebraham jessebraham force-pushed the refactor/system branch 2 times, most recently from 9357cd2 to f69c651 Compare April 22, 2024 16:25
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for taking care of this!

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@MabezDev MabezDev enabled auto-merge April 23, 2024 13:18
@MabezDev MabezDev added this pull request to the merge queue Apr 23, 2024
Merged via the queue into esp-rs:main with commit 086b605 Apr 23, 2024
21 checks passed
@jessebraham jessebraham deleted the refactor/system branch April 23, 2024 13:47
@Ben-PH
Copy link

Ben-PH commented Apr 24, 2024

It seems this PR is the source of some frustration I had today, and I believe it comes down to there being no version bump:

  • I was wanting to try the pcnt module.
  • I copy-pasted the pcnt example code.
  • Imports could not be resolved.
  • lsp "goto definition" on systems module: desired SystemControl struct nowhere to be found in source
  • updated my local clone of esp-hal and ran the pcnt as instructed: all good
  • looked at upstream main branch and found SystemControl
  • looked it up on my local updated clone, found it where expected
  • changed my toml to us esp-hal = { git = "...} and that fixed the issue

My conclusion is that without the version bump, cargo does not know that it needs to refresh/update esp-hal.

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.

5 participants