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

Changeset View

Standalone View

View Options

daemon/backends/upower/ddcutilbrightness.cpp

  • This file was added.
1​#include <powerdevil_debug.h>
2​
3​#include "ddcutilbrightness.h"
4​
5​DDCutilBrightness::DDCutilBrightness()
6​{
7​ QList<DDCA_Display_Handle> m_displayHandleList = QList<DDCA_Display_Handle>();
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
8​ QList<DDCA_Display_Info> m_displayInfoList = QList<DDCA_Display_Info>();
9​
10​ //QList<DDCA_Capabilities> m_parsedCapabilities_perDisp = QList<DDCA_Capabilities>();
11​
12​ QList<QMap<QString, int>*> m_descrToVcp_perDisp = QList<QMap<QString, int>*>();
13​ QList<QMap<int, QMap<int, QString>*>*> m_vcpTovcpValueWithDescr_perDisp =
14​ QList<QMap<int, QMap<int, QString>*>*>();
15​}
16​
17​void DDCutilBrightness::detect(){
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() { ..
18​
19​ DDCA_Status rc;
20​ DDCA_Display_Identifier did;
21​ DDCA_Display_Ref dref;
22​ DDCA_Display_Handle dh = NULL; // initialize to avoid clang analyzer warning
broulikUnsubmitted
Done

Prefer nullptr

broulik: Prefer `nullptr`
23​
24​
25​
26​ qCWarning(POWERDEVIL)<<"\nCheck for monitors using ddca_get_displays()...\n";
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…
27​ // Inquire about detected monitors.
28​ DDCA_Display_Info_List * dlist = ddca_get_displays();
29​ qCWarning(POWERDEVIL)<<"ddca_get_displays() returned "<< dlist;
30​ qCWarning(POWERDEVIL)<<dlist->ct << "display(s) were detected";
31​ for(int iDisp=0;iDisp<dlist->ct;iDisp++)
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…
32​ {
33​
34​ qCWarning(POWERDEVIL)<<"\nCreate a Display Identifier for display"<<iDisp <<
35​ " : "<< dlist->info[iDisp].model_name;
36​
37​ m_displayInfoList.append(dlist->info[iDisp]);
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…
38​
39​ rc = ddca_create_dispno_display_identifier(iDisp+1, &did); // ddcutil uses 1 paded indexing for displays
40​
41​ char * did_repr = ddca_repr_display_identifier(did);
42​
43​ qCWarning(POWERDEVIL)<<"did="<<did_repr;
44​
45​ qCWarning(POWERDEVIL)<<"Create a display reference from the display identifier...";
46​ rc = ddca_create_display_ref(did, &dref);
47​
48​ if (rc != 0) {
49​ qCWarning(POWERDEVIL)<<"ddct_get_display_ref() returned "<<
50​ rc<< " ("<<ddca_status_code_name(rc) <<
51​ "): "<< ddca_status_code_desc(rc);
52​ }
53​ else {
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…
54​ char * dref_repr = ddca_repr_display_ref(dref);
55​ qCWarning(POWERDEVIL)<<"dref= " << dref_repr;
davidedmundsonUnsubmitted
Not Done

this needs a ddca_free_display_identifier I think?

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

return !m_displayHandleList.isEmpty();

broulik: `return !m_displayHandleList.isEmpty();`
140​
141​}
142​
143​long DDCutilBrightness::brightness() const
144​{
145​
146​ //FIXME: gets value for display 1
147​ Single_Vcp_Value *returnValue;
148​
149​ ddca_get_vcp_value(m_displayHandleList.at(0),
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…
150​ m_descrToVcp_perDisp.at(0)->value("Brightness"),
151​ NON_TABLE_VCP_VALUE, &returnValue);
152​
153​ return (long)returnValue->val.c.cur_val;
154​}
155​
156​long DDCutilBrightness::brightnessMax() const
157​{
158​ Single_Vcp_Value *returnValue;
159​
160​ ddca_get_vcp_value(m_displayHandleList.at(0),
161​ m_descrToVcp_perDisp.at(0)->value("Brightness"),
162​ NON_TABLE_VCP_VALUE, &returnValue);
163​
164​ return (long) returnValue->val.c.max_val;
165​
166​}
167​
168​void DDCutilBrightness::setBrightness(long value)
169​{
170​ qCDebug(POWERDEVIL)<<"[ddcutilbrightness] setting brightness to "<<value;
171​ ddca_set_continuous_vcp_value(m_displayHandleList.at(0),
172​ m_descrToVcp_perDisp.at(0)->value("Brightness"),
173​ (int)value);
174​}
175​