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