Fix reconfiguration of fast modbus events #831
Conversation
WalkthroughThe changes in this pull request include updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
src/serial_client_register_poller.h (1)
56-56: Add documentation for the new method.Consider adding a documentation comment explaining the purpose and behavior of
RescheduleDisconnectedDevices(). This would help other developers understand when and why this method is called, especially given its important role in handling device reconnection after reboots.Example documentation:
/** * Reschedules polling for devices that were disconnected. * Called during port cycles to ensure event-driven devices * are properly reconfigured after a reboot. */ void RescheduleDisconnectedDevices();src/serial_client_register_poller.cpp (2)
140-141: Consider optimizing duplicate rescheduling calls.The
RescheduleDisconnectedDevices()is called at the start of bothClosedPortCycleandOpenPortCycle. While this ensures devices are rescheduled regardless of port state, it could lead to unnecessary duplicate work if both cycles run in quick succession.Consider adding a flag to track if rescheduling has been performed recently:
class TSerialClientRegisterPoller { + steady_clock::time_point LastRescheduleTime; + const std::chrono::seconds RESCHEDULE_COOLDOWN{1}; // ... void RescheduleDisconnectedDevices() { + auto now = steady_clock::now(); + if (now - LastRescheduleTime < RESCHEDULE_COOLDOWN) { + return; + } // ... existing implementation ... + LastRescheduleTime = now; } };Also applies to: 185-186
268-279: Consider performance optimizations for device rescheduling.The current implementation might cause performance issues when dealing with a large number of disconnected devices, as it processes all devices at once.
Consider implementing rate limiting or batching:
+ const size_t BATCH_SIZE = 10; + const std::chrono::milliseconds BATCH_DELAY{100}; + void TSerialClientRegisterPoller::RescheduleDisconnectedDevices() { - for (auto& device: DisconnectedDevicesWaitingForReschedule) { + size_t processed = 0; + auto it = DisconnectedDevicesWaitingForReschedule.begin(); + while (it != DisconnectedDevicesWaitingForReschedule.end()) { + auto& device = *it; auto range = Devices.equal_range(device); for (auto devIt = range.first; devIt != range.second; ++devIt) { Scheduler.Remove(devIt->second); devIt->second->RescheduleAllRegisters(); Scheduler.AddEntry(devIt->second, devIt->second->GetDeadline(), devIt->second->GetPriority()); } + it = DisconnectedDevicesWaitingForReschedule.erase(it); + processed++; + + // Process in batches to avoid blocking + if (processed % BATCH_SIZE == 0 && it != DisconnectedDevicesWaitingForReschedule.end()) { + std::this_thread::sleep_for(BATCH_DELAY); + } } - DisconnectedDevicesWaitingForReschedule.clear(); }src/poll_plan.h (1)
84-88: Add documentation for TEntry requirements.The implementation is correct, but it would be helpful to document that
TEntrymust supportoperator==since bothContainsandRemovemethods rely on it.Add this documentation above the class declaration:
/** * @brief Priority queue schedule for entries that can be compared * @tparam TEntry Type that must support operator== * @tparam ComparePredicate Predicate for comparing entries */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
debian/changelog(1 hunks)src/poll_plan.h(3 hunks)src/serial_client_register_poller.cpp(4 hunks)src/serial_client_register_poller.h(2 hunks)
🔇 Additional comments (5)
src/serial_client_register_poller.h (1)
66-66: LGTM: Well-designed member variable.
The DisconnectedDevicesWaitingForReschedule vector is appropriately placed and named. Using std::vector is a good choice for managing a dynamic collection of disconnected devices that need rescheduling.
src/serial_client_register_poller.cpp (1)
268-279: Verify handling of event-only devices.
The implementation reschedules all disconnected devices, but we need to verify it correctly handles devices that are configured for event-only polling.
Let's check the codebase for event-only device configurations:
src/poll_plan.h (2)
16-23: LGTM! Well-implemented removal with heap maintenance.
The implementation correctly maintains the heap property after removal using std::make_heap. The template predicate parameter provides good flexibility for removal criteria.
310-315: Document the method and verify reboot handling.
The implementation correctly removes entries from both priority queues, which aligns with the PR's objective of handling device reboots. However, it would benefit from documentation explaining its role in the reboot scenario.
Add documentation above the method:
/**
* @brief Removes an entry from both high and low priority queues
* @param entry The entry to remove
*
* This method is used during device reboot handling to ensure proper
* reconfiguration of Modbus events by removing existing schedule entries.
*/Let's verify how this method is used in the reboot handling:
✅ Verification successful
Remove method usage verified; add documentation.
The Remove method is correctly utilized in the RescheduleDisconnectedDevices function within serial_client_register_poller.cpp, aligning with the PR's objective of handling device reboots. It would benefit from documentation explaining its role in the reboot scenario.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of Remove method in reboot/disconnect handling
rg -l "Remove.*\(" | xargs rg -A 5 "disconnect|reboot|restart"
Length of output: 64
Script:
#!/bin/bash
# Search for all usages of the Remove method
rg "Remove\s*\(" --context 5
Length of output: 2664
debian/changelog (1)
1-5: LGTM! The changelog entry accurately describes the changes.
The changelog entry properly documents the fix for reconfiguring fast modbus events after device restart, which aligns with the PR objectives. The version bump (2.147.1) follows semantic versioning conventions.
|
|
||
| void TSerialClientRegisterPoller::RescheduleDisconnectedDevices() | ||
| { | ||
| for (auto& device: DisconnectedDevicesWaitingForReschedule) { |
There was a problem hiding this comment.
| for (auto& device: DisconnectedDevicesWaitingForReschedule) { | |
| for (const auto& device: DisconnectedDevicesWaitingForReschedule) { |
|
Добавил тестов |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
test/poll_test.cpp (1)
595-598: Consider consolidating repeated method calls.The test has three identical consecutive calls to
EnqueueReadEventswith the same parameters.Consider using a loop to reduce code duplication:
- EnqueueReadEvents(30ms, 0, true, MAX_EVENT_RESPONSE_SIZE); - EnqueueReadEvents(30ms, 0, true, MAX_EVENT_RESPONSE_SIZE); - EnqueueReadEvents(30ms, 0, true, MAX_EVENT_RESPONSE_SIZE); + for(int j = 0; j < 3; ++j) { + EnqueueReadEvents(30ms, 0, true, MAX_EVENT_RESPONSE_SIZE); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
test/TPollTest.ReconnectWithOnlyEvents.datis excluded by!**/*.dat
📒 Files selected for processing (1)
test/poll_test.cpp(5 hunks)
🔇 Additional comments (4)
test/poll_test.cpp (4)
12-13: LGTM: Well-defined constant for event response size.
The constant MAX_EVENT_RESPONSE_SIZE is appropriately defined with a clear purpose and hex value that matches the Modbus protocol specification.
Line range hint 133-155: LGTM: Improved method signature with better parameter organization.
The updated EnqueueReadEvents method now has a clearer signature with explicit parameters, and the new overload provides a convenient way to call with default values.
157-183: LGTM: Well-implemented reset event simulation.
The EnqueueResetEvent method correctly simulates a device reset event with appropriate Modbus function codes and event type flags. The implementation aligns with the PR objective to handle device reboots.
675-725: LGTM: Comprehensive test case for device reset handling.
The new test case ReconnectWithOnlyEvents thoroughly validates the system's behavior when a device with event-only polling is reset:
- Correctly sets up initial event polling
- Simulates device reset
- Verifies re-enabling of events
- Confirms return to normal event polling
This test case directly addresses the PR's objective of fixing event reconfiguration after device restarts.
| @@ -635,3 +671,55 @@ TEST_F(TPollTest, SemiSporadicRegister) | |||
| Cycle(serialClient, lastAccessedDevice); | |||
| } | |||
| } | |||
|
|
|||
| TEST_F(TPollTest, ReconnectWithOnlyEvents) | |||
There was a problem hiding this comment.
@KraPete valgrind ругается на этот тест, если в VALGRIND_FLAGS заменить -q на --track-origins=yes покажет детали:
[ RUN ] TPollTest.ReconnectWithOnlyEvents
<6>INFO: [serial client] Events are enabled for <modbus:1:holding: 1>
<6>INFO: [serial client] Events are disabled for <modbus:1: reboot>
<6>INFO: [serial device] device modbus:1 is connected
==702== Conditional jump or move depends on uninitialised value(s)
==702== at 0x54C31B: ModbusExt::IsModbusExtPacket(unsigned char const*, unsigned long) (in /build/source/test/wb-homa-test)
==702== by 0x54C362: ModbusExt::GetPacketStart(unsigned char const*, unsigned long) (in /build/source/test/wb-homa-test)
==702== by 0x54C3A1: std::_Function_handler<bool (unsigned char*, unsigned long), ModbusExt::ExpectEvents()::{lambda(unsigned char*, unsigned long)#1}>::_M_invoke(std::_Any_data const&, unsigned char*&&, unsigned long&&) (in /build/source/test/wb-homa-test)
==702== by 0x5B73F9: TFakeSerialPort::ReadFrame(unsigned char*, unsigned long, std::chrono::duration<long, std::ratio<1l, 1000000l> > const&, std::chrono::duration<long, std::ratio<1l, 1000000l> > const&, std::function<bool (unsigned char*, unsigned long)>) (in /build/source/test/wb-homa-test)
==702== by 0x62E5A8: (anonymous namespace)::TFakeSerialPortWithTime::ReadFrame(unsigned char*, unsigned long, std::chrono::duration<long, std::ratio<1l, 1000000l> > const&, std::chrono::duration<long, std::ratio<1l, 1000000l> > const&, std::function<bool (unsigned char*, unsigned long)>) (in /build/source/test/wb-homa-test)
==702== by 0x54D863: ModbusExt::ReadEvents(TPort&, std::chrono::duration<long, std::ratio<1l, 1000l> >, unsigned char, ModbusExt::TEventConfirmationState&, ModbusExt::IEventsVisitor&) (in /build/source/test/wb-homa-test)
==702== by 0x4B9847: TSerialClientEventsReader::ReadEvents(TPort&, std::chrono::duration<long, std::ratio<1l, 1000l> >, std::function<void (std::shared_ptr<TRegister>)>, std::function<std::chrono::time_point<std::chrono::_V2::steady_clock, std::chrono::duration<long, std::ratio<1l, 1000000000l> > > ()>) (in /build/source/test/wb-homa-test)
==702== by 0x49C18E: TSerialClientRegisterAndEventsReader::OpenPortCycle(TPort&, std::function<void (std::shared_ptr<TRegister>)>, TSerialClientDeviceAccessHandler&) (in /build/source/test/wb-homa-test)
wb-mqtt-serial> ==702== by 0x635ADE: TPollTest::Cycle(TSerialClientRegisterAndEventsReader&, TSerialClientDeviceAccessHandler&) (in /build/source/test/wb-homa-test)
==702== by 0x632C28: TPollTest_ReconnectWithOnlyEvents_Test::TestBody() (in /build/source/test/wb-homa-test)
==702== by 0x4A3DDFC: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /gtest-1.15.2/lib/libgtest.so.1.15.2)
==702== by 0x4A28335: testing::Test::Run() (in /gtest-1.15.2/lib/libgtest.so.1.15.2)
==702== Uninitialised value was created by a stack allocation
==702== at 0x54D793: ModbusExt::ReadEvents(TPort&, std::chrono::duration<long, std::ratio<1l, 1000l> >, unsigned char, ModbusExt::TEventConfirmationState&, ModbusExt::IEventsVisitor&) (in /build/source/test/wb-homa-test)
==702==
https://wirenboard.youtrack.cloud/issue/SOFT-4633/Ne-vosstanavlivaetsya-opros-ustrojstv-posle-perezegruzki
Если устройство опрашивалось только через события, то после его перезагрузки опрос останавливался. После перезагрузки настройки отправки событий в устройстве сбрасываются. Их надо перенастроить. Для этого надо послать команды, они посылаются только перед обычным чтением регистров. Но в списке регистров для чтения ничего не было про это устройстов, т.к. всё через события.
По событию перезагрузки записываем устройстов в список тех, что надо снова запланировать для опроса.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
wb-mqtt-serialpackage to version 2.147.1, enhancing the handling of Modbus events after device restarts.Bug Fixes
Documentation