You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Karel Kočí <cy...@email.cz> on 2022/07/20 14:08:59 UTC
LCD Framebuffer putarea and display redraw
Hi
I discovered that commit 664d45dcbace03a879017aa99566592be31f4308 broke LCD
framebuffer (at least for ST7789). The issue is with `putarea` call. Originally
it was called only when full display redraw was requested but now it is called
every time when defined. The core of the issue is that from documentation the
buffer passed to the putarea should contain pixels for that area but LCD
framebuffer always passes the complete buffer.
I see two possible fixes. Either we call putarea only when full display update
is requested or we change the definition of the putarea in such a way that it is
driver's resposibility to pick the are from buffer. The first solution is simple
and is pretty much revert to the previous state. The second change is kind of
what is suggested in the new comment added in
664d45dcbace03a879017aa99566592be31f4308 but no work was done to propagate that
to actuall API documentation in 'include/nuttx/lcd/lcd.h' 😡.
Thus, what do you think? Revert or propagation of the new behavior?
With regards
Karel Kočí
Re: LCD Framebuffer putarea and display redraw
Posted by Karel Kočí <cy...@email.cz>.
Excerpts from Alan Carvalho de Assis's message of July 21, 2022 1:20 pm:
> Hi Karel,
>
> On Thursday, July 21, 2022, Karel Kočí <cy...@email.cz> wrote:
>
>> Hi
>>
>> Excerpts from Alan Carvalho de Assis's message of July 20, 2022 5:21 pm:
>> > Hi Karel,
>> >
>> > On 7/20/22, Karel Kočí <cy...@email.cz> wrote:
>> >> Hi
>> >>
>> >> I discovered that commit 664d45dcbace03a879017aa99566592be31f4308
>> broke LCD
>> >>
>> >> framebuffer (at least for ST7789). The issue is with `putarea` call.
>> >> Originally
>> >> it was called only when full display redraw was requested but now it is
>> >> called
>> >> every time when defined. The core of the issue is that from
>> documentation
>> >> the
>> >> buffer passed to the putarea should contain pixels for that area but LCD
>> >> framebuffer always passes the complete buffer.
>> >>
>> >
>> > Analyzing logically the expected behavior, now putarea() is now called
>> > correctly, so the modification that the guy did on PR #6551 is good.
>> > In the past when you was wanting to update only an area of the LCD
>> > there existed an updatearea() function inside each LCD drivers that
>> > was called.
>> >
>> > If you enable CONFIG_NX_UPDATE you will see that
>> > graphics/nxbe/nxbe_notify_rectangle.c still expecting that
>> > updatearea() inside the lcd_dev_s (I discovered it yesterday and I'm
>> > trying to fix it).
>> >
>> > That function was transfered to lcd_framebuffer.c but we you noticed
>> > it was calling putarea() only to redraw all the display, that is
>> > obviously incorrect. In the other hand, putrun() is a raster function
>> > that normally update the LCD line by line (it could be any row or
>> > column, but not an area like putarea).
>> >
>> > Yesterday I sent the PR #6639 that uses the putarea() to rendering of
>> > APA102 because it was using putrun() and I could see the image be
>> > rendered in the screen because APA102 requires me to send the bit
>> > stream sequentially for all the LEDs in the matrix.
>> >
>> > Now it is very fast as you can see here:
>> >
>> > https://www.youtube.com/watch?v=qA-UmD8ujlE
>> >
>> >> I see two possible fixes. Either we call putarea only when full display
>> >> update
>> >> is requested or we change the definition of the putarea in such a way
>> that
>> >> it is
>> >> driver's resposibility to pick the are from buffer. The first solution
>> is
>> >> simple
>> >> and is pretty much revert to the previous state. The second change is
>> kind
>> >> of
>> >> what is suggested in the new comment added in
>> >> 664d45dcbace03a879017aa99566592be31f4308 but no work was done to
>> propagate
>> >> that
>> >> to actuall API documentation in 'include/nuttx/lcd/lcd.h' 😡.
>> >>
>> >> Thus, what do you think? Revert or propagation of the new behavior?
>> >>
>> >
>> > Although the first solution is simpler it is incorrect (that is the
>> > way it was doing before), putarea exist to update an area of the LCD
>> > screen.
>> I am not against the idea at all. It makes sense.
>>
>> > I think the right solution is to fix ST7789 to update only the
>> > requested area received by putarea().
>> I am going to do that (at least for ST7789). I am also going to update
>> documentation for LCD API. Honestly, the only ambiguity and the reason for
>> my
>> original mail and the need to actually debug this was that this comment
>> was
>> ignored
>> https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912844774
>> and
>> changes were merged without updated documentation. It wasn't hard for me
>> to
>> locate the commit that broke it but it was hard to figure out what was the
>> original idea of the committer as that was missing in the commit message.
>> Going trough comments in PR is not exactly optimal even if I would not
>> ignore PR
>> all together.
>>
>>
>>
> I completely agree with you! During many years nice features were added on
> NuttX, but the documentation didn't follow it.
>
> Documentation still a weak factor on NuttX. Some years ago someone
> suggested we could run some script on our code to extract the functions
> documentation, because although we are not using Doxygen the current
> functions documentation seems very standardized.
I do not consider exported documentation as critical as I read it in header
files anyway most of the times.
The primary issue, as I see it, is the missing or not updated documentation, as
you stated as well. The missing documentation is not nice but I can still read
the code (documentation just makes work much more faster). The invalid
documentation is on the other hand pretty nasty and should be avoided as much as
possible.
In any case I created PR https://github.com/apache/incubator-nuttx/pull/6657
that resolves this issue for me but other drivers should be updated as well.
The quick grep revealed these LCD drivers:
* apa102 (seems to be updated in 9587e4a85e46dd9214f208914134edb34f6cb0a2)
* gc9a01 (has the same code as st7789 had and thus needs probably be fixed)
With regards
Karel Kočí
Re: LCD Framebuffer putarea and display redraw
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
Hi Karel,
On Thursday, July 21, 2022, Karel Kočí <cy...@email.cz> wrote:
> Hi
>
> Excerpts from Alan Carvalho de Assis's message of July 20, 2022 5:21 pm:
> > Hi Karel,
> >
> > On 7/20/22, Karel Kočí <cy...@email.cz> wrote:
> >> Hi
> >>
> >> I discovered that commit 664d45dcbace03a879017aa99566592be31f4308
> broke LCD
> >>
> >> framebuffer (at least for ST7789). The issue is with `putarea` call.
> >> Originally
> >> it was called only when full display redraw was requested but now it is
> >> called
> >> every time when defined. The core of the issue is that from
> documentation
> >> the
> >> buffer passed to the putarea should contain pixels for that area but LCD
> >> framebuffer always passes the complete buffer.
> >>
> >
> > Analyzing logically the expected behavior, now putarea() is now called
> > correctly, so the modification that the guy did on PR #6551 is good.
> > In the past when you was wanting to update only an area of the LCD
> > there existed an updatearea() function inside each LCD drivers that
> > was called.
> >
> > If you enable CONFIG_NX_UPDATE you will see that
> > graphics/nxbe/nxbe_notify_rectangle.c still expecting that
> > updatearea() inside the lcd_dev_s (I discovered it yesterday and I'm
> > trying to fix it).
> >
> > That function was transfered to lcd_framebuffer.c but we you noticed
> > it was calling putarea() only to redraw all the display, that is
> > obviously incorrect. In the other hand, putrun() is a raster function
> > that normally update the LCD line by line (it could be any row or
> > column, but not an area like putarea).
> >
> > Yesterday I sent the PR #6639 that uses the putarea() to rendering of
> > APA102 because it was using putrun() and I could see the image be
> > rendered in the screen because APA102 requires me to send the bit
> > stream sequentially for all the LEDs in the matrix.
> >
> > Now it is very fast as you can see here:
> >
> > https://www.youtube.com/watch?v=qA-UmD8ujlE
> >
> >> I see two possible fixes. Either we call putarea only when full display
> >> update
> >> is requested or we change the definition of the putarea in such a way
> that
> >> it is
> >> driver's resposibility to pick the are from buffer. The first solution
> is
> >> simple
> >> and is pretty much revert to the previous state. The second change is
> kind
> >> of
> >> what is suggested in the new comment added in
> >> 664d45dcbace03a879017aa99566592be31f4308 but no work was done to
> propagate
> >> that
> >> to actuall API documentation in 'include/nuttx/lcd/lcd.h' 😡.
> >>
> >> Thus, what do you think? Revert or propagation of the new behavior?
> >>
> >
> > Although the first solution is simpler it is incorrect (that is the
> > way it was doing before), putarea exist to update an area of the LCD
> > screen.
> I am not against the idea at all. It makes sense.
>
> > I think the right solution is to fix ST7789 to update only the
> > requested area received by putarea().
> I am going to do that (at least for ST7789). I am also going to update
> documentation for LCD API. Honestly, the only ambiguity and the reason for
> my
> original mail and the need to actually debug this was that this comment
> was
> ignored
> https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912844774
> and
> changes were merged without updated documentation. It wasn't hard for me
> to
> locate the commit that broke it but it was hard to figure out what was the
> original idea of the committer as that was missing in the commit message.
> Going trough comments in PR is not exactly optimal even if I would not
> ignore PR
> all together.
>
>
>
I completely agree with you! During many years nice features were added on
NuttX, but the documentation didn't follow it.
Documentation still a weak factor on NuttX. Some years ago someone
suggested we could run some script on our code to extract the functions
documentation, because although we are not using Doxygen the current
functions documentation seems very standardized.
So, as you could guess, it never happened.
BR,
Alan
Re: LCD Framebuffer putarea and display redraw
Posted by Karel Kočí <cy...@email.cz>.
Hi
Excerpts from Alan Carvalho de Assis's message of July 20, 2022 5:21 pm:
> Hi Karel,
>
> On 7/20/22, Karel Kočí <cy...@email.cz> wrote:
>> Hi
>>
>> I discovered that commit 664d45dcbace03a879017aa99566592be31f4308 broke LCD
>>
>> framebuffer (at least for ST7789). The issue is with `putarea` call.
>> Originally
>> it was called only when full display redraw was requested but now it is
>> called
>> every time when defined. The core of the issue is that from documentation
>> the
>> buffer passed to the putarea should contain pixels for that area but LCD
>> framebuffer always passes the complete buffer.
>>
>
> Analyzing logically the expected behavior, now putarea() is now called
> correctly, so the modification that the guy did on PR #6551 is good.
> In the past when you was wanting to update only an area of the LCD
> there existed an updatearea() function inside each LCD drivers that
> was called.
>
> If you enable CONFIG_NX_UPDATE you will see that
> graphics/nxbe/nxbe_notify_rectangle.c still expecting that
> updatearea() inside the lcd_dev_s (I discovered it yesterday and I'm
> trying to fix it).
>
> That function was transfered to lcd_framebuffer.c but we you noticed
> it was calling putarea() only to redraw all the display, that is
> obviously incorrect. In the other hand, putrun() is a raster function
> that normally update the LCD line by line (it could be any row or
> column, but not an area like putarea).
>
> Yesterday I sent the PR #6639 that uses the putarea() to rendering of
> APA102 because it was using putrun() and I could see the image be
> rendered in the screen because APA102 requires me to send the bit
> stream sequentially for all the LEDs in the matrix.
>
> Now it is very fast as you can see here:
>
> https://www.youtube.com/watch?v=qA-UmD8ujlE
>
>> I see two possible fixes. Either we call putarea only when full display
>> update
>> is requested or we change the definition of the putarea in such a way that
>> it is
>> driver's resposibility to pick the are from buffer. The first solution is
>> simple
>> and is pretty much revert to the previous state. The second change is kind
>> of
>> what is suggested in the new comment added in
>> 664d45dcbace03a879017aa99566592be31f4308 but no work was done to propagate
>> that
>> to actuall API documentation in 'include/nuttx/lcd/lcd.h' 😡.
>>
>> Thus, what do you think? Revert or propagation of the new behavior?
>>
>
> Although the first solution is simpler it is incorrect (that is the
> way it was doing before), putarea exist to update an area of the LCD
> screen.
I am not against the idea at all. It makes sense.
> I think the right solution is to fix ST7789 to update only the
> requested area received by putarea().
I am going to do that (at least for ST7789). I am also going to update
documentation for LCD API. Honestly, the only ambiguity and the reason for my
original mail and the need to actually debug this was that this comment was
ignored
https://github.com/apache/incubator-nuttx/pull/6551#discussion_r912844774 and
changes were merged without updated documentation. It wasn't hard for me to
locate the commit that broke it but it was hard to figure out what was the
original idea of the committer as that was missing in the commit message.
Going trough comments in PR is not exactly optimal even if I would not ignore PR
all together.
With regards
Karel Kočí
Re: LCD Framebuffer putarea and display redraw
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
Hi Karel,
On 7/20/22, Karel Kočí <cy...@email.cz> wrote:
> Hi
>
> I discovered that commit 664d45dcbace03a879017aa99566592be31f4308 broke LCD
>
> framebuffer (at least for ST7789). The issue is with `putarea` call.
> Originally
> it was called only when full display redraw was requested but now it is
> called
> every time when defined. The core of the issue is that from documentation
> the
> buffer passed to the putarea should contain pixels for that area but LCD
> framebuffer always passes the complete buffer.
>
Analyzing logically the expected behavior, now putarea() is now called
correctly, so the modification that the guy did on PR #6551 is good.
In the past when you was wanting to update only an area of the LCD
there existed an updatearea() function inside each LCD drivers that
was called.
If you enable CONFIG_NX_UPDATE you will see that
graphics/nxbe/nxbe_notify_rectangle.c still expecting that
updatearea() inside the lcd_dev_s (I discovered it yesterday and I'm
trying to fix it).
That function was transfered to lcd_framebuffer.c but we you noticed
it was calling putarea() only to redraw all the display, that is
obviously incorrect. In the other hand, putrun() is a raster function
that normally update the LCD line by line (it could be any row or
column, but not an area like putarea).
Yesterday I sent the PR #6639 that uses the putarea() to rendering of
APA102 because it was using putrun() and I could see the image be
rendered in the screen because APA102 requires me to send the bit
stream sequentially for all the LEDs in the matrix.
Now it is very fast as you can see here:
https://www.youtube.com/watch?v=qA-UmD8ujlE
> I see two possible fixes. Either we call putarea only when full display
> update
> is requested or we change the definition of the putarea in such a way that
> it is
> driver's resposibility to pick the are from buffer. The first solution is
> simple
> and is pretty much revert to the previous state. The second change is kind
> of
> what is suggested in the new comment added in
> 664d45dcbace03a879017aa99566592be31f4308 but no work was done to propagate
> that
> to actuall API documentation in 'include/nuttx/lcd/lcd.h' 😡.
>
> Thus, what do you think? Revert or propagation of the new behavior?
>
Although the first solution is simpler it is incorrect (that is the
way it was doing before), putarea exist to update an area of the LCD
screen.
I think the right solution is to fix ST7789 to update only the
requested area received by putarea().
BR,
Alan
Re: LCD Framebuffer putarea and display redraw
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Jul 20, 2022 at 10:09 AM Karel Kočí <cy...@email.cz> wrote:
> I discovered that commit 664d45dcbace03a879017aa99566592be31f4308 broke LCD
> framebuffer (at least for ST7789). The issue is with `putarea` call. Originally
> it was called only when full display redraw was requested but now it is called
> every time when defined. The core of the issue is that from documentation the
> buffer passed to the putarea should contain pixels for that area but LCD
> framebuffer always passes the complete buffer.
>
> I see two possible fixes. Either we call putarea only when full display update
> is requested or we change the definition of the putarea in such a way that it is
> driver's resposibility to pick the are from buffer. The first solution is simple
> and is pretty much revert to the previous state. The second change is kind of
> what is suggested in the new comment added in
> 664d45dcbace03a879017aa99566592be31f4308 but no work was done to propagate that
> to actuall API documentation in 'include/nuttx/lcd/lcd.h' 😡.
>
> Thus, what do you think? Revert or propagation of the new behavior?
For reference, this is PR #6551. I added a comment to it linking to
this email thread...
Nathan