You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/09/09 13:31:06 UTC
[GitHub] [incubator-nuttx] shuai532209720 opened a new pull request #4504: add new interface for fb driver and lcd dev driver
shuai532209720 opened a new pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504
## Summary
### fb
* add get and set panel power interface for fb driver.
* add get and set framerate interface for fb driver.
* add panel information at videoinfo for fb driver.
### lcd dev
* add get and set framerate interface for lcd dev driver.
## Impact
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] shuai532209720 commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
shuai532209720 commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705364324
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
LCD cannot be operated using framebuffer mode only. If both are enabled, the code is redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705904932
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
> It does not seem correct for the Framebuffer API to handle these operations on the display device.
> Why not make these go through the LCD APIs?
Here is several reasons:
1. FB and LCD driver interface is already separated in the current design(e.g. FBIOPUT_CURSOR v.s. LCDDEVIO_SETCURSOR). So, the new IOCTL(e.g. FBIOSET_POWER) reduce the difference between two interfaces.
2. If we expose the display related operation into two device nodes, it make the userspace GUI library integration harder, because:
- The library need to manage two handles instead one
- It's hard to identify fb/lcd pair if the device support multiple display
> I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
Two problems here:
1. LCD driver already expose LCDDEVIO_GETPOWER/LCDDEVIO_SETPOWER
2. FBIOSET_POWER accept an integer argument to specify the power level, but is strange to pass the power level to FBIO_INIT
2. More important, up_fbuninitialize will destroy device node, and then is impossible to reenable FB again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705904932
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
> It does not seem correct for the Framebuffer API to handle these operations on the display device.
> Why not make these go through the LCD APIs?
Here are several reasons:
1. FB and LCD driver interface is already separated in the current design(e.g. FBIOPUT_CURSOR v.s. LCDDEVIO_SETCURSOR). So, the new IOCTL(e.g. FBIOSET_POWER) reduce the difference between two interfaces.
2. If we expose the display related operation into two device nodes, it make the userspace GUI library integration harder, because:
- The library need to manage two handles instead one
- It's hard to identify fb/lcd pair if the device support multiple display
> I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
Two problems here:
1. LCD driver already expose LCDDEVIO_GETPOWER/LCDDEVIO_SETPOWER
2. FBIOSET_POWER accept an integer argument to specify the power level, but is strange to pass the power level to FBIO_INIT
2. More important, up_fbuninitialize will destroy device node, and then is impossible to reenable FB again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705393291
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
https://github.com/apache/incubator-nuttx/blob/60b2a0e2a03878198db24a451bfe115a7d5e0e54/include/nuttx/video/fb.h#L638-L653
What about renaming the `FBIO[GET/SET]_POWER` IOCTL commands for ones that something like `FBIO_[INIT/DEINIT]`? Then the implementation for these commands would rely on the `up_fbinitialize`/`up_fbuninitialize` APIs. Does this work out right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] shuai532209720 commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
shuai532209720 commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705450223
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
FBIO[GET/SET]_POWER was used to control panel backlight.
As for the framerate configuration, it can be used for AOD dispalys.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705393291
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
https://github.com/apache/incubator-nuttx/blob/60b2a0e2a03878198db24a451bfe115a7d5e0e54/include/nuttx/video/fb.h#L638-L653
What about renaming the `FBIO[GET/SET]_POWER` IOCTL commands for ones that something like `FBIO_[INIT/DEINIT]`? Then the implementation for these commands would rely on the `up_fbinitialize`/`up_fbuninitialize` APIs. Would this work out right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei merged pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
gustavonihei merged pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705393291
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
https://github.com/apache/incubator-nuttx/blob/60b2a0e2a03878198db24a451bfe115a7d5e0e54/include/nuttx/video/fb.h#L638-L653
What about renaming the `FBIO[GET/SET]_POWER` IOCTL commands for ones that something like `FBIO_[INIT/DEINIT]`? Then the implementation for these commands would rely on the `up_fbinitialize`/`up_fbuninitialize` APIs. Would this work out right?
As for the framerate configuration, it also seems weird to me, but I don't have the required background for suggesting anything different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705343611
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
It does not seem correct for the Framebuffer API to handle these operations on the display device.
Why not make these go through the LCD APIs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705393291
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
https://github.com/apache/incubator-nuttx/blob/60b2a0e2a03878198db24a451bfe115a7d5e0e54/include/nuttx/video/fb.h#L638-L653
What about renaming the `FBIO[GET/SET]_POWER` IOCTL commands for something like `FBIO_[INIT/DEINIT]`? Then the implementation for these commands would rely on the `up_fbinitialize`/`up_fbuninitialize` APIs. Would this work out right?
As for the framerate configuration, it also seems weird to me, but I don't have the required background for suggesting anything different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4504: add new interface for fb driver and lcd dev driver
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4504:
URL: https://github.com/apache/incubator-nuttx/pull/4504#discussion_r705904932
##########
File path: include/nuttx/video/fb.h
##########
@@ -569,6 +583,24 @@ struct fb_vtable_s
FAR const struct fb_overlayblend_s *blend);
# endif
#endif
+
+ /* Specific Controls ******************************************************/
+
+ /* Set the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*setframerate)(FAR struct fb_vtable_s *vtable, int rate);
+
+ /* Get the frequency of the framebuffer update panel (0: disable refresh) */
+
+ int (*getframerate)(FAR struct fb_vtable_s *vtable);
+
+ /* Get the panel power status (0: full off). */
+
+ int (*getpower)(FAR struct fb_vtable_s *vtable);
+
+ /* Enable/disable panel power (0: full off). */
+
+ int (*setpower)(FAR struct fb_vtable_s *vtable, int power);
Review comment:
> It does not seem correct for the Framebuffer API to handle these operations on the display device.
> Why not make these go through the LCD APIs?
Here are several reasons:
1. FB and LCD driver interface is already separated in the current design(e.g. FBIOPUT_CURSOR v.s. LCDDEVIO_SETCURSOR). So, the new IOCTL(e.g. FBIOSET_POWER) reduce the difference between two interfaces.
2. If we expose the display related operation into two device nodes, it make the userspace GUI library integration harder, because:
- The library need to manage two handles instead one
- It's hard to identify fb/lcd pair if the device support multiple display
> I believe the expected method for powering off the display device according to the Framebuffer API is through `fb_uninitialize`, but nobody is currently calling it.
Two problems here:
1. LCD driver already expose LCDDEVIO_GETPOWER/LCDDEVIO_SETPOWER
2. FBIOSET_POWER accept an integer argument to specify the power level, but it is strange to pass the power level to FBIO_INIT
2. More important, up_fbuninitialize will destroy device node, and then is impossible to reenable FB again
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org