Add brightness control using ddcutil lib
ClosedPublic

Authored by dvogel on Apr 10 2017, 11:24 AM.

Details

Summary

Setting up ddcutil for non-root CLI use is required first.
Please refer to https://github.com/d-vogel/QMLddcutil/blob/master/README.md section.

Limitations:
Not tested for multiple DDC capable monitors setup, should only change brightness of first monitor anyway.

Diff Detail

Repository
R122 Powerdevil
Lint
Lint Skipped
Unit
Unit Tests Skipped
dvogel created this revision.Apr 10 2017, 11:24 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 10 2017, 11:24 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik edited edge metadata.Apr 10 2017, 12:28 PM

Cool!

daemon/backends/upower/ddcutilbrightness.cpp
7

No need to explicitly initialize these, the constructor could just remain empty

17

Coding style, opening brace for functions on next line:

void DDCUtilBrightness:detect()
{
...
}
22

Prefer nullptr

26

Coding style:

qCWarning(POWERDEVIL) << "...";

Also, no need for the line breaks? Also, should rather be qCDebug or qCInfo, it's not a warning after all

31

Coding style:

for (int i = 0; i < ...; ++i) {

(brace on the same line for everything else, ie. for, while, if)

37

Perhaps clear() the list at the beginning of this method? Also, use reserve() if you already know how many items you're going to append to the list

52–53

Coding style, braces on the same line:

} else {

Also, I would prefer if you returned (and or use continue in a loop) instead of nesting if after if, ie.

if (!foo) {
    return;
}
if (!bar) {
    return;
}

instead of

if (!foo) {
    if (!bar) {
        ...
73

You don't seem to be cleaning up those containers in the destructor (there is none). Also, I don't think you should allocate those on the heap

139

return !m_displayHandleList.isEmpty();

daemon/backends/upower/ddcutilbrightness.h
1

Missing copyright header in this and other files

2

Feel free to use #pragma once in PowerDevil code :)

22

Prefer QVector over QList

daemon/backends/upower/powerdevilupowerbackend.cpp
217–218

Coding style:

} else {
382

Whitespace

413

Whitespace

716

Coding style

if (m_brightnessControl->isSupported()) {
dvogel marked 9 inline comments as done.Apr 11 2017, 9:16 PM

Ok, so there is apparently an issue when the screen gets dimmed.

Application: org_kde_powerdevil (org_kde_powerdevil), signal: Segmentation fault
Using host libthread_db library "/usr/lib/libthread_db.so.1".
[Current thread is 1 (Thread 0x7f6097336840 (LWP 740))]

Thread 5 (Thread 0x7f607bfff700 (LWP 757)):
#0  0x00007f6090dc637d in read () at /usr/lib/libc.so.6
#1  0x00007f608c055aa0 in  () at /usr/lib/libglib-2.0.so.0
#2  0x00007f608c01126e in g_main_context_check () at /usr/lib/libglib-2.0.so.0
#3  0x00007f608c011744 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007f608c011b32 in g_main_loop_run () at /usr/lib/libglib-2.0.so.0
#5  0x00007f6081190446 in  () at /usr/lib/libgio-2.0.so.0
#6  0x00007f608c039175 in  () at /usr/lib/libglib-2.0.so.0
#7  0x00007f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#8  0x00007f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 4 (Thread 0x7f6080823700 (LWP 756)):
#0  0x00007f6090dc637d in read () at /usr/lib/libc.so.6
#1  0x00007f608c055aa0 in  () at /usr/lib/libglib-2.0.so.0
#2  0x00007f608c01126e in g_main_context_check () at /usr/lib/libglib-2.0.so.0
#3  0x00007f608c011744 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007f608c0118bc in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#5  0x00007f608c011901 in  () at /usr/lib/libglib-2.0.so.0
#6  0x00007f608c039175 in  () at /usr/lib/libglib-2.0.so.0
#7  0x00007f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#8  0x00007f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 3 (Thread 0x7f60824eb700 (LWP 746)):
#0  0x00007f6090dc637d in read () at /usr/lib/libc.so.6
#1  0x00007f608c055aa0 in  () at /usr/lib/libglib-2.0.so.0
#2  0x00007f608c01126e in g_main_context_check () at /usr/lib/libglib-2.0.so.0
#3  0x00007f608c011744 in  () at /usr/lib/libglib-2.0.so.0
#4  0x00007f608c0118bc in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#5  0x00007f6091e4406b in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#6  0x00007f6091ded89a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#7  0x00007f6091c0fa73 in QThread::exec() () at /usr/lib/libQt5Core.so.5
#8  0x00007f60925e4125 in  () at /usr/lib/libQt5DBus.so.5
#9  0x00007f6091c146d8 in  () at /usr/lib/libQt5Core.so.5
#10 0x00007f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#11 0x00007f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 2 (Thread 0x7f608393f700 (LWP 743)):
#0  0x00007f6090dca67d in poll () at /usr/lib/libc.so.6
#1  0x00007f609194a8e0 in  () at /usr/lib/libxcb.so.1
#2  0x00007f609194c679 in xcb_wait_for_event () at /usr/lib/libxcb.so.1
#3  0x00007f6085c9b239 in  () at /usr/lib/libQt5XcbQpa.so.5
#4  0x00007f6091c146d8 in  () at /usr/lib/libQt5Core.so.5
#5  0x00007f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#6  0x00007f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 1 (Thread 0x7f6097336840 (LWP 740)):
[KCrash Handler]
#6  0x00007f607ab9a425 in DDCutilBrightness::brightness() const (this=0x1eb51c0) at /home/dorianvogel/plasma-dev/powerdevil/daemon/backends/upower/ddcutilbrightness.cpp:153
#7  0x00007f607ab8b615 in PowerDevilUPowerBackend::brightness(PowerDevil::BackendInterface::BrightnessControlType) const (this=0x1ea92f0, type=PowerDevil::BackendInterface::Screen) at /home/dorianvogel/plasma-dev/powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp:426
#8  0x00007f607ab8bd2b in PowerDevilUPowerBackend::setBrightness(int, PowerDevil::BackendInterface::BrightnessControlType) (this=0x1ea92f0, value=57, type=PowerDevil::BackendInterface::Screen) at /home/dorianvogel/plasma-dev/powerdevil/daemon/backends/upower/powerdevilupowerbackend.cpp:481
#9  0x00007f6096e18b4f in PowerDevil::BundledActions::DimDisplay::triggerImpl(QMap<QString, QVariant> const&) (this=0x1ed71e0, args=...) at /home/dorianvogel/plasma-dev/powerdevil/daemon/actions/bundled/dimdisplay.cpp:93
#10 0x00007f6096dec3ce in PowerDevil::Action::trigger(QMap<QString, QVariant> const&) (this=0x1ed71e0, args=...) at /home/dorianvogel/plasma-dev/powerdevil/daemon/powerdevilaction.cpp:101
#11 0x00007f6096e189e4 in PowerDevil::BundledActions::DimDisplay::setBrightnessHelper(int, int) (this=0x1ed71e0, screen=57, keyboard=0) at /home/dorianvogel/plasma-dev/powerdevil/daemon/actions/bundled/dimdisplay.cpp:88
#12 0x00007f6096e186f3 in PowerDevil::BundledActions::DimDisplay::onWakeupFromIdle() (this=0x1ed71e0) at /home/dorianvogel/plasma-dev/powerdevil/daemon/actions/bundled/dimdisplay.cpp:48
#13 0x00007f6096df809c in PowerDevil::Core::onResumingFromIdle() (this=0x1e5d3d0) at /home/dorianvogel/plasma-dev/powerdevil/daemon/powerdevilcore.cpp:797
#14 0x00007f6096e1eef3 in PowerDevil::Core::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (_o=0x1e5d3d0, _c=QMetaObject::InvokeMetaMethod, _id=24, _a=0x7ffcc3955bd0) at /home/dorianvogel/plasma-dev/powerdevil/build/daemon/powerdevilcore_automoc.dir/moc_powerdevilcore_SR3RJSZTCRXVIY.cpp:231
#15 0x00007f6091e1ad49 in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/libQt5Core.so.5
#16 0x00007f60966a08fb in  () at /usr/lib/libKF5IdleTime.so.5
#17 0x00007f6091e1ad49 in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/libQt5Core.so.5
#18 0x00007f607a2f6b8b in  () at /usr/lib/qt/plugins/kf5/org.kde.kidletime.platforms/KF5IdleTimeXcbPlugin0.so
#19 0x00007f607a2f706e in  () at /usr/lib/qt/plugins/kf5/org.kde.kidletime.platforms/KF5IdleTimeXcbPlugin0.so
#20 0x00007f6091dec55f in QAbstractEventDispatcher::filterNativeEvent(QByteArray const&, void*, long*) () at /usr/lib/libQt5Core.so.5
#21 0x00007f6085c9c974 in QXcbConnection::handleXcbEvent(xcb_generic_event_t*) () at /usr/lib/libQt5XcbQpa.so.5
#22 0x00007f6085c9d655 in QXcbConnection::processXcbEvents() () at /usr/lib/libQt5XcbQpa.so.5
#23 0x00007f6091e1bba9 in QObject::event(QEvent*) () at /usr/lib/libQt5Core.so.5
#24 0x00007f6091def2da in QCoreApplication::notify(QObject*, QEvent*) () at /usr/lib/libQt5Core.so.5
#25 0x00007f6091def440 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt5Core.so.5
#26 0x00007f6091df1bcd in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () at /usr/lib/libQt5Core.so.5
#27 0x00007f6091e43c43 in  () at /usr/lib/libQt5Core.so.5
#28 0x00007f608c0115a7 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#29 0x00007f608c011810 in  () at /usr/lib/libglib-2.0.so.0
#30 0x00007f608c0118bc in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#31 0x00007f6091e4404f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#32 0x00007f6091ded89a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#33 0x00007f6091df5de4 in QCoreApplication::exec() () at /usr/lib/libQt5Core.so.5
#34 0x00000000004066e6 in main(int, char**) (argc=1, argv=0x7ffcc39564a8) at /home/dorianvogel/plasma-dev/powerdevil/daemon/powerdevilapp.cpp:151
davidedmundson added inline comments.
daemon/backends/upower/ddcutilbrightness.cpp
149

you should check the return of this == 0

In case of error I would expect returnValue to not be set.

dvogel updated this revision to Diff 13395.Apr 13 2017, 7:58 PM
dvogel marked 6 inline comments as done.

Applied changes suggested by reviewers.
The only issue remaining is brightness restoration when waking up the monitor after shutting it down: we try to set brightness before the monitor is actually ready. Setting then fails, and the monitor wakes up with the last value it was dimmed to before shut down.
TODO: Make ddcutillib optional, for now not having ddcutil installed will most likely result in a crash.

dvogel updated this revision to Diff 13586.Apr 19 2017, 6:29 AM

Addition of a QTimer set by default to 1 sec to filter setBrightness calls: the actual DDC communication happens 1 sec after the last setBrightness() call.
This solves brightness flickering when scrolling quickly on the battery icon, and communication failure over DDC when waking the monitor from power-save mode (Dell U2212HM wakes up in less than 1 sec).

What's the current status of this?

The final conclusion was that I should figure out some cmake to allow building without ddccontrol. I kind of did it locally (tho it's a bit dirty).
The second big point is that powerdevil only supports one brightness controller at a time. The solution kbroulik suggested, is that powerdevil should be refactored in some sort of plugin architecture. However I personally do not have any idea at all how that should look, and most probably the time to do it.

My other idea in the last few days, would be to create a plasmoid, based on the (simple) qml app on my GitHub.

Cheers

daemon/backends/upower/ddcutilbrightness.cpp
73

Oh thanks for that ! I was kind of confused by the difference between stack and heap allocation.
Also I didn't know QVector::at() and QVector::operator[] were different. Unfortunately, the code is more complex using operator[].

dvogel updated this revision to Diff 14843.May 25 2017, 2:32 PM

Absence of ddcutil on the system is now handled:
the ddcbrightness object is still instantiated in powerdevilupowerbackend, however, this is a dummy object, returning isSupported()=FALSE, making powerdevilupowerbackend avoid using ddcutil.

The same brightness is applied to all ddc-capable displays this seems to be the most obvious choice (compared to setting only one monitor)
Multi-monitor not tested:

  • desktop + 2+ monitors: should work, as ddcutilbrightness supports multiple monitors
  • laptop + external monitor: not tested, when plugging a new monitor while the laptop LCD is already used for brightness ==> detection is just triggered when powerdevil starts, so should be fine; when booting with external monitor attached ==> ddcutil is the last possibility in ddcutilupowerbackend.cpp, the laptop LCD should always be controlled.

Looks good, I think I just have one comment about a leak, but the docs are confusing, so it's possible I'm wrong.

daemon/backends/upower/ddcutilbrightness.cpp
54

this needs a ddca_free_display_identifier I think?

134

yes it is needed.

From the docs:

  • It is the responsibility of the caller to free the returned struct
  • using ddca_free_parsed_capabilities().

but generally speaking with C APIs if you get something that takes a pointer to a pointer, it's creating a new object.

dvogel updated this revision to Diff 14854.May 26 2017, 11:42 AM

applied comments from d_ed
fixed brightness jumping to previous position after brightness change: see DDCutilBrightness::brightness()

davidedmundson accepted this revision.May 26 2017, 12:02 PM
This revision is now accepted and ready to land.May 26 2017, 12:02 PM
This revision was automatically updated to reflect the committed changes.
dvogel updated this revision to Diff 21592.EditedOct 30 2017, 9:43 PM

Reworked for simplification.

[EDIT] Please ignore this update of the diff, as discussed with bshah, I will create a new one.