Brightness change - Githubissues

bugadani commented 4 years ago

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

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

PR description

Some open questions that I still have:

Should there be an option for custom brightness levels? Adjusting the precharge period can make the screen go darker than default, but the results are not straight forward
Should there be an additional method per display mode? I personally prefer the change_property with a closure, because it allows for the same (and more) functionality without implementing a bunch of forwarder methods. Or maybe it is overly generic in the current form.

So for now, this is only a draft.

bugadani commented 4 years ago

I believe the various display properties need some thought here. I prefer the closure method for setting them, because it doesn't require implementing the same functionality in like 3 places ( one impl and 2 forwarders in the display modes), but your request is totally sensible. Should I include other properties in the common set (i.e. the trait)?

bugadani commented 4 years ago

I've copied the rtfm_dvd example and set it up so when the logo hits the sides, the brightness changes. Unfortunately, I have no hardware to test it.

I'm still leaving this as draft, because I just don't like the 2 parameter implementation (omitting precharge period 1 was an arbitrary choice, for example), it doesn't feel... exact.

bugadani commented 4 years ago

I really don't have a better idea of how to implement this in a somewhat nice manner, so I'm marking the PR as ready.

jamwaffles commented 4 years ago

I just merged #126 so if you can give this a rebase I'll approve and merge once the build passes.

jamwaffles commented 4 years ago

The build is failing with some unused/not found errors.

Once fixed, you can try running TARGET=thumbv7m-none-eabi ./build.sh on your PC to test things before pushing if you want.

bugadani commented 4 years ago

Ah yes, I renamed change_brightness to set, but forgot the example... Oh well

bugadani commented 4 years ago

That's better

jamwaffles commented 4 years ago

Thanks!

rust-embedded-community / ssd1306

Brightness change #121

PR description