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

Examples and test cleanup #1590

Merged
merged 3 commits into from
May 28, 2024
Merged

Conversation

MabezDev
Copy link
Member

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Closes #1436

Opening a bit before finishing (I plan on polishing some of the remaining examples) but I wanted to get opinions on the examples I nuked, and the ones I've kept so far.

@MabezDev MabezDev added the skip-changelog No changelog modification needed label May 23, 2024
hil-test/tests/interrupt.rs Outdated Show resolved Hide resolved
@Dominaezzz
Copy link
Collaborator

Are the deleted examples going to become tests? Otherwise I don't see benefit in losing them.

The eh1 SPI examples, aren't they useful to demonstrate how to get from a HAL type to an eh1 trait object? (I'm a bit biased since I'm trying to avoid merge conflicts 😅)
Or perhaps this is something that should be a snippet in the doc.

@MabezDev
Copy link
Member Author

Are the deleted examples going to become tests? Otherwise I don't see benefit in losing them.

Most already are, at least the crypto stuff is tested, and I converted the direct vectoring example to an interrupt latency test. The other examples IMO do not add that much value, I don't personally see the value in an example that does the same as another but imports eh to do so.

Copy link
Member

@jessebraham jessebraham 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!

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 - Thanks!

@jessebraham jessebraham enabled auto-merge May 24, 2024 18:22
@jessebraham jessebraham added this pull request to the merge queue May 24, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 24, 2024
* initial cull of examples

* Remove direct vector example, replace with interrupt latency test

* Remove clock monitor in favour of just a test

---------

Co-authored-by: Jesse Braham <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 24, 2024
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 force-pushed the examples-and-test-cleanup branch from 7734644 to 16695e7 Compare May 28, 2024 12:28
@MabezDev MabezDev enabled auto-merge May 28, 2024 12:28
@MabezDev MabezDev force-pushed the examples-and-test-cleanup branch from 16695e7 to b2aa8f5 Compare May 28, 2024 12:28
@MabezDev MabezDev added this pull request to the merge queue May 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 28, 2024
@MabezDev MabezDev force-pushed the examples-and-test-cleanup branch from b2aa8f5 to 14d0c6e Compare May 28, 2024 14:40
@MabezDev MabezDev enabled auto-merge May 28, 2024 14:41
@MabezDev MabezDev added this pull request to the merge queue May 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 28, 2024
* initial cull of examples

* Remove direct vector example, replace with interrupt latency test

* Remove clock monitor in favour of just a test
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 28, 2024
@jessebraham
Copy link
Member

This test seems pretty flaky, maybe we should skip it for the time being, just for the sake of getting the other changes merged?

@MabezDev
Copy link
Member Author

The thing is I'm quite confused as to why there is more latency than I see running it locally 🤔. I thought about doing a few runs, but actually, the first-time latency is an important metric in its own right.

I think I'll just set a high enough number for now so that it passes; it will still catch interrupts not working at all for example as the test will time out.

@MabezDev MabezDev force-pushed the examples-and-test-cleanup branch from 14d0c6e to 4b976e7 Compare May 28, 2024 18:41
@MabezDev MabezDev added this pull request to the merge queue May 28, 2024
Merged via the queue into esp-rs:main with commit db39d01 May 28, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

De-duplicate and generally improve examples
6 participants