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 2022/07/01 10:26:02 UTC
[GitHub] [incubator-nuttx] adamkaliszan opened a new pull request, #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
adamkaliszan opened a new pull request, #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551
Kconfig changes
SSD1680 driver changes
ioctl(..., FBIO_REDRAW, ...) added
## Summary
It is possible to ask E-ink driver to redraw. In case of frame buffer modification, the screen is not updated. We want to reduce number of redraw operation. It is time and energy consuming. Moreover during redrowing, display blinks twice.
In order to redraw use ioctl function.
Param CONFIG_FB_UPDATE doesn't solve the problem when we want draw more then one areas with one redrawing the dispaly.
## Impact
ioctl for frame buffer changes. Added value FBIO_REDRAW.
## Testing
Yes on ssd1680 e-ink display. There are bugs in driver, that will be fixed in separate pull request. The part with redrawing works fine.
--
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 merged pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912355692
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
> So if there is Null argument whole content of frame buffer is send to driver and next driver redraws the screen. I would like to stress that there are three steps to display something:
>
> 1. User app writes into the frame buffer
> 2. Content of frame buffer is moved into the memory of driver
> 3. Display generates picture on base of memory data. It is done "automatically". E-ink is an exception of a rule. There is a special command that generate the picture.
Why not merge step 2 and 3 into one? I am not familiar with E-ink. Does E-ink have special requirement?
>
> > You can pass NULL pointer to indicate to update the whole screen
> > > On the other hand in order to synchronize frame buffer with driver we have to call it. After your suggestion I'm going to:
> > >
> > > * modify function that synchronize fb with driver (this function is called after ioctl(.., FB_UPDATE, ..). After modification e-ink will redraw its content
> > > * leave Kconfig changes
>
> In my previous implementation FBIO_UPDATE was triggering the step 2 and FBIO_REDRAW was triggering the step 3. I'm going to implement that FBIO_UPDATE with null argument triggers the steps 2 and 3. When we copy whole frame buffer, there is no need to specify area.
You can still merge step 2 and 3 even FBIO_UPDATE with area, I think. area just mean update the full screen to simplify the most common case.
>
> > It's no real difference to ioctl(FBIO_UPDATE) or ioctl(FBIO_REDRAW).
> > > I'm trying to solve bug that I discovered in SSD1680 driver.
> >
> >
> > If so, the ioctl is better to define for SSD1680 only.
> > There is no need. SSD works corectly when we copy all rows. The problem was when I was updating some rows. I noticed, that the content of not modified rows was changed. ioctl(.., FBIO_UPDATE, NULL) will solve the problem.
> > > The problem exists if we are updating some pixels in drivers memory, but when we update all pixels everything works perfectly (hardware bug???). So I'm going to postpone updating the memory of driver till we want to redraw the screen. Before redrawing the screen all the content of shadow of frame buffer will be sent to e-ink display.
> >
> >
> > this is exactly reason why FBIO_UPDATE was created.
> > I think, that it was created to avoid of using task that periodically updates the driver memory (read data from frame buffer) or a task that compares the content of drivers memory and frame buffer. Display's memory is updated when frame buffer has different content then display. I'm going to extend functionality of FBIO_UPDATE by triggering step 3. Only e-ink drivers need this trigger.
FBIO_UPDATE work with the hardware which isn't transfer the framebuffer continuously to the panel, but require the manual trigger:
1. Software update the frame buffer content as need
2. Manually call FBIO_UPDATE to trigger the transfer when the update finish
> > > There is one more reason of using shadow buffer (it consumes memory): implementation of function that read the content of display. The SPI driver have no implementation of half duplex mode and the display wokrs in SPI half duplex (MISO and MOSI lines are shared).
> > > So concluding there are many reasons of using FB_UPDATE. The drawback of FB_UPDATE is that some application may call to often FB_UPDATE functionality. On the other hand such applications should be modified to work with e-ink display
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912734241
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,40 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
-
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
Review Comment:
I think, that drivers implementation of putarea callback should check if there is a full screen mode.
In case of driver with shadow frame buffer and full screen mode, the layouts are the same (we can use memcpy or DMA).
In case of full screen and lack of shadow buffer some drivers have the same memory layout. E.g. SSD in portrait mode has the same layout.
I would move full screen checking to drivers implementation.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912871360
##########
drivers/lcd/ssd1680.c:
##########
@@ -126,11 +126,11 @@ struct ssd1680_dev_s
/* Libc extension */
Review Comment:
OK. I forgot about description. I will fix it.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r913255275
##########
drivers/lcd/ssd1680.c:
##########
@@ -126,11 +126,11 @@ struct ssd1680_dev_s
/* Libc extension */
Review Comment:
I wrote extended memcpy functions, where we can address bits (not bytes). Libc doesnt have fuction tohat let us to address bits.
I cal remove this comment. I wrote just two functions. I was wandering if add it to libs or not.
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912361028
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
@adamkaliszan could you review whether https://github.com/apache/incubator-nuttx/pull/6564 could resolve your problem.
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
@adamkaliszan could you review whether https://github.com/apache/incubator-nuttx/pull/6564 could resolve your problem.
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r911967645
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
> FB_UPDATE is more uncomfortable in use, because we have to specify area.
You can pass NULL pointer to indicate to update the whole screen
> On the other hand in order to synchronize frame buffer with driver we have to call it. After your suggestion I'm going to:
>
> * modify function that synchronize fb with driver (this function is called after ioctl(.., FB_UPDATE, ..). After modification e-ink will redraw its content
> * leave Kconfig changes
>
It's no real difference to ioctl(FBIO_UPDATE) or ioctl(FBIO_REDRAW).
> I'm trying to solve bug that I discovered in SSD1680 driver.
If so, the ioctl is better to define for SSD1680 only.
> The problem exists if we are updating some pixels in drivers memory, but when we update all pixels everything works perfectly (hardware bug???). So I'm going to postpone updating the memory of driver till we want to redraw the screen. Before redrawing the screen all the content of shadow of frame buffer will be sent to e-ink display.
this is exactly reason why FBIO_UPDATE was created.
> There is one more reason of using shadow buffer (it consumes memory): implementation of function that read the content of display. The SPI driver have no implementation of half duplex mode and the display wokrs in SPI half duplex (MISO and MOSI lines are shared).
>
> So concluding there are many reasons of using FB_UPDATE. The drawback of FB_UPDATE is that some application may call to often FB_UPDATE functionality. On the other hand such applications should be modified to work with e-ink display
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r911899938
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
@adamkaliszan look like FB_UPDATE implement the similar feature?
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912446544
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
I agree. I will add new callback and modify ssd1680 e-ink driver.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912214117
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
So if there is Null argument whole content of frame buffer is send to driver and next driver redraws the screen. I would like to stress that there are three steps to display something:
1) User app writes into the frame buffer
2) Content of frame buffer is moved into the memory of driver
3) Display generates picture on base of memory data. It is done "automatically". E-ink is an exception of a rule. There is a special command that generate the picture.
> You can pass NULL pointer to indicate to update the whole screen
>
> > On the other hand in order to synchronize frame buffer with driver we have to call it. After your suggestion I'm going to:
> >
> > * modify function that synchronize fb with driver (this function is called after ioctl(.., FB_UPDATE, ..). After modification e-ink will redraw its content
> > * leave Kconfig changes
>
In my previous implementation FBIO_UPDATE was triggering the step 2 and FBIO_REDRAW was triggering the step 3. I'm going to implement that FBIO_UPDATE with null argument triggers the steps 2 and 3. When we copy whole frame buffer, there is no need to specify area.
> It's no real difference to ioctl(FBIO_UPDATE) or ioctl(FBIO_REDRAW).
>
> > I'm trying to solve bug that I discovered in SSD1680 driver.
>
> If so, the ioctl is better to define for SSD1680 only.
There is no need. SSD works corectly when we copy all rows. The problem was when I was updating some rows. I noticed, that the content of not modified rows was changed. ioctl(.., FBIO_UPDATE, NULL) will solve the problem.
>
> > The problem exists if we are updating some pixels in drivers memory, but when we update all pixels everything works perfectly (hardware bug???). So I'm going to postpone updating the memory of driver till we want to redraw the screen. Before redrawing the screen all the content of shadow of frame buffer will be sent to e-ink display.
>
> this is exactly reason why FBIO_UPDATE was created.
I think, that it was created to avoid of using task that periodically updates the driver memory (read data from frame buffer) or a task that compares the content of drivers memory and frame buffer. Display's memory is updated when frame buffer has different content then display. I'm going to extend functionality of FBIO_UPDATE by triggering step 3. Only e-ink drivers need this trigger.
>
> > There is one more reason of using shadow buffer (it consumes memory): implementation of function that read the content of display. The SPI driver have no implementation of half duplex mode and the display wokrs in SPI half duplex (MISO and MOSI lines are shared).
> > So concluding there are many reasons of using FB_UPDATE. The drawback of FB_UPDATE is that some application may call to often FB_UPDATE functionality. On the other hand such applications should be modified to work with e-ink display
--
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 diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r913367272
##########
drivers/lcd/ssd1680.c:
##########
@@ -722,10 +715,11 @@ static int ssd1680_configuredisplay(struct ssd1680_dev_s *priv)
}
/****************************************************************************
- * Name: ssd1680_update_all
+ * Name: ssd1680_update_all_and_redraw
Review Comment:
Ok, it's fine to keep as the current change. Please address other comments, thanks.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912247006
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
Review Comment:
NULL => send all frame bufer data to driver and next redraw display.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r911936115
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
FB_UPDATE is more uncomfortable in use, because we have to specify area. On the other hand in order to synchronize frame buffer with driver we have to call it.
After your suggestion I'm going to:
- modify function that synchronize fb with driver (this function is called after ioctl(.., FB_UPDATE, ..). After modification e-ink will redraw its content
- leave Kconfig changes
I'm trying to solve bug that I discovered in SSD1680 driver. The problem exists if we are updating some pixels in drivers memory, but when we update all pixels everything works perfectly (hardware bug???). So I'm going to postpone updating the memory of driver till we want to redraw the screen. Before redrawing the screen all the content of shadow of frame buffer will be sent to e-ink display. There is one more reason of using shadow buffer (it consumes memory):
implementation of function that read the content of display. The SPI driver have no implementation of half duplex mode and the display wokrs in SPI half duplex (MISO and MOSI lines are shared).
So concluding there are many reasons of using FB_UPDATE. The drawback of FB_UPDATE is that some application may call to often FB_UPDATE functionality. On the other hand such applications should be modified to work with e-ink display
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912725481
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,40 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
-
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
Review Comment:
I think the memory layout is different from the frame buffer and what's lcd driver want. That's why I add the full screen check before forward to putarea.
--
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] pkarashchenko commented on a diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912860389
##########
drivers/lcd/ssd1680.c:
##########
@@ -1312,8 +1303,8 @@ FAR struct lcd_dev_s *ssd1680_initialize(FAR struct spi_dev_s *dev,
return &priv->dev;
}
-FAR void *bitscpy_ds(FAR void *dest, int dest_offset, FAR const void *src,
- size_t nbits)
+static FAR void *bitscpy_ds(FAR void *dest, int dest_offset,
+ FAR const void *src, size_t nbits)
Review Comment:
If possible:
```suggestion
static FAR void *bitscpy_ds(FAR void *dest, int dest_offset,
FAR const void *src, size_t nbits)
```
##########
drivers/lcd/ssd1680.c:
##########
@@ -1371,8 +1362,8 @@ FAR void *bitscpy_ds(FAR void *dest, int dest_offset, FAR const void *src,
return dest;
}
-FAR void *bitscpy_ss(FAR void *dest, FAR const void *src, int src_offset,
- size_t nbits)
+static FAR void *bitscpy_ss(FAR void *dest, FAR const void *src,
+ int src_offset, size_t nbits)
Review Comment:
If possible:
```suggestion
static FAR void *bitscpy_ss(FAR void *dest, FAR const void *src,
int src_offset, size_t nbits)
```
##########
include/nuttx/lcd/lcd.h:
##########
@@ -137,6 +137,8 @@ struct lcd_planeinfo_s
* buffers.
*/
+ int (*redraw)(FAR struct lcd_dev_s *dev);
Review Comment:
I think this should be moved and placed below `getarea`. A proper description should be added as well
##########
drivers/lcd/ssd1680.c:
##########
@@ -126,11 +126,11 @@ struct ssd1680_dev_s
/* Libc extension */
Review Comment:
is below interface really a `Libc extension`? Or it is a copy-paste leftover?
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912247479
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
If the last argument has value 0, driver redraws display. Maybe it is better to add a pointer to the function that redraws display?
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912441043
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
> #6564 solve the problem partially. It solve the problem of NULL value, but two things are missing:
>
> 1. caling funcion **ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);**
> 2. Eink driver changes. I can add to my pull request code from [drivers/lcd_framebuffer: Optimize updateearea for the full screan case #6564](https://github.com/apache/incubator-nuttx/pull/6564). My version has less operation of assignments, but some part of code is duplicated.
>
> I would like to discuss callling function **ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);** If there is 0 pixels to update, the e-ink driver redraws the screen. Maybe it is better to add (to object **pinfo**) a pointer to the function that triggers redrawing screen. In order to reduce the number of changes I decided to use pointer **putrun** to trigger redrawing the screen.
>
if lcd driver doesn't implement updatearea, the additional callback or special meaning of putrun in lcd_dev_s is required to get the notification of the end update. I think the new callback is better because:
1. it is more clean to express the intent
2. it is more compatible with the driver which doesn’t need the end of update.
> I can merge #6564 to my pull request.
>
> Unfortunately SSD1680 driver has no implemented function updatearea. I can implement it in a future. This display has very nice support for it, when works in portrait mode.
--
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] hartmannathan commented on pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#issuecomment-1190368455
Please note: This change may require additional changes as reported in the email thread ["LCD Framebuffer putarea and display redraw"](https://lists.apache.org/thread/4yk5z9qlb9qhslwpbyzgkt07ff578or9) started 20 July 2022.
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912357302
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
Do you notice putarea callback in lcd_planeinfo_s? you don't need additional putrun with special parameters to notify lcd driver that the update is end.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912395022
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
#6564 solve the problem partially. It solve the problem of NULL value, but two things are missing:
1) caling funcion **ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);**
2) Eink driver changes. I can add to my pull request code from #6564. My version has less operation of assignments, but some part of code is duplicated.
I would like to discuss callling function **ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);** If there is 0 pixels to update, the e-ink driver redraws the screen. Maybe it is better to add (to object **pinfo**) a pointer to the function that triggers redrawing screen.
In order to reduce the number of changes I decided to use pointer **putrun** to trigger redrawing the screen.
I can merge https://github.com/apache/incubator-nuttx/pull/6564 to my pull request.
Unfortunately SSD1680 driver has no implemented function updatearea. I can implement it in a future. This display has very nice support for it, when works in portrait mode.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912367361
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
Step 2 and 3 are merged now :). I was considering implementation of driver without shadow memory. In this case we have to write immediately to drivers memory. In order to avoid display redrawing after each memory update the steps 2 and 3 ware separated.
Now I'm going to write small optimization of SSD1680 driver for landscape mode (8x less data transver wia SPI). I will do it in next pull request.
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912725481
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,40 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
-
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
Review Comment:
I think the memory layout is different from the frame buffer and what's lcd driver want.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912864162
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,40 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
-
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
Review Comment:
OK it is ready.
--
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 diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912844774
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,40 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
-
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
Review Comment:
Ok, reasonable. could you add some comment in the related place to notify the driver writer?
--
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 diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912920264
##########
include/nuttx/lcd/lcd.h:
##########
@@ -123,6 +123,21 @@ struct lcd_planeinfo_s
fb_coord_t row_end, fb_coord_t col_start,
fb_coord_t col_end, FAR uint8_t *buffer);
+ /* This method can be used to redraw display's content.
+ *
+ * dev - LCD interface to redraw its memory content
+ *
+ * NOTE: In case of non e-ink dispalys redrawing is cheap and can be done
+ * atter each memory modification. Redrawing e-ink display is time and
Review Comment:
atter->after
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,50 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
+ /* Each Driver's callback function putarea may be optimized by checking
+ * if it is a full screen/full row mode or not.
+ * In case of full screen/row mode the memory layout of drivers memory
+ * and data provided to putarea function may be (or not, it depends of
+ * display and driver implementation) identical.
+ * Identical memory layout let us to use:
+ * - memcopy (if there is shadow buffer in driver implementation)
+ * - apply DMA channel to transfer data to driver memory.
+ */
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
+ if (ret < 0)
{
- /* Yes, go the fast path */
-
- return pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
+ lcderr("Failed to update area");
+ return ret;
}
}
-
- for (row = starty; row <= endy; row++)
+ else
{
- /* REVISIT: Some LCD hardware certain alignment requirements on DMA
- * memory.
- */
+ width = endx - startx + 1;
- ret = pinfo->putrun(pinfo->dev, row, startx, run, width);
- if (ret < 0)
+ /* Get the starting position in the framebuffer */
+
+ run = priv->fbmem + starty * priv->stride;
+ run += (startx * pinfo->bpp + 7) >> 3;
Review Comment:
dup at line 219-220
##########
drivers/lcd/ssd1680.c:
##########
@@ -722,10 +715,11 @@ static int ssd1680_configuredisplay(struct ssd1680_dev_s *priv)
}
/****************************************************************************
- * Name: ssd1680_update_all
+ * Name: ssd1680_update_all_and_redraw
Review Comment:
why not rename ssd1680_update_all_and_redraw to ssd1680_redraw?
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -220,34 +220,50 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
run += (startx * pinfo->bpp + 7) >> 3;
}
- /* Update the whole screen? */
-
- if (startx == 0 && endx == priv->xres - 1 &&
- starty == 0 && endy == priv->yres - 1)
+ if (pinfo->putarea != NULL)
{
- /* Yes, LCD driver support putarea callback? */
+ /* Each Driver's callback function putarea may be optimized by checking
+ * if it is a full screen/full row mode or not.
+ * In case of full screen/row mode the memory layout of drivers memory
+ * and data provided to putarea function may be (or not, it depends of
+ * display and driver implementation) identical.
+ * Identical memory layout let us to use:
+ * - memcopy (if there is shadow buffer in driver implementation)
+ * - apply DMA channel to transfer data to driver memory.
+ */
- if (pinfo->putarea != NULL)
+ ret = pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
+ if (ret < 0)
{
- /* Yes, go the fast path */
-
- return pinfo->putarea(pinfo->dev, starty, endy, startx, endx, run);
+ lcderr("Failed to update area");
+ return ret;
}
}
-
- for (row = starty; row <= endy; row++)
+ else
{
- /* REVISIT: Some LCD hardware certain alignment requirements on DMA
- * memory.
- */
+ width = endx - startx + 1;
Review Comment:
remove, done at line 215
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912247479
##########
drivers/lcd/lcd_framebuffer.c:
##########
@@ -173,7 +173,30 @@ static int lcdfb_updateearea(FAR struct fb_vtable_s *vtable,
fb_coord_t endy;
int ret;
- DEBUGASSERT(area != NULL);
+ if (area == NULL)
+ {
+ run = priv->fbmem;
+
+ for (row = 0; row < priv->yres; row++)
+ {
+ ret = pinfo->putrun(pinfo->dev, row, 0, run, priv->xres);
+ if (ret < 0)
+ {
+ return ret;
+ }
+
+ run += priv->stride;
+ }
+
+ /* Just redraw the display
+ * TODO add new function pointer to xtruct lcd_planeinfo_s
+ * Now calling pointer putrun with npix = 0 activates redrawing display
+ */
+
+ ret = pinfo->putrun(pinfo->dev, 0, 0, priv->fbmem, 0);
Review Comment:
if last argument has value 0 driver redraws display. Maybe it is better to add a pointer to the function that redraws display?
--
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 diff in pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912361028
##########
drivers/lcd/Kconfig:
##########
@@ -18,6 +18,15 @@ menuconfig LCD
if LCD
+config FB_REDRAW
Review Comment:
@adamkaliszan could you review whether https://github.com/apache/incubator-nuttx/pull/6564 could resolve your problem.
--
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 pull request #6551: framebuffer for e-ink display. Added new IOCTL param FBIO_REDRAW
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#issuecomment-1173480760
@adamkaliszan the title need update too.
--
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] adamkaliszan commented on a diff in pull request #6551: framebuffer for e-ink display. Added callback function to redraw.
Posted by GitBox <gi...@apache.org>.
adamkaliszan commented on code in PR #6551:
URL: https://github.com/apache/incubator-nuttx/pull/6551#discussion_r913264470
##########
drivers/lcd/ssd1680.c:
##########
@@ -722,10 +715,11 @@ static int ssd1680_configuredisplay(struct ssd1680_dev_s *priv)
}
/****************************************************************************
- * Name: ssd1680_update_all
+ * Name: ssd1680_update_all_and_redraw
Review Comment:
Updating drivers memory is postponed till redrawing. Previous name was confusing for me. <aybe it is better to call this function ssd1680_update_memory_and_redraw ?
--
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