src/communication/modbusmaster.cpp (1)

240-248: Consider passing QString by const reference.

Both logDebug and logError take QString by value. Since they only read the string, const QString& avoids unnecessary copies.

♻️ Suggested change

In the header (modbusmaster.h):

-    void logDebug(QString msg);
-    void logError(QString msg);
+    void logDebug(const QString& msg);
+    void logError(const QString& msg);

In the implementation:

-void ModbusMaster::logDebug(QString msg)
+void ModbusMaster::logDebug(const QString& msg)
 {
     qCDebug(scopeCommConnection) << QString("[Conn %0] %1").arg(_connectionId + 1).arg(msg);
 }

-void ModbusMaster::logError(QString msg)
+void ModbusMaster::logError(const QString& msg)
 {
     qCWarning(scopeCommConnection) << QString("[Conn %0] %1").arg(_connectionId + 1).arg(msg);
 }

src/util/scopelogging.cpp (1)

34-34: Commented-out code.

This commented-out filter rule could be removed if it's no longer needed, or given a short comment explaining when it might be re-enabled.

src/util/scopelogging.cpp (1)

34-34: Commented-out code left behind.

If this filter rule is no longer needed, consider removing it rather than leaving it commented out. If it's kept as a developer reference, a short explanatory comment would help.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/util/scopelogging.cpp` at line 34, Remove the leftover commented-out call
to QLoggingCategory::setFilterRules("*=false\n") or add a concise explanatory
comment if it must remain for developers; locate the occurrence of the commented
line mentioning QLoggingCategory::setFilterRules in scopelogging.cpp and either
delete that commented line or replace it with a short note explaining why the
filter rule is kept commented (when/where to enable it) so the intent is clear.

src/communication/modbusmaster.cpp (1)

58-58: String formatting overhead when debug logging is disabled.

dumpToString() and string concatenation at each call site are evaluated unconditionally, even when the scopeCommConnection debug category is disabled. On a hot polling path this is wasteful. Consider guarding with an early check:

void ModbusMaster::logDebug(const QString& msg)
{
    if (scopeCommConnection().isDebugEnabled())
        qCDebug(scopeCommConnection) << QString("[Conn %0] %1").arg(_connectionId + 1).arg(msg);
}

However, note this only avoids the qCDebug formatting — the caller-side string building (including dumpToString) still runs. For a complete fix, callers would need to check scopeCommConnection().isDebugEnabled() before constructing the message, or use a macro/lambda pattern.

Also applies to: 119-119, 179-180, 236-236

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/communication/modbusmaster.cpp` at line 58, The debug message
construction currently calls dumpToString() and concatenates strings
unconditionally (e.g., in the register read log site that calls
logDebug("Register list read: " + dumpToString(registerList))), causing
unnecessary work when scopeCommConnection() debug is disabled; fix by guarding
caller-side message construction with if
(scopeCommConnection().isDebugEnabled()) before calling dumpToString() and
logDebug(), or change usages to pass already-checked/lazy-evaluated messages
into ModbusMaster::logDebug; locate references to dumpToString, the logDebug
method, and call sites in ModbusMaster where messages are built (including the
Register list read call and the other occurrences noted) and wrap those call
sites with the debug-enabled check to avoid unneeded string formatting.

tests/communication/tst_modbusdataunit.cpp (1)

68-73: LGTM — test correctly validates the fixed format string.

As a nice-to-have, consider covering the other ObjectType variants (e.g. COIL, HOLDING_REGISTER) to pin the full range of ModbusAddress::toString() output alongside the slave-id suffix:

🧪 Optional: parametrised object-type coverage

 void TestModbusDataUnit::toString()
 {
     ModbusDataUnit unit(123, ModbusAddress::ObjectType::INPUT_REGISTER, 42);
     QString expected = "input register, 123, slave id 42";
     QCOMPARE(unit.toString(), expected);
+
+    ModbusDataUnit unit2(1, ModbusAddress::ObjectType::COIL, 1);
+    QVERIFY(unit2.toString().endsWith(", slave id 1"));
+
+    ModbusDataUnit unit3(40001, ModbusAddress::ObjectType::HOLDING_REGISTER, 255);
+    QVERIFY(unit3.toString().endsWith(", slave id 255"));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/communication/tst_modbusdataunit.cpp` around lines 68 - 73, Current
test only asserts the INPUT_REGISTER output; extend TestModbusDataUnit::toString
to cover all ModbusAddress::ObjectType variants by adding parametrised cases (or
a small loop) that constructs ModbusDataUnit with each ObjectType (e.g., COIL,
DISCRETE_INPUT, HOLDING_REGISTER, INPUT_REGISTER), builds the corresponding
expected string using ModbusAddress::toString() semantic (e.g., "coil,
<address>, slave id <id>" / "holding register, <address>, slave id <id>"), and
QCOMPAREs unit.toString() against each expected value so the test pins full
range of ModbusAddress::toString() outputs plus the slave-id suffix; reference
TestModbusDataUnit::toString, ModbusDataUnit, and ModbusAddress::ObjectType when
locating the code to modify.

src/importexport/datafileexporter.cpp (1)

361-370: Correct %0-based → %1-based fix; consider hardening against %N in substituted values.

The 9-placeholder chained .arg() approach is functionally correct for typical serial port names. However, with chained calls, if any earlier substituted value (e.g. portName()) happened to contain a %2–%9 literal, subsequent .arg() calls would over-substitute it. Direct concatenation removes this ambiguity entirely:

♻️ Optional: use string concatenation to avoid phantom substitution

-        strSettings = QString("%1%2%3%4%5%6%7%8%9")
-                        .arg(connData->portName())
-                        .arg(Util::separatorCharacter())
-                        .arg(connData->baudrate())
-                        .arg(Util::separatorCharacter())
-                        .arg(strParity)
-                        .arg(Util::separatorCharacter())
-                        .arg(strDataBits)
-                        .arg(Util::separatorCharacter())
-                        .arg(strStopBits);
+        const QChar sep = Util::separatorCharacter();
+        strSettings = connData->portName()
+                      + sep + QString::number(connData->baudrate())
+                      + sep + strParity
+                      + sep + strDataBits
+                      + sep + strStopBits;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/importexport/datafileexporter.cpp` around lines 361 - 370, Replace the
chained nine-placeholder QString::arg() call that builds strSettings with direct
concatenation of the individual components to avoid accidental "%N" substitution
in values; specifically build strSettings from connData->portName(),
Util::separatorCharacter(), connData->baudrate() (convert to QString if
numeric), Util::separatorCharacter(), strParity, Util::separatorCharacter(),
strDataBits, Util::separatorCharacter(), and strStopBits so no .arg() chaining
is used and embedded percent sequences in portName() or other values cannot be
re-interpreted.