Excerpts from Alan Carvalho de Assis's message of July 21, 2022 1:20 pm: > Hi Karel, > > On Thursday, July 21, 2022, Karel Kočí <cyn...@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čí <cyn...@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čí
pgpryb0szf8YC.pgp
Description: PGP signature
