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

RFC: Change/deprecation in display API #17508

Closed
pabigot opened this issue Jul 12, 2019 · 4 comments
Closed

RFC: Change/deprecation in display API #17508

pabigot opened this issue Jul 12, 2019 · 4 comments
Assignees
Labels
area: API Changes to public APIs area: Display RFC Request For Comments: want input from the community

Comments

@pabigot
Copy link
Collaborator

pabigot commented Jul 12, 2019

I propose that the display API functions named display_blanking_{on,off} be deprecated and replaced with these functions and clarified semantics:

/**
 * @brief Turn the display off.
 *
 * This function turns off the display.  The expected result for
 * e-paper displays is that the displayed content is unchanged.  The
 * expected result for other displays is that the display will appear
 * blank.
 *
 * Drivers that control backlighting should turn it off.
 *
 * Drivers that support power control may power down the display.
 *
 * Display content in frame buffers held by the driver or device may
 * be lost as a result of this operation, whether or not the content
 * remains displayed.
 *
 * Use display_on() prior to any further interaction with the display
 * device.  Attempts to update the display while it is off produce
 * undefined behavior.
 *
 * @return a non-negative value on success, or a negative error code.
 */
static inline int display_off(const struct device *dev);

/**
 * @brief Turn the display on.
 *
 * This function turns on the display.
 *
 * If display content is not preserved by display_off() the driver
 * should ensure that the display is set to consistent content
 * (cleared to black, a distinct "no data" pattern, etc.) when this
 * operation completes.
 *
 * If backlighting is controlled by the display driver, backlighting
 * should be turned on consistent with the last backlighting
 * configuration.  (Not all displays may support backlighting; not all
 * displays that support it may allow control of it.)
 *
 * On successful return from this function the application may use
 * display_write() to provide new content.
 *
 * @return a non-negative value on success, or a negative error code.
 */
static inline int display_on(const struct device *dev);

There are several motivations for this, the primary one being that the existing names are counter-intuitive:

  • display_blanking_on() is implemented in three drivers as turning the display off (ili9340, sdl, ssd1306). In one it is implemented to turn the display on (ssd16xx).
  • display_blanking_off() is implemented in three drivers as turning the display on. In one it is implemented to turn the display off.
  • Three drivers don't support the operation, perhaps because it was unclear what the functions were supposed to do (dummy, framebuf, mcux_elcdif).

A second is that some displays, particularly e-paper ones, should be powered down when not in use. The existing API does not address how that could be accomplished. Porting the whole power management API to display drivers is a bigger step that IMO is not yet justified, so the proposed documentation ties power control to display state.

The existing drivers are inconsistent whether the display is turned on or off by driver initialization. I am ambivalent on the desired behavior, but feel strongly that it must be specified. I'm leaning toward display is on (i.e. applications that care should turn it off or write to it ASAP), because that is the most convenient approach for e-paper displays and should be consistent with existing drivers except perhaps sdl which turns it off (sorta). Preferences?

Please provide design feedback here. Based on that I will prepare a PR that updates the API to go along with the IL03xx e-paper driver I'm working on now.

@pabigot pabigot added the RFC Request For Comments: want input from the community label Jul 12, 2019
@pabigot pabigot added area: Display area: API Changes to public APIs labels Jul 12, 2019
@vanwinkeljan
Copy link
Member

The intention of the API is to only blank the display panel and not to control the power state of the display controller. To control the power states the power management API should be used, see comment on PR #7833 (comment) . Note that in the initial API proposal the function where called display_on and display_off but to avoid the confusion that it would do power control they were renamed.

display_blanking_on() is implemented in three drivers as turning the display off (ili9340, sdl, ssd1306). In one it is implemented to turn the display on (ssd16xx).

The ssd16xx implementation is incorrect and should be swapped, the same issue occurred for the ssd1306 driver (#13609). I will raise an issue for this and fix it.

Three drivers don't support the operation, perhaps because it was unclear what the functions were supposed to do (dummy, framebuf, mcux_elcdif).

The dummy API was introduced to be able to run valgrind with the native posix target as the SDL implementation doesn't clean up its memory allocation as such the blanking API just returns "no error".
Can't comment on the 2 other driver but either they do not support hardware based blanking or it is just not implemented.

Porting the whole power management API to display drivers is a bigger step that IMO is not yet justified

Can you clarify why it is not justified to use the power management framework?

The existing drivers are inconsistent whether the display is turned on or off by driver initialization. I am ambivalent on the desired behavior, but feel strongly that it must be specified. I'm leaning toward display is on (i.e. applications that care should turn it off or write to it ASAP), because that is the most convenient approach for e-paper displays and should be consistent with existing drivers except perhaps sdl which turns it off (sorta). Preferences?

I would have picked off as the ILI display controller put the display panel off after reset, but I agree we should consolidate and document the behaviour. My idea was that this gives the application time to prepare the initial buffer before enabling the display panel.

@pabigot
Copy link
Collaborator Author

pabigot commented Jul 14, 2019

Can you clarify why it is not justified to use the power management framework?

Mostly because if "display off" was acceptable there would be no obvious need for other power management infrastructure. The current API does not explain what "blanking" means, and the only use of the term I can find that is relevant to displays refers to the intervals when the electron flow is turned off during horizontal and vertical scanning in CRTs. I don't understand why it's being used here, other than it was required for approval (referencing #7833 (comment)).

If the preferred understanding is that "blanking on" means the display shows nothing but can be immediately restored to its pre-blanking state through "blanking off" with no other operations, rather like screen save, that should be documented, along with the expected effect of blanking on any driver-controlled backlighting. But that functionality makes little sense for e-paper displays, so I'll make those operations -ENOTSUP and either implement the driver power management, or just always turn the display off between writes.

Stepping back, I raised this issue because the display API does not describe behavior in sufficient detail for me to implement a driver for it, so I guessed (wrongly). Explaining what blanking is, documenting the required effect of other operations (such as writing new data) while blanking is on, and specifying the blanking state of a newly initialized driver, would go some ways towards fixing that gap.

@vanwinkeljan
Copy link
Member

@pabigot thx for the info I will update the API documentation.

@vanwinkeljan
Copy link
Member

Created PR #17521 to correct ssd16xx driver

@pabigot pabigot closed this as completed Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Display RFC Request For Comments: want input from the community
Projects
None yet
Development

No branches or pull requests

4 participants