ddcutil .deb

[Sanford Rockowitz]

Jonathan,

Much as I'd like to see some external pressure for ddcutil's inclusion 
in distros (it's been a slow process), I think it's premature to be 
including the library version in distros. Probably better off for now 
for PowerDevil to use its own private copy.  I'm happy to work with 
you/Dorian to improve the PowerDevil code.  It will help identify 
changes that the libddcutil API needs in order to be more usable.  One 
particular are of concern I have is support for multi-threaded clients.  
As I noted, DDC/CI operations are slow because of protocol mandated 
sleeps (typically 50 ms per operation).  GUIs may not want to wait for 
operations to complete.

Regards,
Sanford

On 09/04/2017 08:07 AM, Jonathan Riddell wrote:
> 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 <rockowitz at minsoft.com> -----
>
> Date: Fri, 1 Sep 2017 10:52:29 -0400
> From: Sanford Rockowitz <rockowitz at minsoft.com>
> To: Jonathan Riddell <jr at 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
>
>