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
