rust-embedded-community / ssd1306

SSD1306 OLED driver
Apache License 2.0
300 stars 68 forks source link

Remove DisplayMode #119

Closed bugadani closed 4 years ago

bugadani commented 4 years ago

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 commented 4 years ago

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

bugadani commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Ok, no worries :)

therealprof commented 4 years ago

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.

bugadani commented 4 years ago

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

bugadani commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Nice cleanup. Would be great to land this.

bugadani commented 4 years ago

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

jamwaffles commented 4 years ago

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