Open
Conversation
WalkthroughThis pull request removes MBC file import functionality and its associated Modbus communication infrastructure. Deleted are: the MBC configuration example file, import dialogs and UI models, Modbus connection management classes, register value handling and polling layers, and all related unit tests and test infrastructure. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dialogs/mainwindow.cpp (1)
850-862:⚠️ Potential issue | 🟠 MajorAdd an explicit
.mbcrejection guard inDataFileHandler::openDataFile()before the data-file parsing path.Currently, drag-and-drop and CLI opens of
.mbcfiles fallthrough to the generic data importer, which will attempt CSV-like parsing and fail with confusing importer errors rather than a clear "unsupported format" message. AlthoughFILE_TYPE_MBCexists in the file selection helpers, no dedicated MBC handler or import functionality has been implemented in the codebase. The.mbcfile format should be explicitly rejected inopenDataFile()with a specific error message, or alternatively, a proper MBC import path should be implemented to align with the documented functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dialogs/mainwindow.cpp` around lines 850 - 862, Add an explicit `.mbc` rejection in DataFileHandler::openDataFile(): detect files whose suffix (case-insensitive) equals "mbc" (the same extension represented by FILE_TYPE_MBC/mbc) and immediately abort the data-file import path with a clear, user-facing error (e.g., "Unsupported file format: .mbc — no MBC importer implemented") instead of falling through to CSV-like parsing; update DataFileHandler::openDataFile to return/throw the appropriate error or call the existing UI/error-reporting helper so MainWindow::handleFileOpen (which currently routes non-.mbs files to _pDataFileHandler->openDataFile) surfaces a specific unsupported-format message for .mbc files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/communication/modbuspoll.cpp`:
- Around line 60-82: The current loop in ModbusPoll builds a results list of
default ResultDouble() (which are NO_VALUE) and emits
registerDataReady(results), causing empty samples downstream; fix by either (A)
preventing this placeholder path when the Modbus backend is disabled: stop
scheduling the timer and do not emit registerDataReady (e.g., return early from
ModbusPoll::triggerRegisterRead or disable _pPollTimer when backend not
configured), or (B) populate results with actual register values before
emitting: iterate _registerList and read each register via the real Modbus read
path (use the existing read method or backend API used elsewhere) to set valid
ResultDouble values, then emit registerDataReady(results) and reschedule the
timer as before.
---
Outside diff comments:
In `@src/dialogs/mainwindow.cpp`:
- Around line 850-862: Add an explicit `.mbc` rejection in
DataFileHandler::openDataFile(): detect files whose suffix (case-insensitive)
equals "mbc" (the same extension represented by FILE_TYPE_MBC/mbc) and
immediately abort the data-file import path with a clear, user-facing error
(e.g., "Unsupported file format: .mbc — no MBC importer implemented") instead of
falling through to CSV-like parsing; update DataFileHandler::openDataFile to
return/throw the appropriate error or call the existing UI/error-reporting
helper so MainWindow::handleFileOpen (which currently routes non-.mbs files to
_pDataFileHandler->openDataFile) surfaces a specific unsupported-format message
for .mbc files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 238e6d37-61b9-4fbd-9523-05750240f5de
📒 Files selected for processing (84)
data/example.mbcsrc/communication/modbusconnection.cppsrc/communication/modbusconnection.hsrc/communication/modbusdataunit.cppsrc/communication/modbusdataunit.hsrc/communication/modbusmaster.cppsrc/communication/modbusmaster.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.hsrc/communication/modbusresultmap.hsrc/communication/readregisters.cppsrc/communication/readregisters.hsrc/communication/registervaluehandler.cppsrc/communication/registervaluehandler.hsrc/dialogs/importmbcdialog.cppsrc/dialogs/importmbcdialog.hsrc/dialogs/importmbcdialog.uisrc/dialogs/mainwindow.cppsrc/dialogs/mainwindow.hsrc/dialogs/mainwindow.uisrc/dialogs/mbcheader.hsrc/importexport/mbcfileimporter.cppsrc/importexport/mbcfileimporter.hsrc/importexport/mbcregisterdata.cppsrc/importexport/mbcregisterdata.hsrc/models/guimodel.cppsrc/models/guimodel.hsrc/models/mbcregisterfilter.cppsrc/models/mbcregisterfilter.hsrc/models/mbcregistermodel.cppsrc/models/mbcregistermodel.hsrc/models/mbcupdatemodel.cppsrc/models/mbcupdatemodel.hsrc/util/modbusdatatype.cpptests/CMakeLists.txttests/communication/CMakeLists.txttests/communication/communicationhelpers.htests/communication/tst_communication.cpptests/communication/tst_communication.htests/communication/tst_communicationstats.cpptests/communication/tst_communicationstats.htests/communication/tst_modbusconnection.cpptests/communication/tst_modbusconnection.htests/communication/tst_modbusdatatype.cpptests/communication/tst_modbusdatatype.htests/communication/tst_modbusdataunit.cpptests/communication/tst_modbusdataunit.htests/communication/tst_modbusmaster.cpptests/communication/tst_modbusmaster.htests/communication/tst_modbusmasterfake.cpptests/communication/tst_modbusmasterfake.htests/communication/tst_modbuspoll.cpptests/communication/tst_modbuspoll.htests/communication/tst_modbusregister.cpptests/communication/tst_modbusregister.htests/communication/tst_readregisters.cpptests/communication/tst_readregisters.htests/communication/tst_registervaluehandler.cpptests/communication/tst_registervaluehandler.htests/importexport/CMakeLists.txttests/importexport/mbctestdata.cpptests/importexport/mbctestdata.htests/importexport/tst_mbcfileimporter.cpptests/importexport/tst_mbcfileimporter.htests/importexport/tst_mbcregisterfilter.cpptests/importexport/tst_mbcregisterfilter.htests/models/CMakeLists.txttests/models/tst_mbcregistermodel.cpptests/models/tst_mbcregistermodel.htests/models/tst_mbcupdatemodel.cpptests/models/tst_mbcupdatemodel.htests/testslave/modbusconnectionfake.cpptests/testslave/modbusconnectionfake.htests/testslave/testdevice.cpptests/testslave/testdevice.htests/testslave/testslavedata.cpptests/testslave/testslavedata.htests/testslave/testslavemodbus.cpptests/testslave/testslavemodbus.htests/testslave/testslavemodbusmulti.cpptests/testslave/testslavemodbusmulti.htests/util/CMakeLists.txttests/util/tst_modbusaddress.cpptests/util/tst_modbusaddress.h
💤 Files with no reviewable changes (80)
- src/util/modbusdatatype.cpp
- tests/communication/communicationhelpers.h
- src/dialogs/mainwindow.ui
- tests/CMakeLists.txt
- tests/util/CMakeLists.txt
- src/communication/modbusresultmap.h
- tests/models/CMakeLists.txt
- data/example.mbc
- src/dialogs/mbcheader.h
- tests/importexport/CMakeLists.txt
- tests/communication/CMakeLists.txt
- tests/communication/tst_modbusmasterfake.h
- tests/models/tst_mbcupdatemodel.h
- tests/communication/tst_readregisters.cpp
- src/dialogs/importmbcdialog.ui
- tests/communication/tst_modbusdataunit.h
- src/importexport/mbcfileimporter.h
- tests/testslave/testslavemodbus.cpp
- src/models/guimodel.h
- tests/communication/tst_modbusmasterfake.cpp
- tests/models/tst_mbcregistermodel.cpp
- tests/importexport/tst_mbcfileimporter.h
- tests/util/tst_modbusaddress.cpp
- tests/communication/tst_registervaluehandler.cpp
- tests/communication/tst_communication.cpp
- src/importexport/mbcregisterdata.cpp
- tests/importexport/tst_mbcfileimporter.cpp
- tests/testslave/testdevice.cpp
- tests/util/tst_modbusaddress.h
- tests/testslave/testslavemodbusmulti.h
- tests/communication/tst_modbusregister.cpp
- src/models/mbcregisterfilter.h
- src/communication/modbusdataunit.h
- tests/communication/tst_communicationstats.h
- src/communication/modbusmaster.h
- tests/communication/tst_modbusdataunit.cpp
- tests/importexport/mbctestdata.cpp
- tests/communication/tst_modbusconnection.h
- tests/testslave/modbusconnectionfake.h
- tests/testslave/modbusconnectionfake.cpp
- src/communication/modbusdataunit.cpp
- tests/testslave/testslavedata.h
- tests/communication/tst_modbusdatatype.h
- src/communication/registervaluehandler.cpp
- tests/communication/tst_modbusmaster.h
- tests/importexport/tst_mbcregisterfilter.cpp
- src/models/mbcregisterfilter.cpp
- tests/testslave/testdevice.h
- tests/communication/tst_modbusmaster.cpp
- src/importexport/mbcregisterdata.h
- tests/importexport/mbctestdata.h
- tests/testslave/testslavemodbus.h
- tests/models/tst_mbcregistermodel.h
- tests/communication/tst_modbusconnection.cpp
- tests/communication/tst_modbusdatatype.cpp
- tests/communication/tst_modbusregister.h
- src/dialogs/importmbcdialog.h
- tests/communication/tst_readregisters.h
- tests/models/tst_mbcupdatemodel.cpp
- src/communication/registervaluehandler.h
- tests/communication/tst_communicationstats.cpp
- src/communication/readregisters.cpp
- src/models/mbcupdatemodel.h
- tests/communication/tst_modbuspoll.h
- tests/testslave/testslavemodbusmulti.cpp
- tests/communication/tst_modbuspoll.cpp
- src/communication/modbusconnection.h
- tests/communication/tst_communication.h
- src/importexport/mbcfileimporter.cpp
- src/communication/readregisters.h
- tests/communication/tst_registervaluehandler.h
- tests/importexport/tst_mbcregisterfilter.h
- src/models/guimodel.cpp
- src/models/mbcupdatemodel.cpp
- src/models/mbcregistermodel.cpp
- src/communication/modbusmaster.cpp
- src/models/mbcregistermodel.h
- src/dialogs/importmbcdialog.cpp
- tests/testslave/testslavedata.cpp
- src/communication/modbusconnection.cpp
Comment on lines
+60
to
+82
| ResultDoubleList results; | ||
| for (qsizetype i = 0; i < _registerList.size(); i++) | ||
| { | ||
| regAddrList.append(QList<ModbusDataUnit>()); | ||
|
|
||
| _pRegisterValueHandler->registerAddresListForConnection(regAddrList.last(), i); | ||
|
|
||
| if (regAddrList.last().count() > 0) | ||
| { | ||
| _activeMastersCount++; | ||
| } | ||
| results.append(ResultDouble()); | ||
| } | ||
| emit registerDataReady(results); | ||
|
|
||
| for (connectionId_t i = 0u; i < ConnectionTypes::ID_CNT; i++) | ||
| { | ||
| if (regAddrList.at(i).count() > 0) | ||
| { | ||
| quint8 consecutiveMax = lowestConsecutiveMaxForConnection(i); | ||
| _modbusMasters[i]->bActive = true; | ||
| _modbusMasters[i]->pModbusMaster->readRegisterList(regAddrList.at(i), consecutiveMax); | ||
| } | ||
| } | ||
| // Reschedule timer | ||
| uint waitInterval; | ||
| const quint32 passedInterval = static_cast<quint32>(QDateTime::currentMSecsSinceEpoch() - _lastPollStart); | ||
|
|
||
| if (_activeMastersCount == 0) | ||
| if (passedInterval > _pSettingsModel->pollTime()) | ||
| { | ||
| ModbusResultMap emptyResultMap; | ||
| handlePollDone(emptyResultMap, ConnectionTypes::ID_1); | ||
| // Poll again immediately | ||
| waitInterval = 1; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| quint8 ModbusPoll::lowestConsecutiveMaxForConnection(connectionId_t connId) const | ||
| { | ||
| quint8 consecutiveMax = 128; | ||
| QList<deviceId_t> devList = _pSettingsModel->deviceListForConnection(connId); | ||
| for (deviceId_t devId : std::as_const(devList)) | ||
| { | ||
| quint8 devConsecutiveMax = _pSettingsModel->deviceSettings(devId)->consecutiveMax(); | ||
| if (devConsecutiveMax < consecutiveMax) | ||
| else | ||
| { | ||
| consecutiveMax = devConsecutiveMax; | ||
| // Set waitInterval to remaining time | ||
| waitInterval = _pSettingsModel->pollTime() - passedInterval; | ||
| } | ||
|
|
||
| _pPollTimer->singleShot(static_cast<int>(waitInterval), this, &ModbusPoll::triggerRegisterRead); |
There was a problem hiding this comment.
Do not keep polling with placeholder NO_VALUE samples.
This loop now only uses _registerList.size() and appends default-constructed ResultDouble() objects. MainWindow still reaches this path when logging starts (src/dialogs/mainwindow.cpp:471-479), but ResultDouble() defaults to NO_VALUE (src/util/result.h:62-65), and GraphDataHandler::handleRegisterData() forwards that list into QMuParser::setRegistersData() (src/datahandling/graphdatahandler.cpp:73-100). The graph/logging pipeline therefore receives an all-invalid sample set on every poll instead of actual register values.
Please either remove/disable this path together with the Modbus backend, or keep a backend here that can populate successful ResultDouble values before emitting registerDataReady.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/communication/modbuspoll.cpp` around lines 60 - 82, The current loop in
ModbusPoll builds a results list of default ResultDouble() (which are NO_VALUE)
and emits registerDataReady(results), causing empty samples downstream; fix by
either (A) preventing this placeholder path when the Modbus backend is disabled:
stop scheduling the timer and do not emit registerDataReady (e.g., return early
from ModbusPoll::triggerRegisterRead or disable _pPollTimer when backend not
configured), or (B) populate results with actual register values before
emitting: iterate _registerList and read each register via the real Modbus read
path (use the existing read method or backend API used elsewhere) to set valid
ResultDouble values, then emit registerDataReady(results) and reschedule the
timer as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Refactoring
Tests