Skip to content

Commit b73b629

Browse files
vanvughtvanvught
committed
Code quality: fix bugs, improve safety, refactor naming
Apply code quality improvements across three libraries: - display.h: Remove const from function parameters, rename variables for consistency (pData→data, nLine→line), fix naming conventions (SYMBOLS→kSymbols), fix const correctness issues - netif.cpp: Fix bug where gw.addr was used instead of gateway.addr parameter - remoteconfig.cpp: Add missing includes, refactor HandleList() with bounds checking and null-safety, rename variable p→print for clarity, remove unnecessary null check before delete, remove unused forward declaration
1 parent 492f0f5 commit b73b629

3 files changed

Lines changed: 76 additions & 45 deletions

File tree

‎lib-display/include/spi/display.h‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -105,17 +105,17 @@ class Display : public LcdDriver {
105105
#else
106106
printf("ST7789 ");
107107
#endif
108-
printf("(%u,%u)\n", rows_, cols_);
108+
printf("(%u,%u)\n", static_cast<unsigned>(rows_), static_cast<unsigned>(cols_));
109109
}
110110

111111
void Cls() { FillColour(kColorBackground); }
112112

113-
void SetCursorPos(const uint32_t nCol, const uint32_t row) {
113+
void SetCursorPos(uint32_t nCol, uint32_t row) {
114114
cursor_x_ = nCol * s_pFONT->kWidth;
115115
cursor_y_ = row * s_pFONT->kHeight;
116116
}
117117

118-
void PutChar(const int c) {
118+
void PutChar(int c) {
119119
DrawChar(cursor_x_, cursor_y_, static_cast<char>(c), s_pFONT, kColorBackground, kColorForeground);
120120

121121
cursor_x_ += s_pFONT->kWidth;
@@ -137,7 +137,7 @@ class Display : public LcdDriver {
137137
}
138138
}
139139

140-
void ClearLine(const uint32_t nLine) {
140+
void ClearLine(uint32_t nLine) {
141141
if (__builtin_expect((!(nLine <= rows_)), 0)) {
142142
return;
143143
}
@@ -151,7 +151,7 @@ class Display : public LcdDriver {
151151
SetCursorPos(0, (nLine - 1U));
152152
}
153153

154-
void TextLine(const uint32_t nLine, const char* pText, const uint32_t nLength) {
154+
void TextLine(uint32_t nLine, const char* pText, const uint32_t nLength) {
155155
if (__builtin_expect((!(nLine <= rows_)), 0)) {
156156
return;
157157
}
@@ -162,18 +162,18 @@ class Display : public LcdDriver {
162162

163163
void ClearEndOfLine() { clear_end_of_line_ = true; }
164164

165-
void Text(const char* pData, uint32_t nLength) {
166-
if (nLength > cols_) {
167-
nLength = cols_;
165+
void Text(const char* data, uint32_t length) {
166+
if (length > cols_) {
167+
length = cols_;
168168
}
169169

170-
for (uint32_t i = 0; i < nLength; i++) {
171-
PutChar(pData[i]);
170+
for (uint32_t i = 0; i < length; i++) {
171+
PutChar(data[i]);
172172
}
173173
}
174174

175-
int Write(const uint32_t nLine, const char* pText) {
176-
const auto* p = pText;
175+
int Write(uint32_t line, const char* text) {
176+
const auto* p = text;
177177
int nCount = 0;
178178

179179
const auto columns = static_cast<int>(cols_);
@@ -182,28 +182,28 @@ class Display : public LcdDriver {
182182
++p;
183183
}
184184

185-
TextLine(nLine, pText, static_cast<uint8_t>(nCount));
185+
TextLine(line, text, static_cast<uint8_t>(nCount));
186186

187187
return nCount;
188188
}
189189

190-
int Printf(const uint8_t nLine, const char* format, ...) {
190+
int Printf(uint8_t line, const char* format, ...) {
191191
char buffer[32];
192192

193193
va_list arp;
194194

195195
va_start(arp, format);
196196

197-
auto i = vsnprintf(buffer, sizeof(buffer) / sizeof(buffer[0]), format, arp);
197+
const auto kSize = vsnprintf(buffer, sizeof(buffer) / sizeof(buffer[0]), format, arp);
198198

199199
va_end(arp);
200200

201-
TextLine(nLine, buffer, static_cast<uint16_t>(i));
201+
TextLine(line, buffer, static_cast<uint16_t>(kSize));
202202

203-
return i;
203+
return kSize;
204204
}
205205

206-
void TextStatus(const char* pText) {
206+
void TextStatus(const char* text) {
207207
SetCursorPos(0, static_cast<uint8_t>(rows_ - 1));
208208

209209
for (uint32_t i = 0; i < (cols_ - 1); i++) {
@@ -212,7 +212,7 @@ class Display : public LcdDriver {
212212

213213
SetCursorPos(0, static_cast<uint8_t>(rows_ - 1));
214214

215-
Write(rows_, pText);
215+
Write(rows_, text);
216216
}
217217

218218
void TextStatus(const char* text, ansi::Colours::Colour colour) {
@@ -221,20 +221,20 @@ class Display : public LcdDriver {
221221
}
222222

223223
void Progress() {
224-
static constexpr char SYMBOLS[] = {'/', '-', '\\', '|'};
224+
static constexpr char kSymbols[] = {'/', '-', '\\', '|'};
225225
static uint32_t nSymbolsIndex;
226226

227227
SetCursorPos(GetColumns() - 1U, GetRows() - 1U);
228-
PutChar(SYMBOLS[nSymbolsIndex++]);
228+
PutChar(kSymbols[nSymbolsIndex++]);
229229

230-
if (nSymbolsIndex >= sizeof(SYMBOLS)) {
230+
if (nSymbolsIndex >= sizeof(kSymbols)) {
231231
nSymbolsIndex = 0;
232232
}
233233
}
234234

235-
void SetContrast(const uint8_t nContrast) { SetBackLight(nContrast); }
235+
void SetContrast(uint8_t nContrast) { SetBackLight(nContrast); }
236236

237-
void SetSleep(const bool bSleep) {
237+
void SetSleep(bool bSleep) {
238238
is_sleep_ = bSleep;
239239

240240
EnableSleep(bSleep);
@@ -282,7 +282,6 @@ class Display : public LcdDriver {
282282
private:
283283
void SetSleepTimer(const bool bActive);
284284

285-
private:
286285
uint32_t cols_;
287286
uint32_t rows_;
288287
uint32_t sleep_timeout_{1000U * 60U * display::Defaults::kSleepTimeout};

‎lib-network/src/core/netif.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ void SetGw(network::ip4_addr_t gateway) {
183183

184184
void SetAddr(network::ip4_addr_t ipaddr, network::ip4_addr_t netmask, network::ip4_addr_t gateway) {
185185
DEBUG_ENTRY();
186-
DEBUG_PRINTF(IPSTR " " IPSTR " " IPSTR, IP2STR(ipaddr.addr), IP2STR(netmask.addr), IP2STR(gw.addr));
186+
DEBUG_PRINTF(IPSTR " " IPSTR " " IPSTR, IP2STR(ipaddr.addr), IP2STR(netmask.addr), IP2STR(gateway.addr));
187187

188188
auto change_reason = NetifReason::kNone;
189189
netif_ext_callback_args_t cb_args;

‎lib-remoteconfig/src/remoteconfig.cpp‎

Lines changed: 51 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
* THE SOFTWARE.
2424
*/
2525

26+
2627
#if defined(DEBUG_REMOTECONFIG)
2728
#undef NDEBUG
2829
#endif
@@ -31,6 +32,7 @@
3132
#include <cstdio>
3233
#include <cstring>
3334
#include <cassert>
35+
#include <algorithm>
3436

3537
#include "remoteconfig.h"
3638
#include "firmware/firmwareversion.h"
@@ -41,6 +43,7 @@
4143
#include "dmxnode_nodetype.h"
4244
#include "json/remoteconfigparams.h"
4345
#endif
46+
#include "common/utils/utils_array.h"
4447
#include "display.h"
4548
#include "configstore.h"
4649
#include "firmware/debug/debug_dump.h"
@@ -126,9 +129,9 @@ RemoteConfig::~RemoteConfig() {
126129

127130
#if !defined(CONFIG_REMOTECONFIG_MINIMUM)
128131
#if defined(ENABLE_HTTPD)
129-
if (http_daemon_ != nullptr) {
130-
delete http_daemon_;
131-
}
132+
133+
delete http_daemon_;
134+
132135
#endif
133136
network::apps::mdns::ServiceRecordDelete(network::apps::mdns::Services::kConfig);
134137
#endif
@@ -235,7 +238,6 @@ void RemoteConfig::HandleUptime() {
235238

236239
const auto kUptime = timing::UpTime();
237240
const auto kLength = snprintf(udp_buffer_, remoteconfig::udp::kBufferSize - 1, "uptime: %us\n", static_cast<unsigned int>(kUptime));
238-
239241
network::udp::Send(handle_, reinterpret_cast<const uint8_t*>(udp_buffer_), static_cast<uint32_t>(kLength), ip_from_, remoteconfig::udp::kPort);
240242

241243
DEBUG_EXIT();
@@ -245,8 +247,8 @@ void RemoteConfig::HandleUptime() {
245247
void RemoteConfig::HandleVersion() {
246248
DEBUG_ENTRY();
247249

248-
const auto* p = FirmwareVersion::Get()->GetPrint();
249-
const auto kLength = snprintf(udp_buffer_, remoteconfig::udp::kBufferSize - 1, "version:%s\n", p);
250+
const auto* print = FirmwareVersion::Get()->GetPrint();
251+
const auto kLength = snprintf(udp_buffer_, remoteconfig::udp::kBufferSize - 1, "version:%s\n", print);
250252
network::udp::Send(handle_, reinterpret_cast<const uint8_t*>(udp_buffer_), static_cast<uint32_t>(kLength), ip_from_, remoteconfig::udp::kPort);
251253

252254
DEBUG_EXIT();
@@ -256,28 +258,62 @@ void RemoteConfig::HandleList() {
256258
DEBUG_ENTRY();
257259

258260
constexpr auto kCmdLength = kGet[static_cast<uint32_t>(remoteconfig::udp::get::Command::kList)].kLength;
261+
259262
auto* list_response = &udp_buffer_[kCmdLength + 2U];
260-
const auto kListResponseBufferLength = remoteconfig::udp::kBufferSize - (kCmdLength + 2U);
261-
int32_t list_length;
263+
264+
constexpr auto kListResponseBufferLength = remoteconfig::udp::kBufferSize - (kCmdLength + 2U);
262265

263266
uint8_t display_name[common::store::remoteconfig::kDisplayNameLength];
267+
264268
ConfigStore::Instance().RemoteConfigCopyArray(display_name, &common::store::RemoteConfig::display_name);
265-
display_name[common::store::remoteconfig::kDisplayNameLength - 1] = '\0'; // Just to be safe
269+
270+
display_name[common::store::remoteconfig::kDisplayNameLength - 1U] = '\0';
266271

267272
#if !defined(CONFIG_REMOTECONFIG_MINIMUM)
268-
const auto* const kNodeTypeName = dmxnode::GetNodeType(dmxnode::kNodeType);
273+
const char* node_type_name = dmxnode::GetNodeType(dmxnode::kNodeType);
274+
275+
if (node_type_name == nullptr) {
276+
node_type_name = "Unknown";
277+
}
269278
#else
270-
const auto* const kNodeTypeName = "Bootloader TFTP";
279+
constexpr const char* node_type_name = "Bootloader TFTP";
271280
#endif
272281

282+
const auto kOutputIndex = static_cast<uint32_t>(output_);
283+
assert(kOutputIndex < common::ArraySize(kOutput));
284+
285+
const char* output_name = kOutput[kOutputIndex];
286+
287+
if (output_name == nullptr) {
288+
output_name = "Unknown";
289+
}
290+
291+
int list_length;
292+
273293
if (display_name[0] != '\0') {
274-
list_length = snprintf(list_response, kListResponseBufferLength - 1, "" IPSTR ",%s,%s,%u,%s\n", IP2STR(network::GetPrimaryIp()), kNodeTypeName, kOutput[static_cast<uint32_t>(output_)], static_cast<unsigned int>(active_outputs_),
275-
display_name);
294+
list_length =
295+
snprintf(list_response, kListResponseBufferLength, IPSTR ",%s,%s,%u,%s\n",
296+
IP2STR(network::GetPrimaryIp()),
297+
node_type_name,
298+
output_name,
299+
static_cast<unsigned>(active_outputs_),
300+
reinterpret_cast<const char*>(display_name));
276301
} else {
277-
list_length = snprintf(list_response, kListResponseBufferLength - 1, "" IPSTR ",%s,%s,%u\n", IP2STR(network::GetPrimaryIp()), kNodeTypeName, kOutput[static_cast<uint32_t>(output_)], static_cast<unsigned int>(active_outputs_));
302+
list_length = snprintf(list_response, kListResponseBufferLength, IPSTR ",%s,%s,%u\n",
303+
IP2STR(network::GetPrimaryIp()),
304+
node_type_name,
305+
output_name,
306+
static_cast<unsigned>(active_outputs_));
278307
}
279308

280-
network::udp::Send(handle_, reinterpret_cast<const uint8_t*>(list_response), static_cast<uint32_t>(list_length), ip_from_, remoteconfig::udp::kPort);
309+
if (list_length < 0) {
310+
DEBUG_EXIT();
311+
return;
312+
}
313+
314+
const auto kBytesToSend = static_cast<uint32_t>(std::min<size_t>(static_cast<size_t>(list_length), kListResponseBufferLength - 1U));
315+
316+
network::udp::Send(handle_, reinterpret_cast<const uint8_t*>(list_response), kBytesToSend, ip_from_, remoteconfig::udp::kPort);
281317

282318
DEBUG_EXIT();
283319
}
@@ -354,10 +390,6 @@ void RemoteConfig::HandleTftpGet() {
354390
DEBUG_EXIT();
355391
}
356392

357-
namespace board {
358-
bool Reboot();
359-
} // namespace board
360-
361393
void RemoteConfig::HandleReboot() {
362394
DEBUG_ENTRY();
363395
board::Reboot();

0 commit comments

Comments
 (0)