diff --git a/libmodmqttsrv/modbus_executor.cpp b/libmodmqttsrv/modbus_executor.cpp index 7df5764..376f092 100644 --- a/libmodmqttsrv/modbus_executor.cpp +++ b/libmodmqttsrv/modbus_executor.cpp @@ -23,7 +23,6 @@ ModbusExecutor::ModbusExecutor( { //some random past value, not using steady_clock:min() due to overflow mLastCommandTime = std::chrono::steady_clock::now() - std::chrono::hours(100000); - mLastQueue = mSlaveQueues.end(); mCurrentSlaveQueue = mSlaveQueues.end(); mInitialPoll = false; mReadRetryCount = mMaxReadRetryCount; @@ -119,13 +118,13 @@ void ModbusExecutor::addWriteCommand(const std::shared_ptr& pCommand) { if (mWriteCommandsQueued == 0) { // skip queuing for if there is no queued write commands. - // This improves write latency in use case, where there is a lot of polling - // and sporadic write. I belive this is main use case for this gateway. + // This improves write latency in use case, where there is a lot of polling + // and sporadic write. I belive this is main use case for this gateway. - // TODO this could leat to poll queue starvation when write commands - // arive in sync with slave execution time. Maybe there should be a - // configuration switch to turn it off? - if (mWaitingCommand != nullptr) + // TODO this could lead to poll queue starvation when write commands + // arive in sync with slave execution time. Maybe there should be a + // configuration switch to turn it off? + if (mWaitingCommand != nullptr) mSlaveQueues[pCommand->mSlaveId].readdCommand(mWaitingCommand); mWaitingCommand = pCommand; @@ -235,8 +234,8 @@ ModbusExecutor::executeNext() { || ( mWaitingCommand->getDelay().delay_type == ModbusCommandDelay::DelayType::ON_SLAVE_CHANGE - && mLastQueue != mSlaveQueues.end() - && mCurrentSlaveQueue->first != mLastQueue->first + && mLastCommand != nullptr + && mWaitingCommand->mSlaveId != mLastCommand->mSlaveId ) ) { auto delay_passed = std::chrono::steady_clock::now() - mLastCommandTime; @@ -346,14 +345,14 @@ ModbusExecutor::sendCommand() { assert(mWriteCommandsQueued >= 0); } } - mLastQueue = mCurrentSlaveQueue; mLastCommand = mWaitingCommand; // to retry just leave mCurrentCommand // for next executeNext() call if (!retry) { mWaitingCommand.reset(); - mCommandsLeft--; + if (mCommandsLeft > 0) + mCommandsLeft--; } } @@ -386,7 +385,7 @@ void ModbusExecutor::resetCommandsCounter() { if (mCurrentSlaveQueue->second.mPollQueue.empty()) mCommandsLeft = WRITE_BATCH_SIZE; - else + else if (mCurrentSlaveQueue != mSlaveQueues.end()) mCommandsLeft = mCurrentSlaveQueue->second.mPollQueue.size() * 2; } diff --git a/libmodmqttsrv/modbus_executor.hpp b/libmodmqttsrv/modbus_executor.hpp index 1dc16bb..0f65993 100644 --- a/libmodmqttsrv/modbus_executor.hpp +++ b/libmodmqttsrv/modbus_executor.hpp @@ -74,7 +74,6 @@ class ModbusExecutor { std::chrono::steady_clock::time_point mLastCommandTime; //used to determine if we have to respect delay of RegisterPoll::ReadDelayType::ON_SLAVE_CHANGE - std::map::iterator mLastQueue; std::shared_ptr mWaitingCommand; std::shared_ptr mLastCommand; diff --git a/unittests/mqtt_command_only_tests.cpp b/unittests/mqtt_command_only_tests.cpp index 2b3ecb5..0e6dee6 100644 --- a/unittests/mqtt_command_only_tests.cpp +++ b/unittests/mqtt_command_only_tests.cpp @@ -3,6 +3,8 @@ #include "defaults.hpp" #include "yaml_utils.hpp" +TEST_CASE ("Write value in write-only configuration should succeed") { + TestConfig config(R"( modmqttd: modbus: @@ -22,18 +24,18 @@ TestConfig config(R"( objects: - topic: test_switch commands: - - name: set + - name: set1 register: tcptest.1.2 register_type: holding )"); -TEST_CASE ("Write value in write-only configuration should succeed") { + MockedModMqttServerThread server(config.toString()); server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0); server.start(); - server.waitForSubscription("test_switch/set"); + server.waitForSubscription("test_switch/set1"); - server.publish("test_switch/set", "7"); + server.publish("test_switch/set1", "7"); server.waitForModbusValue("tcptest",1,2, modmqttd::RegisterType::HOLDING, 0x7); server.stop(); @@ -41,16 +43,47 @@ TEST_CASE ("Write value in write-only configuration should succeed") { TEST_CASE ("Write should respect slave delay_before_command") { + +TestConfig config(R"( +modmqttd: +modbus: + networks: + - name: tcptest + address: localhost + port: 501 + delay_before_first_command: 5ms + slaves: + - address: 2 + delay_before_first_command: 100ms + +mqtt: + client_id: mqtt_test + broker: + host: localhost + objects: + - topic: test_switch + commands: + - name: set1 + register: tcptest.1.1 + register_type: holding + - name: set2 + register: tcptest.2.2 + register_type: holding +)"); + MockedModMqttServerThread server(config.toString()); - server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0); + server.setModbusRegisterValue("tcptest", 1, 1, modmqttd::RegisterType::HOLDING, 0); + server.setModbusRegisterValue("tcptest", 2, 2, modmqttd::RegisterType::HOLDING, 0); server.start(); - auto now = std::chrono::steady_clock::now(); - server.waitForSubscription("test_switch/set"); + server.waitForSubscription("test_switch/set1"); + server.waitForSubscription("test_switch/set2"); - server.publish("test_switch/set", "7"); - server.publish("test_switch/set", "8"); - server.waitForModbusValue("tcptest",1,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(700)); + auto now = std::chrono::steady_clock::now(); + server.publish("test_switch/set1", "7"); + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + server.publish("test_switch/set2", "8"); + server.waitForModbusValue("tcptest",2,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(70000)); auto after = std::chrono::steady_clock::now(); //global is ignored REQUIRE(after - now >= std::chrono::milliseconds(100)); @@ -60,6 +93,7 @@ TEST_CASE ("Write should respect slave delay_before_command") { TEST_CASE ("When network delay") { +SECTION("is set to delay_before_command write should wait") { TestConfig global_delay_config(R"( modmqttd: @@ -81,7 +115,7 @@ TestConfig global_delay_config(R"( register_type: holding )"); -SECTION("is set to delay_before_command write should wait") { + MockedModMqttServerThread server(global_delay_config.toString()); server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0); server.start(); @@ -99,20 +133,45 @@ SECTION("is set to delay_before_command write should wait") { server.stop(); } + SECTION("is set to delay_before_first_command write should wait") { - global_delay_config.mYAML["modbus"]["networks"][0].remove("delay_before_command"); - global_delay_config.mYAML["modbus"]["networks"][0]["delay_before_first_command"] = "500ms"; - MockedModMqttServerThread server(global_delay_config.toString()); - server.setModbusRegisterValue("tcptest", 1, 2, modmqttd::RegisterType::HOLDING, 0); +TestConfig global_first_delay_config(R"( +modmqttd: +modbus: + networks: + - name: tcptest + delay_before_first_command: 500ms + address: localhost + port: 501 +mqtt: + client_id: mqtt_test + broker: + host: localhost + objects: + - topic: test_switch + commands: + - name: set1 + register: tcptest.1.1 + register_type: holding + - name: set2 + register: tcptest.2.2 + register_type: holding +)"); + + + MockedModMqttServerThread server(global_first_delay_config.toString()); + server.setModbusRegisterValue("tcptest", 1, 1, modmqttd::RegisterType::HOLDING, 0); + server.setModbusRegisterValue("tcptest", 2, 2, modmqttd::RegisterType::HOLDING, 0); server.start(); - auto now = std::chrono::steady_clock::now(); - server.waitForSubscription("test_switch/set"); + server.waitForSubscription("test_switch/set1"); + server.waitForSubscription("test_switch/set2"); - server.publish("test_switch/set", "7"); - server.publish("test_switch/set", "8"); - server.waitForModbusValue("tcptest",1,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(700)); + auto now = std::chrono::steady_clock::now(); + server.publish("test_switch/set1", "7"); + server.publish("test_switch/set2", "8"); + server.waitForModbusValue("tcptest",2,2, modmqttd::RegisterType::HOLDING, 0x8, std::chrono::milliseconds(700)); auto after = std::chrono::steady_clock::now(); REQUIRE(after - now >= std::chrono::milliseconds(500));