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

drivers: display: Add display_clear function #84555

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rruuaanng
Copy link
Collaborator

@rruuaanng rruuaanng commented Jan 26, 2025

from #12937

vanwinkeljan:
It is suggested that a new API should be introduced to clear the display screen, rather than misusing display_set_contrast.

cc @vanwinkeljan

I believe this makes sense for application developers. Sometimes, when implementing screen clearing, I have to write functions that send raw messages, so I prefer to leave the implementation to the driver vendors rather than the users.

Add a display_clear function to the display subsystem
to clear the target devices screen display. Introduce
a clear interface at the driver layer to implement
the display_clear function.

Signed-off-by: James Roy <[email protected]>
Add the display_clear function in the display
subsystem to clear the target devices screen
display by resetting it to white.

Signed-off-by: James Roy <[email protected]>
@rruuaanng rruuaanng marked this pull request as ready for review February 1, 2025 01:56
@zephyrbot zephyrbot added Release Notes To be mentioned in the release notes area: Display labels Feb 1, 2025
@fabiobaltieri fabiobaltieri requested a review from soburi February 1, 2025 19:05
Comment on lines +302 to +303
struct display_driver_api *api =
(struct display_driver_api *)dev->api;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this would fit in 80 columns, unwrap?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to synchronize the code style of other functions, I have to do this (maybe I can take this opportunity to tidy up their style.

static inline int display_read(const struct device *dev, const uint16_t x,
			       const uint16_t y,
			       const struct display_buffer_descriptor *desc,
			       void *buf)
{
	struct display_driver_api *api =
		(struct display_driver_api *)dev->api;

	if (api->read == NULL) {
		return -ENOSYS;
	}

	return api->read(dev, x, y, desc, buf);
}

/**
* @typedef display_clear
* @brief Callback API for clearing the screen of the display
* See display_clear() for argument description
Copy link
Member

@soburi soburi Feb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it is simply called "clear," its behavior is not uniquely determined.
It would be better to make it more clear.
For example, setting memory to 0 (in this case, the colors will be inverted for MONO01 and MONO10), making the screen black...
Perhaps it would be better to call the action of filling the entire screen with a specific color "clear" (It makes it easier to keep the behavior unique, so I prefer this.).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of these difficulties, I think including a reference implementation with SDL in this PR would be a good idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, setting memory to 0 (in this case, the colors will be inverted for MONO01 and MONO10), making the screen black...

The default clearing behavior can be defined as filling the screen with solid black(I think)—unless inversion is enabled, in which case it would be white. In other words, I believe this should be determined by the driver implementer. If they consider the "default" state of the screen to be white or any other color, that should be acceptable.

Because of these difficulties, I think including a reference implementation with SDL in this PR would be a good idea.

I believe display_sdl.c is an appropriate position.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When designing a HAL, it is necessary to find a "greatest common denominator" that allows uniform behavior on any hardware and is compatible with many hardware devices.
This is also the valuable benefit of using Zephyr.
Therefore, "depends on implementation" is a kind of resignation, and the design must avoid this.

@rruuaanng rruuaanng marked this pull request as draft February 3, 2025 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Display Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants