Hi all, Thanks for the comments ! And sorry for the late reply. I realize the implementation is far from optimal, and would be happy to rework it. Unfortunately I'm quite busy with other things right now, and for about two more weeks so I can not really swear anything until then.
Jonathan: keeping the feature "in development" for now sounds reasonable, the fact that powerdevil's architecture does not allow ddc brightness control for laptop might become a source of frustration for users. Regards, Dorian Le 4 sept. 2017 2:07 PM, "Jonathan Riddell" <j...@jriddell.org> a écrit : > > Thanks for your comments Sanford. The code isn't mine it's done by > Dorian for KDE Plasma's Powerdevil which gets a beta release next week > (I'm the release dude). > > Dorian please review ddcutil Sanford's comments below for changes that > could be made. > > Sanford given the API/ABI stability should we be recommending distros > package up libddcutil and use this code or would that cause more > problems and we should mark it as in-development for this release? > > Jonathan > > > ----- Forwarded message from Sanford Rockowitz <rockow...@minsoft.com> > ----- > > Date: Fri, 1 Sep 2017 10:52:29 -0400 > From: Sanford Rockowitz <rockow...@minsoft.com> > To: Jonathan Riddell <j...@jriddell.org> > Subject: Re: ddcutil .deb > > Jonathan, > > I've taken a look at your powerdevil code at > https://cgit.kde.org/powerdevil.git/tree/daemon/backends/ > upower/ddcutilbrightness.cpp. > It can be simplified to improve performance and reliability. > > 1) The DDCA_Display_Identifier -> DDCA_Display_Ref lookup exists to > allow for choosing a monitor by its characteristics, when you don't > already have a list of monitors to select from, e.g. on the ddcutil > command line. Struct DDCA_Display_Info, returned (as an array) by > ddca_display_info_list(), contains field dref, which is a valid > DDCA_Display_Ref you can use. See function > display_selection_using_ddca_get_displays() in sample file > demo_display_selection.c. > > The documentation could be clearer here. > > 2) There's no need to look up the VCP value for the Brightness field. > Per the MCCS spec, it's always x10. > > 3) Rather than reading and parsing the capabilities string to > determine if a monitor supports VCP feature code x10, it's both faster > and more reliable just to try reading the value of code x10. > > Reading the capabilities string is a very expensive operation, > entailing multiple I2C write/read exchanges, as a single read can > return at most 32 bytes. Every write/read exchange contains sleeps > mandated by the DDC/CI protocol. (In fact, ddcutil spends 90% of it's > elapsed time sleeping.) Since the I2C protocol is unreliable, a > write/read exchange may have to be retried, and even worse a whole > sequence of write/read exchanges may have be to restarted. Most > monitors are relatively clean, but some, such Dell P2412h monitors > that I have tested, have so many I2C failures that read capabilities > sometimes fails even after retries. > > Moreover, the capabilities string is not necessarily accurate. See, > for example, the description of the HP LP2480zx monitor. on page > http://www.ddcutil.com/monitor_notes/. The string does not include > VCP feature code x10, even though the monitor supports it. > > BTW, while monitors vary enormously in the VCP feature codes they > support, I've yet to see a monitor that doesn't support feature x10. > > 4) You may want to control/capture messages issued from the library. > See functions ddca_set_fout(), ddca_set_ferr(), > ddca_set_output_level(), etc. > > 5) In certain very exceptional situations (i.e. things that should > never happen, but hey, it's conceivable) the library may abort. > Instead of aborting, the library can return to a location in the > caller registered using function ddca_register_jmp_buf(). See > function handle_library_abort() in sample file demo_global_settings.c. > I have to admit that I'm not entirely comfortable with making a > longjmp visible to the library client, but I'm trying to avoid forcing > the library user to check for truly exceptional failures on each API > call, while still avoiding library aborts that cause the client to > fail. Let me know if you find this design useful/usable. > > Regards, > Sanford > >
