Differential D5381 Diff 14854 daemon/backends/upower/ddcutilbrightness.cpp

Changeset View

Standalone View

View Options

daemon/backends/upower/ddcutilbrightness.cpp

  • This file was added.
1​/* This file is part of the KDE project
2​ * Copyright (C) 2017 Dorian Vogel <dorianvogel@gmail.com>
3​ *
4​ * This library is free software; you can redistribute it and/or
5​ * modify it under the terms of the GNU Library General Public
6​ * License version 2 as published by the Free Software Foundation.
7​ *
8​ * This library is distributed in the hope that it will be useful,
broulikUnsubmitted
Done

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

broulik: No need to explicitly initialize these, the constructor could just remain empty
9​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
10​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
11​ * Library General Public License for more details.
12​ *
13​ * You should have received a copy of the GNU Library General Public License
14​ * along with this library; see the file COPYING.LIB. If not, write to
15​ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
16​ * Boston, MA 02110-1301, USA.
17​ *
18​ */
broulikUnsubmitted
Done

Coding style, opening brace for functions on next line:

void DDCUtilBrightness:detect()
{
...
}
broulik: Coding style, opening brace for functions on next line: ``` void DDCUtilBrightness:detect() { ..
19​
20​#include <powerdevil_debug.h>
21​#include "ddcutilbrightness.h"
22​
23​
broulikUnsubmitted
Done

Prefer nullptr

broulik: Prefer `nullptr`
24​DDCutilBrightness::DDCutilBrightness()
25​{
26​ m_setBrightnessEventFilter.setInterval(100);
27​ m_setBrightnessEventFilter.setSingleShot(true);
broulikUnsubmitted
Done

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

broulik: Coding style: ``` qCWarning(POWERDEVIL) << "..."; ``` Also, no need for the line breaks? Also…
28​ connect(&m_setBrightnessEventFilter, &QTimer::timeout, this, &DDCutilBrightness::setBrightnessAfterFilter);
29​}
30​
31​void DDCutilBrightness::detect()
32​{
broulikUnsubmitted
Done

Coding style:

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

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

broulik: Coding style: ``` for (int i = 0; i < ...; ++i) { ``` (brace on the same line for everything…
33​#ifndef WITH_DDCUTIL
34​ qCInfo(POWERDEVIL) << "[DDCutilBrightness] compiled without DDC/CI support";
35​ return;
36​#else
37​ DDCA_Status rc;
38​
broulikUnsubmitted
Not Done

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

broulik: Perhaps `clear()` the list at the beginning of this method? Also, use `reserve()` if you…
39​ qCDebug(POWERDEVIL) << "Check for monitors using ddca_get_displays()...";
40​ // Inquire about detected monitors.
41​ DDCA_Display_Info_List * dlist = ddca_get_display_info_list();
42​ qCDebug(POWERDEVIL) << "ddca_get_display_info_list() returned "<< dlist;
43​ qCInfo(POWERDEVIL) << "[DDCutilBrightness] " << dlist->ct << "display(s) were detected";
44​
45​ for (int iDisp=0;iDisp<dlist->ct;iDisp++) {
46​ DDCA_Display_Identifier did;
47​ DDCA_Display_Ref dref;
48​ DDCA_Display_Handle dh = nullptr; // initialize to avoid clang analyzer warning
49​
50​ qCDebug(POWERDEVIL) << "Create a Display Identifier for display"<<iDisp <<
51​ " : "<< dlist->info[iDisp].model_name;
52​
53​ m_displayInfoList.append(dlist->info[iDisp]);
54​
broulikUnsubmitted
Done

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) {
        ...
broulik: Coding style, braces on the same line: ```} else {``` Also, I would prefer if you returned (and…
55​ rc = ddca_create_dispno_display_identifier(iDisp+1, &did); // ddcutil uses 1 paded indexing for displays
davidedmundsonUnsubmitted
Not Done

this needs a ddca_free_display_identifier I think?

davidedmundson: this needs a ddca_free_display_identifier I think?
56​
57​ char * did_repr = ddca_did_repr(did);
58​
59​ qCDebug(POWERDEVIL) << "did="<<did_repr;
60​
61​ qCDebug(POWERDEVIL) << "Create a display reference from the display identifier...";
62​ rc = ddca_get_display_ref(did, &dref);
63​
64​ if (rc != 0) {
65​ qCWarning(POWERDEVIL) << "[DDCutilBrightness]: ddct_get_display_ref() returned "<<
66​ rc<< " ("<<ddca_rc_name(rc) <<
67​ "): "<< ddca_rc_name(rc);
68​ continue;
69​ }
70​
71​ char * dref_repr = ddca_dref_repr(dref);
72​ qCDebug(POWERDEVIL) << "dref= " << dref_repr;
73​
74​ qCDebug(POWERDEVIL) << "Open the display reference, creating a display handle...";
broulikUnsubmitted
Done

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

broulik: You don't seem to be cleaning up those containers in the destructor (there is none). Also, I…
dvogelAuthorUnsubmitted
Not Done

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: Oh thanks for that ! I was kind of confused by the difference between stack and heap allocation.
75​ rc = ddca_open_display(dref, &dh);
76​ if (rc != 0) {
77​ qCWarning(POWERDEVIL) << "[DDCutilBrightness]: ddct_open_display"<< rc;
78​ continue;
79​ }
80​
81​ qCDebug(POWERDEVIL) << "adding handle to list";
82​ m_displayHandleList.append(dh);
83​ qCDebug(POWERDEVIL) << "handles nb: "<<m_displayHandleList.count();
84​ char* capabilityString;
85​ DDCA_Capabilities* parsedCapabilities;
86​ ddca_get_capabilities_string(dh, &capabilityString);
87​ ddca_parse_capabilities_string(capabilityString, &parsedCapabilities);
88​ qCInfo(POWERDEVIL) << "[DDCutilBrightness] " << parsedCapabilities->vcp_code_ct << "capabilities parsed";
89​
90​
91​ m_descrToVcp_perDisp.append(QMap<QString, int>());
92​ m_vcpTovcpValueWithDescr_perDisp.append(QMap<int, QMap<int, QString> >() );
93​ //fill the feature description to vcp LUT
94​
95​ DDCA_Version_Feature_Info* featureInfo;
96​ for (int iVcp=0;iVcp<parsedCapabilities->vcp_code_ct;iVcp++) {
97​
98​ int vcpCode=parsedCapabilities->vcp_codes[iVcp].feature_code;
99​
100​ m_vcpTovcpValueWithDescr_perDisp[iDisp].insert(vcpCode, QMap<int, QString>());
101​
102​ m_descrToVcp_perDisp[iDisp].insert(
103​ QString(ddca_get_feature_name(vcpCode)), vcpCode);
104​
105​
106​ ddca_get_feature_info_by_display(m_displayHandleList.at(iDisp), vcpCode, &featureInfo);
107​ if (featureInfo == nullptr) {
108​ continue;
109​ }
110​ qCDebug(POWERDEVIL) << featureInfo->feature_code<<":"<<featureInfo->desc;
111​ if ((featureInfo->feature_flags & DDCA_SIMPLE_NC) != DDCA_SIMPLE_NC) {
112​ continue;
113​ }
114​ for (int iVcpVal=0;featureInfo->sl_values[iVcpVal].value_code!=0;++iVcpVal) {
115​
116​ qCDebug(POWERDEVIL) << "\t"<<featureInfo->sl_values[iVcpVal].value_code
117​ <<":"<< featureInfo->sl_values[iVcpVal].value_name;
118​
119​ bool thisVcpValIsSupported=false;
120​
121​ for (int iSupportedVcpVal=0; iSupportedVcpVal<parsedCapabilities->vcp_codes[iVcp].value_ct; iSupportedVcpVal++) {
122​ if(parsedCapabilities->vcp_codes[iVcp].values[iSupportedVcpVal]
123​ ==featureInfo->sl_values[iVcpVal].value_code) {
124​ thisVcpValIsSupported=true;
125​ }
126​ }
127​
128​ if (thisVcpValIsSupported) {
129​ (m_vcpTovcpValueWithDescr_perDisp[iDisp])[vcpCode].insert(
130​ featureInfo->sl_values[iVcpVal].value_code,
131​ featureInfo->sl_values[iVcpVal].value_name);
132​ }
133​ }
134​ }
135​ ddca_free_display_identifier(did);
davidedmundsonUnsubmitted
Not Done

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.

davidedmundson: yes it is needed. From the docs: * It is the responsibility of the caller to free the…
136​ ddca_free_parsed_capabilities(parsedCapabilities);
137​ }
138​#endif
139​}
140​
broulikUnsubmitted
Done

return !m_displayHandleList.isEmpty();

broulik: `return !m_displayHandleList.isEmpty();`
141​
142​bool DDCutilBrightness::isSupported() const
143​{
144​#ifndef WITH_DDCUTIL
145​ return false;
146​#else
147​ return !m_displayHandleList.isEmpty();
148​#endif
149​}
150​
davidedmundsonUnsubmitted
Done

you should check the return of this == 0

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

davidedmundson: you should check the return of this == 0 In case of error I would expect returnValue to not be…
151​
152​long DDCutilBrightness::brightness()
153​{
154​#ifdef WITH_DDCUTIL
155​ //we check wether the timer is running, this means we received new values but did not send them yet to the monitor
156​ //not checking that results in the brightness slider jump to the previous vqlue when changing.
157​ if(m_setBrightnessEventFilter.isActive()) {
158​ m_lastBrightnessKnown = m_tmpCurrentBrightness;
159​ }
160​ else { //FIXME: gets value for display 1
161​ DDCA_Status rc;
162​ DDCA_Single_Vcp_Value *returnValue;
163​
164​ rc = ddca_get_vcp_value(m_displayHandleList.at(0),
165​ m_descrToVcp_perDisp.at(0).value("Brightness"),
166​ DDCA_NON_TABLE_VCP_VALUE, &returnValue);
167​ qCDebug(POWERDEVIL) << "[DDCutilBrightness::brightness]: ddca_get_vcp_value returned" << rc;
168​
169​ //check rc to prevent crash on wake from idle and the monitor has gone to powersave mode
170​ if (rc == 0) {
171​ m_lastBrightnessKnown = (long)returnValue->val.c.cur_val;
172​ }
173​ }
174​ return m_lastBrightnessKnown;
175​#else
176​ return 0;
177​#endif
178​}
179​
180​long DDCutilBrightness::brightnessMax()
181​{
182​#ifdef WITH_DDCUTIL
183​ DDCA_Status rc;
184​ DDCA_Single_Vcp_Value *returnValue;
185​
186​ rc = ddca_get_vcp_value(m_displayHandleList.at(0),
187​ m_descrToVcp_perDisp.at(0).value("Brightness"),
188​ DDCA_NON_TABLE_VCP_VALUE, &returnValue);
189​ qCDebug(POWERDEVIL) << "[DDCutilBrightness::brightnessMax]: ddca_get_vcp_value returned" << rc;
190​
191​ //check rc to prevent crash on wake from idle and the monitor has gone to powersave mode
192​ if (rc == 0) {
193​ m_lastMaxBrightnessKnown = (long)returnValue->val.c.max_val;
194​ }
195​
196​ return m_lastMaxBrightnessKnown;
197​#else
198​ return 100.0;
199​#endif
200​}
201​
202​void DDCutilBrightness::setBrightness(long value)
203​{
204​ m_tmpCurrentBrightness = value;
205​ qCDebug(POWERDEVIL) << "[DDCutilBrightness]: saving brightness value: " << value;
206​ m_setBrightnessEventFilter.start();
207​}
208​
209​void DDCutilBrightness::setBrightnessAfterFilter()
210​{
211​#ifdef WITH_DDCUTIL
212​ DDCA_Status rc;
213​ for (int iDisp=0;iDisp<m_displayHandleList.count();iDisp++) { //FIXME: we set the same brightness to all monitors, plugin architecture needed
214​ qCDebug(POWERDEVIL) << "[DDCutilBrightness]: setting brightness "<<m_tmpCurrentBrightness;
215​ rc = ddca_set_continuous_vcp_value(m_displayHandleList.at(iDisp),
216​ m_descrToVcp_perDisp.at(iDisp).value("Brightness"),
217​ (int)m_tmpCurrentBrightness);
218​
219​ if (rc != 0) {
220​ qCWarning(POWERDEVIL) << "[DDCutilBrightness::setBrightness] failed, trying to detect()";
221​ detect();
222​ } else {
223​ m_lastBrightnessKnown = m_tmpCurrentBrightness;
224​ }
225​ }
226​#else
227​ return;
228​#endif
229​}