tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.
https://tinygo.org
BSD 3-Clause "New" or "Revised" License
622 stars 196 forks source link

Allow for backlight dimming on supported displays #687

Open ysmilda opened 5 months ago

ysmilda commented 5 months ago

The backlight on most LCD screens (including the one on the Gopherbadge) is based on some form of LED control. The current support for this in the tinygo drivers is limited to turning the backlight on and off which is implemented for three screens in the repo.

When the pin connected to the backlight also has a PWM peripheral connected to it we could expand this functionality. My proposal would be to extend the API for these screens to allow for passing in a PWM interface similar to the tone driver and use that to set the display backlight to a certain value/percentage.

From my experiments of enabling this on the users side we would need the following interface for the PWM:

type PWM interface {
    Configure(config machine.PWMConfig) error
    Channel(pin machine.Pin) (channel uint8, err error)
    Top() uint32
    Set(channel uint8, value uint32)
}

Adding this to the existing API is a bit more difficult as I'm not sure what the guidelines are on breaking changes. One thing that could be done would be to have a separate initialisation call in the lines of NewWithBacklightDimming and make the functions that handle the backlight a noop when the PWM interface has not been set. Optional names for these functions could be SetBacklight(percentage int8) or DimBacklight(percentage int8).

I'm not quite happy yet with how the above solution integrates with the current implementation. But I do think that this can add a valuable way to interact with these screens for users.

conejoninja commented 5 months ago

We are still coming back from gophercon.eu but I'm pinging @aykevl and @deadprogram for the interface design and breaking changes part, so they could review this.

I'm in for the breaking changes, but in this case I think it will break too much, it's too common. Maybe be could pass it in the Configure() function?

aykevl commented 5 months ago

The backlight works in different ways on different systems. On most systems, there is a single pin and high means on (and low means off). But there's also the PineTime, which has 3 backlight pins for different levels of brightness and they actually work in reverse: low is on and high is off.

My proposal would be to remove backlight support from the display drivers, or at least allow machine.NoPin so the user can control display brightness manually. What do you think?

Alternative: @ysmilda you are the person I talked with about DMA and PWM, right? This would be a good time to finally design a simpler API for the PWM, that's purely for controlling LEDs and such that don't need precise frequency control. That means most of the API could remain the same, with just a single extra method to set the backlight as a fractional value (we can bikeshed about the specific value, it could be 0..100 but also 0.255 or some other range).


I should also point out that humans have logarithmic perception of brightness, so if we set the backlight to 50% PWM value it actually appears much brighter (75% or so, I don't know the actual number). Changes in brightness will be much more noticeable at low brightness values than in high brightness values. The API should specify whether the value is linear or logarithmic (linear makes sense to me - mapping this to a logarithmic scale is something that's maybe best left for application developers).

aykevl commented 5 months ago

For comparison, in github.com/aykevl/board, I've used MaxBrightness to return the maximum possible brightness for a given screen and SetBrightness to set the brightness value (from 0 for "backlight off" to the MaxBrightness value). I haven't specified whether it's linear or logarithmic.

There's also a Linux kernel API we could take a look at, but it's kinda terrible so perhaps we shouldn't. (If you ever wondered why Linux laptop brightness values are wonky, this is why: different vendors had different ideas and now nobody can agree on the meaning of "brightness").

ysmilda commented 5 months ago

I like the proposal to just remove backlight support from the displays all together, but I would like to add the proposal of than providing a separate backlight driver. How this is going to handle the multiple types of backlights (e.g. proportional via PWM or pin based levels) I'm not sure yet. I'm leaning towards having a SetBrightness() function that tries it's best to match the given brightness, which for the PineTime would mean one of the 8 (? 2^3) backlight options.

Most systems I worked with don't abstract the logarithmic scale and let that for the app developer to handle, and I think that that is also what we should do here. That way the user keeps full control.

Having worked with the linux kernel API, let's not go there.


I am indeed the one you talked to about the DMA and PWM design. I gave my thoughts on the DMA part in the relevant issue. For the PWM I'm still debating on my stance. I'm leaning towards having an additional function to retrieve the PWM instance with a warning what that means with regards to changing multiple channels. Combining that with the way the drivers/tone driver handles PWM would give a nice API to handle this backlight.

conejoninja commented 5 months ago

I'm in favor of the SetBrightness(uint8) interface, and let each implementation to handle it accordingly. For example for pin based: 0-127 = off, 128-255 = on. PineTime 0-31 = 0, 32-63 = 1, 64-95 = 2, and go own... does it make sense?

ysmilda commented 5 months ago

To get a feel of what this could look like I've added a draft PR.

The driver in the PR can be used as is or maybe passed to the display driver. The latter makes sense for displays that support multiple variants, e.g a display that supports PWM also supports on/off. But is not sensible for every combination.