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

Async API #214

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Async API #214

merged 2 commits into from
Aug 29, 2024

Conversation

sjoerdsimons
Copy link
Contributor

@sjoerdsimons sjoerdsimons commented Aug 10, 2024

Hi! Thank you for helping out with SSD1306 development! Please:

  • Check that you've added documentation to any new methods
  • Rebase from master if you're not already up to date
  • Add or modify an example if there are changes to the public API
  • Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc)
  • Run rustfmt on the project with cargo fmt --all - CI will not pass without this step
  • Check that your branch is up to date with master and that CI is passing once the PR is opened

PR description

Add async support to crate; API stays the same just async

@sjoerdsimons
Copy link
Contributor Author

As usual the circleci pipeline files (would be nice if it's removed at some point :p; @eldruin hope you don't mind me giving a ping as i don't know if you already notifications in new PR's :)

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you! Seems fine to me aside from the comments below but I am no async expert. Have you tested this in hardware?

@@ -29,15 +29,17 @@ jobs:
- run: cargo test --doc --target x86_64-unknown-linux-gnu

test-msrv:
name: build with MSRV
name: test with MSRV
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to only build with the MSRV and test with stable like it is currently done because we have a significant number of dev-dependencies and so it is quite probable that these will break the MSRV test although the failure is not related to this library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do so; but mind that with these patches the only dev-dependency on x86 left is embedded-graphics itself; Which is a lot more tractable then all the bigs required for arm (which is primarily for the examples)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eldruin sorry the previous message was meant to ask if you'd reconsider with that background :) Fwiw the main thing we do test with MSRV in this case is the doctest which imho is useful (and should really work with the MSRV)

If you'd still prefer to just build with MSRV then i'll do that change

Copy link
Member

Choose a reason for hiding this comment

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

I understand the utility from a software engineering perspective.
This is a matter of differing expectations w.r.t. what MSRV means. As a user, the MSRV means to me: can I use it as a dependency?
If the doctests do not compile anymore due to changes in MSRV in, say, embassy, does it mean that this crate must change its MSRV? I would say no. The ecosystem around it changed but this crate or its actual dependencies did not.
It should be noted that there are a whole bunch of dependencies that need to be compiled for this.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right my point was that with the changes i've done for x86 the only dev-dependency left is embedded-graphics itself (which any users is expected to also use).. So the whole bunch of crates is essentially the same betweenthe test and a build a user will do :)

Copy link
Member

Choose a reason for hiding this comment

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

I see. Sorry, I missed that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah; Well i rolled it back now as i didn't see the point in blocking on this for longer :) Happy to send a quick follow-up pr to re-introduce testing in ci if you changed your mind ofcourse

CHANGELOG.md Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/mode/terminal.rs Outdated Show resolved Hide resolved
src/mode/terminal.rs Outdated Show resolved Hide resolved
src/mode/terminal.rs Outdated Show resolved Hide resolved
src/mode/terminal.rs Outdated Show resolved Hide resolved
@sjoerdsimons
Copy link
Contributor Author

Thank you! Seems fine to me aside from the comments below but I am no async expert. Have you tested this in hardware?

Ofcourse; e.g. the async-i2c-spi example running on a breadboard

async_i2c_spi.mp4

Only add dev-dependendencies that are needed for the example on arm
targets as they're not needed anywhere else.
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Is this ready to go now?

@sjoerdsimons
Copy link
Contributor Author

Is this ready to go now?

That's for you to judge! but from my point of view yes

Cargo.toml Outdated Show resolved Hide resolved
use maybe_async_cfg to add an async interface as well, enabled by the
"async" feature. When enabled an Ssd1306Async struct is added with the
same api as Ssd1306, only async.
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@eldruin eldruin merged commit ad2f001 into rust-embedded-community:master Aug 29, 2024
11 of 12 checks passed
@plaes
Copy link

plaes commented Aug 29, 2024

Looks good to me, thank you!

Could you bump version as well?

@eldruin
Copy link
Member

eldruin commented Aug 30, 2024

I have just released this in version 0.9.0

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