Differential D5381 Diff 14843 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(300);
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​ DDCA_Display_Identifier did;
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​ DDCA_Display_Ref dref;
40​ DDCA_Display_Handle dh = nullptr; // initialize to avoid clang analyzer warning
41​
42​ qCDebug(POWERDEVIL) << "Check for monitors using ddca_get_displays()...";
43​ // Inquire about detected monitors.
44​ DDCA_Display_Info_List * dlist = ddca_get_display_info_list();
45​ qCDebug(POWERDEVIL) << "ddca_get_display_info_list() returned "<< dlist;
46​ qCInfo(POWERDEVIL) << "[DDCutilBrightness] " << dlist->ct << "display(s) were detected";
47​
48​ for (int iDisp=0;iDisp<dlist->ct;iDisp++) {
49​ qCDebug(POWERDEVIL) << "Create a Display Identifier for display"<<iDisp <<
50​ " : "<< dlist->info[iDisp].model_name;
51​
52​ m_displayInfoList.append(dlist->info[iDisp]);
53​
54​ rc = ddca_create_dispno_display_identifier(iDisp+1, &did); // ddcutil uses 1 paded indexing for displays
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…
davidedmundsonUnsubmitted
Not Done

this needs a ddca_free_display_identifier I think?

davidedmundson: this needs a ddca_free_display_identifier I think?
55​
56​ char * did_repr = ddca_did_repr(did);
57​
58​ qCDebug(POWERDEVIL) << "did="<<did_repr;
59​
60​ qCDebug(POWERDEVIL) << "Create a display reference from the display identifier...";
61​ rc = ddca_get_display_ref(did, &dref);
62​
63​ if (rc != 0) {
64​ qCWarning(POWERDEVIL) << "[DDCutilBrightness]: ddct_get_display_ref() returned "<<
65​ rc<< " ("<<ddca_rc_name(rc) <<
66​ "): "<< ddca_rc_name(rc);
67​ continue;
68​ }
69​
70​ char * dref_repr = ddca_dref_repr(dref);
71​ qCDebug(POWERDEVIL) << "dref= " << dref_repr;
72​
73​ qCDebug(POWERDEVIL) << "Open the display reference, creating a display handle...";
74​ rc = ddca_open_display(dref, &dh);
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​ if (rc != 0) {
76​ qCWarning(POWERDEVIL) << "[DDCutilBrightness]: ddct_open_display"<< rc;
77​ continue;
78​ }
79​
80​ qCDebug(POWERDEVIL) << "adding handle to list";
81​ m_displayHandleList.append(dh);
82​ qCDebug(POWERDEVIL) << "handles nb: "<<m_displayHandleList.count();
83​ char* capabilityString;
84​ DDCA_Capabilities* parsedCapabilities;
85​ ddca_get_capabilities_string(dh, &capabilityString);
86​ ddca_parse_capabilities_string(capabilityString, &parsedCapabilities);
87​ qCInfo(POWERDEVIL) << "[DDCutilBrightness] " << parsedCapabilities->vcp_code_ct << "capabilities parsed";
88​
89​
90​ m_descrToVcp_perDisp.append(QMap<QString, int>());
91​ m_vcpTovcpValueWithDescr_perDisp.append(QMap<int, QMap<int, QString> >() );
92​ //fill the feature description to vcp LUT
93​
94​ DDCA_Version_Feature_Info* featureInfo;
95​ for (int iVcp=0;iVcp<parsedCapabilities->vcp_code_ct;iVcp++) {
96​
97​ int vcpCode=parsedCapabilities->vcp_codes[iVcp].feature_code;
98​
99​ m_vcpTovcpValueWithDescr_perDisp[iDisp].insert(vcpCode, QMap<int, QString>());
100​
101​ m_descrToVcp_perDisp[iDisp].insert(
102​ QString(ddca_get_feature_name(vcpCode)), vcpCode);
103​
104​
105​ ddca_get_feature_info_by_display(m_displayHandleList.at(iDisp), vcpCode, &featureInfo);
106​ if (featureInfo == nullptr) {
107​ continue;
108​ }
109​ qCDebug(POWERDEVIL) << featureInfo->feature_code<<":"<<featureInfo->desc;
110​ if ((featureInfo->feature_flags & DDCA_SIMPLE_NC) != DDCA_SIMPLE_NC) {
111​ continue;
112​ }
113​ for (int iVcpVal=0;featureInfo->sl_values[iVcpVal].value_code!=0;++iVcpVal) {
114​
115​ qCDebug(POWERDEVIL) << "\t"<<featureInfo->sl_values[iVcpVal].value_code
116​ <<":"<< featureInfo->sl_values[iVcpVal].value_name;
117​
118​ bool thisVcpValIsSupported=false;
119​
120​ for (int iSupportedVcpVal=0; iSupportedVcpVal<parsedCapabilities->vcp_codes[iVcp].value_ct; iSupportedVcpVal++) {
121​ if(parsedCapabilities->vcp_codes[iVcp].values[iSupportedVcpVal]
122​ ==featureInfo->sl_values[iVcpVal].value_code) {
123​ thisVcpValIsSupported=true;
124​ }
125​ }
126​
127​ if (thisVcpValIsSupported) {
128​ (m_vcpTovcpValueWithDescr_perDisp[iDisp])[vcpCode].insert(
129​ featureInfo->sl_values[iVcpVal].value_code,
130​ featureInfo->sl_values[iVcpVal].value_name);
131​ }
132​ }
133​ }
134​ //is this needed ? are variables created in the method automatically free'd ?
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…
135​ ddca_free_parsed_capabilities(parsedCapabilities);
136​ }
137​#endif
138​}
139​
140​
broulikUnsubmitted
Done

return !m_displayHandleList.isEmpty();

broulik: `return !m_displayHandleList.isEmpty();`
141​bool DDCutilBrightness::isSupported() const
142​{
143​#ifndef WITH_DDCUTIL
144​ return false;
145​#else
146​ return !m_displayHandleList.isEmpty();
147​#endif
148​}
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​long DDCutilBrightness::brightness()
152​{
153​#ifdef WITH_DDCUTIL
154​ //FIXME: gets value for display 1
155​ DDCA_Status rc;
156​ DDCA_Single_Vcp_Value *returnValue;
157​
158​ rc = ddca_get_vcp_value(m_displayHandleList.at(0),
159​ m_descrToVcp_perDisp.at(0).value("Brightness"),
160​ DDCA_NON_TABLE_VCP_VALUE, &returnValue);
161​ qCDebug(POWERDEVIL) << "[DDCutilBrightness::brightness]: ddca_get_vcp_value returned" << rc;
162​
163​ //check rc to prevent crash on wake from idle and the monitor has gone to powersave mode
164​ if (rc == 0) {
165​ m_lastBrightnessKnown = (long)returnValue->val.c.cur_val;
166​ }
167​
168​ return m_lastBrightnessKnown;
169​#else
170​ return 0;
171​#endif
172​}
173​
174​long DDCutilBrightness::brightnessMax()
175​{
176​#ifdef WITH_DDCUTIL
177​ DDCA_Status rc;
178​ DDCA_Single_Vcp_Value *returnValue;
179​
180​ rc = ddca_get_vcp_value(m_displayHandleList.at(0),
181​ m_descrToVcp_perDisp.at(0).value("Brightness"),
182​ DDCA_NON_TABLE_VCP_VALUE, &returnValue);
183​ qCDebug(POWERDEVIL) << "[DDCutilBrightness::brightnessMax]: ddca_get_vcp_value returned" << rc;
184​
185​ //check rc to prevent crash on wake from idle and the monitor has gone to powersave mode
186​ if (rc == 0) {
187​ m_lastMaxBrightnessKnown = (long)returnValue->val.c.max_val;
188​ }
189​
190​ return m_lastMaxBrightnessKnown;
191​#else
192​ return 100.0;
193​#endif
194​}
195​
196​void DDCutilBrightness::setBrightness(long value)
197​{
198​ m_tmpCurrentBrightness = value;
199​ qCDebug(POWERDEVIL) << "[DDCutilBrightness]: saving brightness value: " << value;
200​ m_setBrightnessEventFilter.start();
201​}
202​
203​void DDCutilBrightness::setBrightnessAfterFilter()
204​{
205​#ifdef WITH_DDCUTIL
206​ DDCA_Status rc;
207​ for (int iDisp=0;iDisp<m_displayHandleList.count();iDisp++) { //FIXME: we set the same brightness to all monitors plugin architecture needed
208​ qCDebug(POWERDEVIL) << "[DDCutilBrightness]: setting brightness "<<m_tmpCurrentBrightness;
209​ rc = ddca_set_continuous_vcp_value(m_displayHandleList.at(iDisp),
210​ m_descrToVcp_perDisp.at(iDisp).value("Brightness"),
211​ (int)m_tmpCurrentBrightness);
212​
213​ if (rc != 0) {
214​ qCWarning(POWERDEVIL) << "[DDCutilBrightness::setBrightness] failed, trying to detect()";
215​ detect();
216​ } else {
217​ m_lastBrightnessKnown = m_tmpCurrentBrightness;
218​ }
219​ }
220​#else
221​ return;
222​#endif
223​}