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 more examples. Update doctests. #2547

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

playfulFence
Copy link
Contributor

Description

Moves things forward in terms of #2513.
Some examples are still there, part of them will be removed when QA repo/directory is published.

Testing

xtask run-doc-tests esp-hal <chip>

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 18, 2024

I'd say some of the "examples" which are deleted here should go to qa-test (ideally hil test but most of them cannot) instead of just deleting them.

While it's nice the doc tests compile, we should easily be able to verify those drivers still do what they are supposed to do

@playfulFence
Copy link
Contributor Author

I guess since #2558 was merged, I can move part of deleted examples to qa-test directory now.

@MabezDev
Copy link
Member

Let's wait for the release to merge this

@MabezDev MabezDev marked this pull request as draft November 18, 2024 14:07
esp-hal/src/gpio/mod.rs Outdated Show resolved Hide resolved
@playfulFence playfulFence force-pushed the fix/cull_examples branch 2 times, most recently from 5724fc3 to 58358a3 Compare November 21, 2024 11:14
esp-hal/CHANGELOG.md Outdated Show resolved Hide resolved
esp-hal/src/ledc/mod.rs Outdated Show resolved Hide resolved
@playfulFence playfulFence marked this pull request as ready for review November 21, 2024 12:16
@playfulFence playfulFence force-pushed the fix/cull_examples branch 2 times, most recently from c3c7170 to 2a4787a Compare November 21, 2024 16:50
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.

This is close to ready, just some final bits to restore/move and it should be good to go!

examples/src/bin/sleep_timer.rs Outdated Show resolved Hide resolved
examples/src/bin/embassy_hello_world.rs Outdated Show resolved Hide resolved
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.

There's a bunch of unnecessary noise in the doctests, please clean this up. I may have missed some too.

esp-hal/src/uart.rs Outdated Show resolved Hide resolved
esp-hal/src/timer/timg.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32s3/psram.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32s3/psram.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32s3/efuse/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32/psram.rs Outdated Show resolved Hide resolved
esp-hal/src/soc/esp32/efuse/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/rtc_cntl/sleep/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/rtc_cntl/sleep/mod.rs Outdated Show resolved Hide resolved
esp-hal/src/rtc_cntl/sleep/mod.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 22, 2024

I guess in some places there are commented out println! lines because before that didn't compile - we now have that in the before-snippet so no need to comment out these (but it's println! not esp_println::println!)

@playfulFence
Copy link
Contributor Author

I think what @jessebraham meant is that some of those printlns were more of a noise for documentation than a benefit 🤔

I think I agree, probably most of those println made sense in the example but not in the documentation.

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.

There are still quite a lot of print's that are commented out, we recently added noop println:

# macro_rules! println {
# ($($tt:tt)*) => { };
# }

Feel free to add print! there too, and then proceed to uncomment the prints

esp-hal/src/parl_io.rs Outdated Show resolved Hide resolved
esp-hal/src/parl_io.rs Outdated Show resolved Hide resolved
@MabezDev
Copy link
Member

I think I agree, probably most of those println made sense in the example but not in the documentation.

I agree that most aren't needed, but some are.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 22, 2024

Also in a lot of places we now have writeln!(serial_tx,"..... which is definitely more noise in examples than println!

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.

Thanks! This is looking good from my side

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