D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel updated this revision to Diff 21592.
dvogel added a comment.


  Reworked for simplification.

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5381?vs=14856=21592

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h

To: dvogel, broulik, davidedmundson
Cc: strobach, davidedmundson, plasma-devel, ZrenBot, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D5381: Add brightness control using ddcutil lib

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R122:a3a61317ddeb: Add brightness control using ddcutil lib 
(authored by dvogel, committed by davidedmundson).

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5381?vs=14854=14856

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  CMakeLists.txt
  cmake/FindDDCUtil.cmake
  daemon/backends/CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/powerdevilupowerbackend.h

To: dvogel, broulik, davidedmundson
Cc: strobach, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik, davidedmundson
Cc: strobach, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel updated this revision to Diff 14854.
dvogel added a comment.


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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5381?vs=14843=14854

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  CMakeLists.txt
  cmake/FindDDCUtil.cmake
  daemon/backends/CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/powerdevilupowerbackend.h

To: dvogel, broulik
Cc: strobach, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[David Edmundson]

davidedmundson added a comment.


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

INLINE COMMENTS

> ddcutilbrightness.cpp:54
> +
> +rc = ddca_create_dispno_display_identifier(iDisp+1, ); // 
> ddcutil uses 1 paded indexing for displays
> +

this needs a ddca_free_display_identifier I think?

> ddcutilbrightness.cpp:134
> +}
> +//is this needed ? are variables created in the method automatically 
> free'd ?
> +ddca_free_parsed_capabilities(parsedCapabilities);

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.

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik
Cc: strobach, davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel updated this revision to Diff 14843.
dvogel added a comment.


  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.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5381?vs=13586=14843

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  CMakeLists.txt
  cmake/FindDDCUtil.cmake
  daemon/backends/CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/powerdevilupowerbackend.h

To: dvogel, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel added a comment.


  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

INLINE COMMENTS

> broulik wrote in ddcutilbrightness.cpp: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

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[].

REPOSITORY
  R122 Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[David Edmundson]

davidedmundson added a comment.


  What's the current status of this?

REPOSITORY
  R122 Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik
Cc: davidedmundson, plasma-devel, ZrenBot, spstarr, progwolff, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart, lukas

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel updated this revision to Diff 13586.
dvogel added a comment.


  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).

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5381?vs=13395=13586

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  daemon/backends/CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/powerdevilupowerbackend.h

To: dvogel, broulik
Cc: davidedmundson, plasma-devel, spstarr, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel updated this revision to Diff 13395.
dvogel marked 6 inline comments as done.
dvogel added a comment.


  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.

REPOSITORY
  R122 Powerdevil

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D5381?vs=13291=13395

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  daemon/backends/CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/powerdevilupowerbackend.h

To: dvogel, broulik
Cc: davidedmundson, plasma-devel, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5381: Add brightness control using ddcutil lib

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> ddcutilbrightness.cpp:149
> +
> +ddca_get_vcp_value(m_displayHandleList.at(0),
> +   m_descrToVcp_perDisp.at(0)->value("Brightness"),

you should check the return of this == 0

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

REPOSITORY
  R122 Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik
Cc: davidedmundson, plasma-devel, progwolff, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel marked 9 inline comments as done.
dvogel added a comment.


  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  0x7f6090dc637d in read () at /usr/lib/libc.so.6
#1  0x7f608c055aa0 in  () at /usr/lib/libglib-2.0.so.0
#2  0x7f608c01126e in g_main_context_check () at 
/usr/lib/libglib-2.0.so.0
#3  0x7f608c011744 in  () at /usr/lib/libglib-2.0.so.0
#4  0x7f608c011b32 in g_main_loop_run () at /usr/lib/libglib-2.0.so.0
#5  0x7f6081190446 in  () at /usr/lib/libgio-2.0.so.0
#6  0x7f608c039175 in  () at /usr/lib/libglib-2.0.so.0
#7  0x7f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#8  0x7f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 4 (Thread 0x7f6080823700 (LWP 756)):
#0  0x7f6090dc637d in read () at /usr/lib/libc.so.6
#1  0x7f608c055aa0 in  () at /usr/lib/libglib-2.0.so.0
#2  0x7f608c01126e in g_main_context_check () at 
/usr/lib/libglib-2.0.so.0
#3  0x7f608c011744 in  () at /usr/lib/libglib-2.0.so.0
#4  0x7f608c0118bc in g_main_context_iteration () at 
/usr/lib/libglib-2.0.so.0
#5  0x7f608c011901 in  () at /usr/lib/libglib-2.0.so.0
#6  0x7f608c039175 in  () at /usr/lib/libglib-2.0.so.0
#7  0x7f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#8  0x7f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 3 (Thread 0x7f60824eb700 (LWP 746)):
#0  0x7f6090dc637d in read () at /usr/lib/libc.so.6
#1  0x7f608c055aa0 in  () at /usr/lib/libglib-2.0.so.0
#2  0x7f608c01126e in g_main_context_check () at 
/usr/lib/libglib-2.0.so.0
#3  0x7f608c011744 in  () at /usr/lib/libglib-2.0.so.0
#4  0x7f608c0118bc in g_main_context_iteration () at 
/usr/lib/libglib-2.0.so.0
#5  0x7f6091e4406b in 
QEventDispatcherGlib::processEvents(QFlags) () 
at /usr/lib/libQt5Core.so.5
#6  0x7f6091ded89a in 
QEventLoop::exec(QFlags) () at 
/usr/lib/libQt5Core.so.5
#7  0x7f6091c0fa73 in QThread::exec() () at /usr/lib/libQt5Core.so.5
#8  0x7f60925e4125 in  () at /usr/lib/libQt5DBus.so.5
#9  0x7f6091c146d8 in  () at /usr/lib/libQt5Core.so.5
#10 0x7f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#11 0x7f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 2 (Thread 0x7f608393f700 (LWP 743)):
#0  0x7f6090dca67d in poll () at /usr/lib/libc.so.6
#1  0x7f609194a8e0 in  () at /usr/lib/libxcb.so.1
#2  0x7f609194c679 in xcb_wait_for_event () at /usr/lib/libxcb.so.1
#3  0x7f6085c9b239 in  () at /usr/lib/libQt5XcbQpa.so.5
#4  0x7f6091c146d8 in  () at /usr/lib/libQt5Core.so.5
#5  0x7f608ff1d2e7 in start_thread () at /usr/lib/libpthread.so.0
#6  0x7f6090dd454f in clone () at /usr/lib/libc.so.6

Thread 1 (Thread 0x7f6097336840 (LWP 740)):
[KCrash Handler]
#6  0x7f607ab9a425 in DDCutilBrightness::brightness() const 
(this=0x1eb51c0) at 
/home/dorianvogel/plasma-dev/powerdevil/daemon/backends/upower/ddcutilbrightness.cpp:153
#7  0x7f607ab8b615 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  0x7f607ab8bd2b 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  0x7f6096e18b4f in 
PowerDevil::BundledActions::DimDisplay::triggerImpl(QMap 
const&) (this=0x1ed71e0, args=...) at 
/home/dorianvogel/plasma-dev/powerdevil/daemon/actions/bundled/dimdisplay.cpp:93
#10 0x7f6096dec3ce in PowerDevil::Action::trigger(QMap const&) (this=0x1ed71e0, args=...) at 
/home/dorianvogel/plasma-dev/powerdevil/daemon/powerdevilaction.cpp:101
#11 0x7f6096e189e4 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 0x7f6096e186f3 in 
PowerDevil::BundledActions::DimDisplay::onWakeupFromIdle() (this=0x1ed71e0) at 
/home/dorianvogel/plasma-dev/powerdevil/daemon/actions/bundled/dimdisplay.cpp:48
#13 0x7f6096df809c in PowerDevil::Core::onResumingFromIdle() 
(this=0x1e5d3d0) at 
/home/dorianvogel/plasma-dev/powerdevil/daemon/powerdevilcore.cpp:797
#14 0x7f6096e1eef3 

D5381: Add brightness control using ddcutil lib

[Kai Uwe Broulik]

broulik added a comment.


  Cool!

INLINE COMMENTS

> ddcutilbrightness.cpp:7
> +{
> +QList m_displayHandleList = 
> QList();
> +QList m_displayInfoList = QList();

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

> ddcutilbrightness.cpp:17
> +
> +void DDCutilBrightness::detect(){
> +

Coding style, opening brace for functions on next line:

  void DDCUtilBrightness:detect()
  {
  ...
  }

> ddcutilbrightness.cpp:22
> +DDCA_Display_Ref dref;
> +DDCA_Display_Handle dh = NULL;  // initialize to avoid clang analyzer 
> warning
> +

Prefer `nullptr`

> ddcutilbrightness.cpp:26
> +
> +qCWarning(POWERDEVIL)<<"\nCheck for monitors using 
> ddca_get_displays()...\n";
> +// Inquire about detected monitors.

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

> ddcutilbrightness.cpp:31
> +qCWarning(POWERDEVIL) +for(int iDisp=0;iDispct;iDisp++)
> +{

Coding style:

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

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

> ddcutilbrightness.cpp:37
> +
> +m_displayInfoList.append(dlist->info[iDisp]);
> +

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

> ddcutilbrightness.cpp:52-53
> +"): "<< ddca_status_code_desc(rc);
> +}
> +else {
> +char * dref_repr = ddca_repr_display_ref(dref);

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) {
  ...

> ddcutilbrightness.cpp:73
> +
> +m_descrToVcp_perDisp.append(new QMap);
> +m_vcpTovcpValueWithDescr_perDisp.append(new QMap QMap*>);

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

> ddcutilbrightness.cpp:139
> +{
> +return (m_displayHandleList.count()!=0);
> +

`return !m_displayHandleList.isEmpty();`

> ddcutilbrightness.h:1
> +
> +#ifndef DDCUTILBRIGHTNESS_H

Missing copyright header in this and other files

> ddcutilbrightness.h:2
> +
> +#ifndef DDCUTILBRIGHTNESS_H
> +#define DDCUTILBRIGHTNESS_H

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

> ddcutilbrightness.h:22
> +
> +QList m_displayHandleList;
> +QList m_displayInfoList;

Prefer `QVector` over `QList`

> powerdevilupowerbackend.cpp:217-218
> +brightnessJob->start();
> +}
> +else{
> +qCDebug(POWERDEVIL) << "Using DDCutillib";

Coding style:

  } else {

> powerdevilupowerbackend.cpp:382
>  QList controls = allControls.keys(controlType);
> -
> +
>  if (controls.isEmpty()) {

Whitespace

> powerdevilupowerbackend.cpp:413
>  int result = 0;
> -
> +
>  if (type == Screen) {

Whitespace

> powerdevilupowerbackend.cpp:716
>  {
> -m_brightnessControl->setBrightness(value.toInt());
> +if(m_brightnessControl->isSupported()){
> +m_brightnessControl->setBrightness(value.toInt());

Coding style

  if (m_brightnessControl->isSupported()) {

REPOSITORY
  R122 Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D5381

To: dvogel, broulik
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol

D5381: Add brightness control using ddcutil lib

[Dorian Vogel]

dvogel created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION 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.

REPOSITORY
  R122 Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D5381

AFFECTED FILES
  daemon/backends/CMakeLists.txt
  daemon/backends/upower/ddcutilbrightness.cpp
  daemon/backends/upower/ddcutilbrightness.h
  daemon/backends/upower/powerdevilupowerbackend.cpp
  daemon/backends/upower/powerdevilupowerbackend.h

To: dvogel, broulik
Cc: plasma-devel, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas, apol