Remove DisplayMode #119

bugadani · 2020-05-27T12:46:08Z

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

This PR removes the DisplayMode struct. As far as I can tell, DisplayMode is only used currently as the return value of Builder::connect() and it is immediately transformed into a specific DisplayModeTrait implementation.

This PR also removes the now obsolete RawMode which was closely connected to the above mechanism.

This is only a proposal and I fully expect it to be rejected. Also, the Into trait is not implemented for the display modes, as for some reason I get trait impl conflicts when I attempt to do so.

jamwaffles

This actually looks pretty good to me so far, so thanks for taking the time to clean this crate up a bit!

After running cargo fmt and TARGET=thumbv7m-none-eabi ./build.sh, I see the noise_i2c.rs fails to compile. The call to disp.release() should be replaced with disp.properties() (which incidentally is what I should've used in the first place!)

There's also a now-broken doc link as RawMode is removed. I haven't checked the docs yet but there are likely some paragraphs that can be removed now.

jamwaffles · 2020-05-27T13:35:33Z

If this is a superset of #118, please close #118 so we don't get duplicate PRs.

bugadani · 2020-05-27T13:40:55Z

Not so much a superset, but I branched off of that PR in case this one is rejected or that one is accidentally merged :) It's not about duplicate functionality, but incremental changes.

I have one more addition to make: brightness adjustment. Should I include it in this PR as well?

jamwaffles · 2020-05-27T13:46:53Z

Not so much a superset, but I branched off of that PR

Ok, understood! I notice both commits from the other PR are included in this one though, so it might be a good idea to rebase this PR from master to keep all these PRs more tightly scoped.

I have one more addition to make: brightness adjustment.

More additions always welcome! I would prefer this to be in a separate PR if you don't mind. For one thing, it helps keep the changelog organised :)

bugadani · 2020-05-27T13:53:49Z

so it might be a good idea to rebase this PR from master to keep all these PRs more tightly scoped.

Unfortunately, rebasing this to master would cause some conflict so it would be more work than merging #118 and then continuing work on this one. I'll tidy that one up first.

jamwaffles · 2020-05-27T13:57:01Z

Ok, no worries :)

therealprof · 2020-05-27T13:57:42Z

Unfortunately, rebasing this to master would cause some conflict so it would be more work than merging #118 and then continuing work on this one. I'll tidy that one up first.

That is superweird and unexpected. But it's a good idea to keep seperate things separate anyway.

This enabled reclaiming gpios which may be necessary for some hardware configurations to enter low power mode

bugadani · 2020-06-06T15:18:35Z

I've spent some time cleaning up the links. Unfortunately, it looks like the link checker doesn't know about inter-crate link format and fails, however I believe it is the preferred linking format since cargo doc automatically checks them when generating the docs.

Also, no longer based on #118

jamwaffles

Looking good so far, just some small things I picked up.

Unfortunately, it looks like the link checker doesn't know about inter-crate link format and fails

That's correct. The linkchecker in the build script was only ever designed for http:// links.

I really like the intra doc link feature and would much prefer to use it, but as it's currently still nightly I'd prefer to keep the relative path links for now.

jamwaffles · 2020-06-07T09:15:54Z

src/lib.rs

+//! returns a [`DisplayProperties`] instance which is a low level interface to manipulate the display properties (e.g. rotation).
+//! You can coerce the driver into a more useful mode by calling `into()` and defining the type you want to coerce to. For


Please wrap this to 100 columns.

While we're here, I'd also suggest rewording the You can coerce the driver part to The driver can be coerced.

jamwaffles · 2020-06-07T09:22:16Z

src/builder.rs

+//!
+//! [`I2CDIBuilder`]: crate::builder::I2CDIBuilder
+//! [`DisplayProperties`]: crate::properties::DisplayProperties
+//! [`GraphicsMode`]: crate::mode::graphics::GraphicsMode
+//! [`TerminalMode`]: crate::mode::terminal::TerminalMode


Much cleaner, thanks. As mentioned in the review comments, please change these links (and all others in the PR) back to relative paths. Linkchecker and manual docs surfing should help here.

bugadani · 2020-06-07T09:59:17Z

I'll make the changes once I'm at a computer, but are the intra-crate links really nightly? I'll also double check that but I think they are stable, at least docs.rs knows about them IIRC.

jamwaffles · 2020-06-07T10:02:13Z

Unfortunately yes, at least until rust-lang/rust#43466 is closed I guess. As for docs.rs, https://docs.rs/about states that:

Docs.rs automatically builds crates' documentation released on crates.io using the nightly release of the Rust compiler.

Which is why they work there.

therealprof · 2020-06-07T15:25:45Z

Nice cleanup. Would be great to land this.

therealprof

Excellent.

jamwaffles

👌

jamwaffles

Argh, one last thing I didn't catch before in the changelog. I doubt many people were using RawMode but its removal is technically a breaking change.

The name is also incorrect which might be confusing to anybody migrating code using the changelog.

CHANGELOG.md

Co-authored-by: James Waples <[email protected]>

bugadani · 2020-06-08T08:23:19Z

I hope you don't mind if I took the lazy way of pressing the Commit suggestion button 😅

jamwaffles · 2020-06-08T08:28:37Z

No problem - I squash merged this PR anyway :). Thanks for the changes!

jamwaffles requested changes May 27, 2020

View reviewed changes

bugadani force-pushed the feature/remove-displaymode branch 2 times, most recently from f9615ec to bc1ff12 Compare May 27, 2020 17:02

bugadani added 6 commits June 6, 2020 16:50

Add release method

08a7b70

This enabled reclaiming gpios which may be necessary for some hardware configurations to enter low power mode

Remove DisplayMode

2308aa5

Remove some unnecessary trait bounds

d8908c6

Move reset method as it doesn't need WriteOnlyDataCommand

dea68a3

Add DisplayModeTrait to prelude

59740aa

Doc link fixes

3a788cc

bugadani force-pushed the feature/remove-displaymode branch from bc1ff12 to 3a788cc Compare June 6, 2020 14:51

Dániel Buga added 4 commits June 6, 2020 16:53

Run cargo fmt

2df8ca4

Fix links

000139d

Add CHANGELOG entry

e771132

More link fixes

7376e78

bugadani marked this pull request as ready for review June 6, 2020 17:16

jamwaffles requested changes Jun 7, 2020

View reviewed changes

therealprof mentioned this pull request Jun 7, 2020

Allow reclaiming display pins #118

Merged

6 tasks

Dániel Buga added 3 commits June 7, 2020 21:24

Word wrapping fix

916854c

Change back to standard relative links

00dde5b

Fix missed link

41c9a59

therealprof previously approved these changes Jun 7, 2020

View reviewed changes

jamwaffles approved these changes Jun 7, 2020

View reviewed changes

jamwaffles requested changes Jun 8, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

568789f

Co-authored-by: James Waples <[email protected]>

bugadani dismissed therealprof’s stale review via 568789f June 8, 2020 08:21

jamwaffles approved these changes Jun 8, 2020

View reviewed changes

jamwaffles merged commit 83c6cf6 into rust-embedded-community:master Jun 8, 2020

bugadani deleted the feature/remove-displaymode branch June 8, 2020 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove DisplayMode #119

Remove DisplayMode #119

bugadani commented May 27, 2020 •

edited

Loading

jamwaffles left a comment

jamwaffles commented May 27, 2020

bugadani commented May 27, 2020

jamwaffles commented May 27, 2020

bugadani commented May 27, 2020

jamwaffles commented May 27, 2020

therealprof commented May 27, 2020

bugadani commented Jun 6, 2020 •

edited

Loading

jamwaffles left a comment

jamwaffles Jun 7, 2020

jamwaffles Jun 7, 2020

bugadani commented Jun 7, 2020

jamwaffles commented Jun 7, 2020

therealprof commented Jun 7, 2020

therealprof left a comment

jamwaffles left a comment

jamwaffles left a comment

bugadani commented Jun 8, 2020

jamwaffles commented Jun 8, 2020

		//! returns a [`DisplayProperties`] instance which is a low level interface to manipulate the display properties (e.g. rotation).
		//! You can coerce the driver into a more useful mode by calling `into()` and defining the type you want to coerce to. For

Remove DisplayMode #119

Remove DisplayMode #119

Conversation

bugadani commented May 27, 2020 • edited Loading

PR description

jamwaffles left a comment

Choose a reason for hiding this comment

jamwaffles commented May 27, 2020

bugadani commented May 27, 2020

jamwaffles commented May 27, 2020

bugadani commented May 27, 2020

jamwaffles commented May 27, 2020

therealprof commented May 27, 2020

bugadani commented Jun 6, 2020 • edited Loading

jamwaffles left a comment

Choose a reason for hiding this comment

jamwaffles Jun 7, 2020

Choose a reason for hiding this comment

jamwaffles Jun 7, 2020

Choose a reason for hiding this comment

bugadani commented Jun 7, 2020

jamwaffles commented Jun 7, 2020

therealprof commented Jun 7, 2020

therealprof left a comment

Choose a reason for hiding this comment

jamwaffles left a comment

Choose a reason for hiding this comment

jamwaffles left a comment

Choose a reason for hiding this comment

bugadani commented Jun 8, 2020

jamwaffles commented Jun 8, 2020

bugadani commented May 27, 2020 •

edited

Loading

bugadani commented Jun 6, 2020 •

edited

Loading