ddcutil .deb

[Jonathan Riddell]

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