From 7013a095633addd9cad36c5061e78beb4d214e26 Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Mon, 9 Sep 2024 10:35:03 +0200 Subject: [PATCH 1/6] refs #PMP2-305 Added logic to reject information messages with invalid NR. Added related unit test. Various fixes in the unit test HNZ server. Signed-off-by: Florent Peyrusse --- include/hnzpath.h | 6 ++-- src/hnzpath.cpp | 23 ++++++++++--- tests/server/basic_hnz_server.cpp | 26 +++++++++++--- tests/server/basic_hnz_server.h | 9 ++++- tests/test_hnz.cpp | 56 +++++++++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 13 deletions(-) diff --git a/include/hnzpath.h b/include/hnzpath.h index 2ca1aa4..e9999ae 100644 --- a/include/hnzpath.h +++ b/include/hnzpath.h @@ -294,15 +294,17 @@ class HNZPath { * Call this method when a RR message is received. * @param nr NR of the RTU * @param repetition set to true if frame received is repeated + * @return True if the NR contained in the message was correct, else false */ - void m_receivedRR(int nr, bool repetition); + bool m_receivedRR(int nr, bool repetition); /** * Send a RR * @param repetition set to true if frame received is repeated * @param ns NS of the received frame + * @return True if received NR was valid and RR was sent, false if invalid NR was received and no RR was sent */ - void m_sendRR(bool repetition, int ns, int nr); + bool m_sendRR(bool repetition, int ns, int nr); /** * Send an information frame. The address byte, numbering bit (containing NR, diff --git a/src/hnzpath.cpp b/src/hnzpath.cpp index e0a08df..ab102cb 100644 --- a/src/hnzpath.cpp +++ b/src/hnzpath.cpp @@ -376,7 +376,10 @@ vector> HNZPath::m_analyze_frame(MSG_TRAME* frReceived) { } // Computing the frame number & sending RR - m_sendRR(pf == 1, ns, nr); + if (!m_sendRR(pf == 1, ns, nr)) { + // If NR was invalid, skip message processing + messages.clear(); + } } else { // Supervision frame HnzUtility::log_info(beforeLog + " RR received (f = " + to_string(pf) + ", nr = " + to_string(nr) + ")"); @@ -482,7 +485,7 @@ void HNZPath::m_receivedUA() { void HNZPath::m_receivedBULLE() { m_last_msg_time = time(nullptr); } -void HNZPath::m_receivedRR(int nr, bool repetition) { +bool HNZPath::m_receivedRR(int nr, bool repetition) { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_receivedRR - " + m_name_log; if (nr != m_NRR) { int frameOk = (nr - m_NRR + 7) % 8 + 1; @@ -512,13 +515,19 @@ void HNZPath::m_receivedRR(int nr, bool repetition) { } } else { HnzUtility::log_warn(beforeLog + " Received an unexpected repeated RR, ignoring it"); + return false; } } else { // invalid NR - HnzUtility::log_warn(beforeLog + " Ignoring the RR, NR (=" + to_string(nr) + ") is invalid." + + HnzUtility::log_warn(beforeLog + " Ignoring the RR, NR (" + to_string(nr) + ") is invalid. " + "Current NRR : " + to_string(m_NRR + 1)); + return false; } } + else { + HnzUtility::log_debug(beforeLog + " Received RR with NR=NRR (" + to_string(nr) + "), ignoring it"); + } + return true; } void HNZPath::m_sendSARM() { @@ -543,10 +552,13 @@ void HNZPath::m_sendBULLE() { HnzUtility::log_info(beforeLog + " BULLE " + (sent?"sent":"discarded")); } -void HNZPath::m_sendRR(bool repetition, int ns, int nr) { +bool HNZPath::m_sendRR(bool repetition, int ns, int nr) { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_sendRR - " + m_name_log; // use NR to validate frames sent - m_receivedRR(nr, 0); + if(!m_receivedRR(nr, false)) { + HnzUtility::log_warn(beforeLog + " Information frame contained unexpected NR (" + std::to_string(nr) + "), ignoring it"); + return false; + } // send RR message if (ns == m_nr) { @@ -576,6 +588,7 @@ void HNZPath::m_sendRR(bool repetition, int ns, int nr) { // Update timer m_last_msg_time = time(nullptr); + return true; } bool HNZPath::m_sendInfo(unsigned char* msg, unsigned long size) { diff --git a/tests/server/basic_hnz_server.cpp b/tests/server/basic_hnz_server.cpp index 76d3159..04e2b75 100644 --- a/tests/server/basic_hnz_server.cpp +++ b/tests/server/basic_hnz_server.cpp @@ -278,9 +278,6 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { std::lock_guard guard2(m_sarm_ua_mutex); if (ua_ok && sarm_ok) break; } - m_t2->join(); - delete m_t2; - m_t2 = nullptr; if (!is_running) { printf("[HNZ Server][%d] Not running after SARM/UA, exit\n", m_port); fflush(stdout); return false; @@ -288,7 +285,14 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { printf("[HNZ Server][%d] Connection OK !!\n", m_port); fflush(stdout); // Connection established + m_ns = 0; + m_nr = 0; receiving_thread = new thread(&BasicHNZServer::receiving_loop, this); + + // Join sarm thread after starting receiving loop as it may wait up to 3 seconds and some messages received will be missed (no RR sent) + m_t2->join(); + delete m_t2; + m_t2 = nullptr; return true; } @@ -299,8 +303,20 @@ void BasicHNZServer::sendSARM() { createAndSendFrame(0x05, message, sizeof(message)); } -void BasicHNZServer::sendFrame(vector message, bool repeat) { - int num = (((repeat) ? (m_ns - 1) : m_ns) % 8) << 1 + ((m_nr + 1 << 5) % 8); +void BasicHNZServer::sendFrame(vector message, bool repeat, FrameError frameError /*= {}*/) { + int p = repeat ? 1 : 0; + int nr = m_nr; + if (frameError.nr_minus_1) { + nr = (m_nr + 7) % 8; // NR-1 + } + if (frameError.nr_plus_2) { + nr = (m_nr + 2) % 8; // NR+2 + } + int ns = m_ns; + if (repeat) { + ns = (m_ns + 7) % 8; // NS-1 + } + int num = (nr << 5) + (p << 4) + (ns << 1); message.insert(message.begin(), num); int len = message.size(); createAndSendFrame(addr, message.data(), len); diff --git a/tests/server/basic_hnz_server.h b/tests/server/basic_hnz_server.h index bc58287..726148d 100644 --- a/tests/server/basic_hnz_server.h +++ b/tests/server/basic_hnz_server.h @@ -27,7 +27,14 @@ class BasicHNZServer { void sendSARM(); - void sendFrame(vector message, bool repeat); + struct FrameError { + bool nr_minus_1; + bool nr_plus_2; + // Use constructor instead of default values for bracket initializer to work in C++11 + FrameError(bool _nr_minus_1 = false, bool _nr_plus_2 = false): + nr_minus_1(_nr_minus_1), nr_plus_2(_nr_plus_2) {} + }; + void sendFrame(vector message, bool repeat, FrameError frameError = {}); void createAndSendFrame(unsigned char addr, unsigned char *msg, int msgSize); // Note: return the strcutre by value becase a copy must be done by the caller to remain thread safe std::vector> popLastFramesReceived(); diff --git a/tests/test_hnz.cpp b/tests/test_hnz.cpp index 45904ea..b68110b 100644 --- a/tests/test_hnz.cpp +++ b/tests/test_hnz.cpp @@ -405,6 +405,17 @@ class HNZTest : public testing::Test { return frameFound; } + static std::shared_ptr findRR(const std::vector>& frames) { + std::shared_ptr frameFound = nullptr; + for(auto frame: frames) { + if((frame->usLgBuffer > 1) && ((frame->aubTrame[1] & 0x0F) == 0x1)) { + frameFound = frame; + break; + } + } + return frameFound; + } + static void validateFrame(const std::vector>& frames, const std::vector& expectedFrame, bool fullFrame = false) { // When fullFrame is true, expectedFrame shall contain the complete frame: @@ -2655,3 +2666,48 @@ TEST_F(HNZTest, NoMessageBufferedIfConnectionLost) { TVCframe = findFrameWithId(frames, 0x1a); ASSERT_EQ(TVCframe.get(), nullptr) << "TVC 2 was sent after reconnection: " << BasicHNZServer::frameToStr(TVCframe); } + +TEST_F(HNZTest, MessageRejectedIfInvalidNR) { + ServersWrapper wrapper(0x05, getNextPort()); + BasicHNZServer* server = wrapper.server1().get(); + ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s..."; + validateAllTIQualityUpdate(true, false); + if(HasFatalFailure()) return; + + // Clear frames received + std::vector> frames = server->popLastFramesReceived(); + + // Send BULLE with invalid NR (NR-1) + BasicHNZServer::FrameError fe; + fe.nr_minus_1 = true; + server->sendFrame({0x13, 0x04}, false, fe); + debug_print("[HNZ Server] BULLE sent"); + this_thread::sleep_for(chrono::milliseconds(1000)); + + // Check that no RR frame was received + frames = server->popLastFramesReceived(); + std::shared_ptr RRframe = findRR(frames); + ASSERT_EQ(RRframe.get(), nullptr) << "RR was sent in response to BULLE with invalid NR: " << BasicHNZServer::frameToStr(RRframe); + + // Send BULLE with valid NR + server->sendFrame({0x13, 0x04}, false); + debug_print("[HNZ Server] BULLE 2 sent"); + this_thread::sleep_for(chrono::milliseconds(1000)); + + // Check that RR frame was received + frames = server->popLastFramesReceived(); + RRframe = findRR(frames); + ASSERT_EQ(RRframe.get(), nullptr) << "Could not find RR in frames received: " << BasicHNZServer::framesToStr(frames); + + // Send BULLE with invalid NR (NR+2) + BasicHNZServer::FrameError fe2; + fe2.nr_plus_2 = true; + server->sendFrame({0x13, 0x04}, false, fe2); + debug_print("[HNZ Server] BULLE 3 sent"); + this_thread::sleep_for(chrono::milliseconds(1000)); + + // Check that no RR frame was received + frames = server->popLastFramesReceived(); + RRframe = findRR(frames); + ASSERT_EQ(RRframe.get(), nullptr) << "RR was sent in response to BULLE 3 with invalid NR: " << BasicHNZServer::frameToStr(RRframe); +} From 95a60741e66428b545b0ee1c52d451837111e763 Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Tue, 10 Sep 2024 18:05:49 +0200 Subject: [PATCH 2/6] refs #PMP2-305 Updated unit tests to cover the expected cases. Fixed received NR validity calculation to match what is described in the HNZ protocol. Signed-off-by: Florent Peyrusse --- src/hnzpath.cpp | 22 +++++++++++++++++++--- tests/test_hnz.cpp | 6 +++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/hnzpath.cpp b/src/hnzpath.cpp index ab102cb..e0ba50c 100644 --- a/src/hnzpath.cpp +++ b/src/hnzpath.cpp @@ -487,9 +487,25 @@ void HNZPath::m_receivedBULLE() { m_last_msg_time = time(nullptr); } bool HNZPath::m_receivedRR(int nr, bool repetition) { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_receivedRR - " + m_name_log; + // We want to test (m_NRR <= nr <= m_ns) modulo 8 + // Case 0 (OK): m_NRR == nr <= m_ns if (nr != m_NRR) { - int frameOk = (nr - m_NRR + 7) % 8 + 1; - if (frameOk <= m_anticipation_ratio) { + // Case 1 (OK): m_NRR < nr <= m_ns + bool frameOk = (m_NRR < nr) && (nr <= m_ns); + if (m_ns < m_NRR) { + // Case 2 (OK): m_ns < m_NRR < nr (m_ns wrapped left by modulo 8) + if(m_NRR < nr) { + frameOk = true; + } + // Case 3 (OK): nr <= m_ns < m_NRR (m_NRR wrapped right by modulo 8) + else if (nr <= m_ns) { + frameOk = true; + } + // Case 4 (NOK): m_ns < nr < m_NRR (nr out of bounds) + } + // Case 5 (NOK): m_NRR < m_ns < nr (nr out of bounds) + // Case 6 (NOK): nr < m_NRR < m_ns (nr out of bounds) + if (frameOk) { if (!repetition || (m_repeat > 0)) { // valid NR, message(s) well received // remove them from msg sent list @@ -520,7 +536,7 @@ bool HNZPath::m_receivedRR(int nr, bool repetition) { } else { // invalid NR HnzUtility::log_warn(beforeLog + " Ignoring the RR, NR (" + to_string(nr) + ") is invalid. " + - "Current NRR : " + to_string(m_NRR + 1)); + "Current NRR: " + to_string(m_NRR) + ", Current VS: " + to_string(m_ns)); return false; } } diff --git a/tests/test_hnz.cpp b/tests/test_hnz.cpp index b68110b..703ed6d 100644 --- a/tests/test_hnz.cpp +++ b/tests/test_hnz.cpp @@ -2689,15 +2689,15 @@ TEST_F(HNZTest, MessageRejectedIfInvalidNR) { std::shared_ptr RRframe = findRR(frames); ASSERT_EQ(RRframe.get(), nullptr) << "RR was sent in response to BULLE with invalid NR: " << BasicHNZServer::frameToStr(RRframe); - // Send BULLE with valid NR - server->sendFrame({0x13, 0x04}, false); + // Send BULLE with valid NR (and repeat flag or else NS is invalid) + server->sendFrame({0x13, 0x04}, true); debug_print("[HNZ Server] BULLE 2 sent"); this_thread::sleep_for(chrono::milliseconds(1000)); // Check that RR frame was received frames = server->popLastFramesReceived(); RRframe = findRR(frames); - ASSERT_EQ(RRframe.get(), nullptr) << "Could not find RR in frames received: " << BasicHNZServer::framesToStr(frames); + ASSERT_NE(RRframe.get(), nullptr) << "Could not find RR in frames received: " << BasicHNZServer::framesToStr(frames); // Send BULLE with invalid NR (NR+2) BasicHNZServer::FrameError fe2; From 650ef94fd3a1b503a9a672487314c6eb7dbf9c21 Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Thu, 12 Sep 2024 14:43:25 +0200 Subject: [PATCH 3/6] refs #PMP2-305 Updated unit tests to improve code coverage. Fixed some random crashes at HNZ class destruction due to threads concurrency. Made state change only happen in one place for HNZPath. Signed-off-by: Florent Peyrusse --- include/hnzpath.h | 53 ++++--- src/hnz.cpp | 11 +- src/hnzconnection.cpp | 8 +- src/hnzpath.cpp | 86 ++++++------ tests/server/basic_hnz_server.cpp | 71 +++++++--- tests/server/basic_hnz_server.h | 12 +- tests/test_hnz.cpp | 225 +++++++++++++++++++++++++++--- tests/test_hnzconf.cpp | 2 +- 8 files changed, 339 insertions(+), 129 deletions(-) diff --git a/include/hnzpath.h b/include/hnzpath.h index e9999ae..62c0757 100644 --- a/include/hnzpath.h +++ b/include/hnzpath.h @@ -89,7 +89,10 @@ class HNZPath { * Is the HNZ connection with the PA established and still alive? * @return true if connected, false otherwise */ - bool isHNZConnected() { return (m_protocol_state == CONNECTED) && isConnected(); }; + bool isHNZConnected() { + std::lock_guard lock(m_protocol_state_mutex); + return (m_protocol_state == CONNECTED) && isConnected(); + }; /** * Is the TCP connection with the PA established and still alive? @@ -161,7 +164,10 @@ class HNZPath { * Gets the state of the HNZ protocol (CONNECTION, CONNECTED) * @return CONNECTION if SARM/UA step is not complete, CONNECTED after that */ - int getProtocolState() const { return m_protocol_state; } + int getProtocolState() const { + std::lock_guard lock(m_protocol_state_mutex); + return m_protocol_state; + } private: std::unique_ptr m_hnz_client; // HNZ Client that manage TCP connection @@ -173,54 +179,55 @@ class HNZPath { list command_sent; // List of command already sent waiting to be ack - long last_sent_time; // Timestamp of the last message sent - int repeat_max; // max number of authorized repeats + long last_sent_time = 0; // Timestamp of the last message sent + int repeat_max = 0; // max number of authorized repeats int gi_repeat = 0; // number of time a GI is repeated long gi_start_time = 0; // GI start time std::shared_ptr m_connection_thread; // Main thread that maintains the connection + std::mutex m_connection_thread_mutex; // mutex to protect changes in m_connection_thread atomic m_is_running{true}; // If false, the connection thread will stop atomic m_connected{false}; // TCP Connection state with the PA // Initializing to CONNECTED ensures that the initial state transition from go_to_connection generates an audit int m_protocol_state = CONNECTED; // HNZ Protocol connection state + mutable std::recursive_mutex m_protocol_state_mutex; // mutex to protect changes in m_protocol_state bool m_is_active_path = false; // Plugin configuration string m_ip; // IP of the PA - int m_port; // Port to connect to - long long int m_timeoutUs; // Timeout for socket recv in microseconds + int m_port = 0; // Port to connect to + long long int m_timeoutUs = 0; // Timeout for socket recv in microseconds string m_name_log; // Path name used in log string m_path_letter; // Path letter string m_path_name; // Path name - unsigned int m_remote_address; - unsigned char m_address_PA; // remote address + 1 - unsigned char m_address_ARP; // remote address + 3 + unsigned int m_remote_address = 0; + unsigned char m_address_PA = 0; // remote address + 1 + unsigned char m_address_ARP = 0; // remote address + 3 - int m_max_sarm; // max number of SARM messages before handing over to the + int m_max_sarm = 0; // max number of SARM messages before handing over to the // passive path - int m_inacc_timeout; // timeout before declaring the remote server + int m_inacc_timeout = 0; // timeout before declaring the remote server // unreachable - int m_repeat_timeout; // time allowed for the receiver to acknowledge a frame - int m_anticipation_ratio; // number of frames allowed to be received without + int m_repeat_timeout = 0; // time allowed for the receiver to acknowledge a frame + int m_anticipation_ratio = 0; // number of frames allowed to be received without // acknowledgement BulleFormat m_test_msg_receive; // Payload of received BULLE BulleFormat m_test_msg_send; // Payload of sent BULLE - int c_ack_time_max; // Max time to wait before receving a acknowledgement for + int c_ack_time_max = 0; // Max time to wait before receving a acknowledgement for // a control command (in ms) // HNZ protocol related variable - int m_nr; // Number in reception - int m_ns; // Number in sending - int m_NRR; // Received aquit number - int module10M; - long m_last_msg_time; // Timestamp of the last reception - bool sarm_PA_received; // The SARM sent by the PA was received - bool sarm_ARP_UA; // The UA sent by the PA (after receiving our SARM) was + int m_nr = 0; // Number in reception + int m_ns = 0; // Number in sending + int m_NRR = 0; // Received aquit number + long m_last_msg_time = 0; // Timestamp of the last reception + bool sarm_PA_received = false; // The SARM sent by the PA was received + bool sarm_ARP_UA = false; // The UA sent by the PA (after receiving our SARM) was // received - int m_nbr_sarm_sent; // Number of SARM sent - int m_repeat; // Number of times the sent message is repeated + int m_nbr_sarm_sent = 0; // Number of SARM sent + int m_repeat = 0; // Number of times the sent message is repeated /** * Manage the HNZ protocol connection with the PA. Be careful, it doesn't diff --git a/src/hnz.cpp b/src/hnz.cpp index cdde1db..23e5c68 100644 --- a/src/hnz.cpp +++ b/src/hnz.cpp @@ -168,8 +168,7 @@ bool HNZ::setJsonConfig(const string& protocol_conf_json, const string& msg_conf } void HNZ::receive(std::shared_ptr hnz_path_in_use) { - if (m_hnz_conf) { - // Parent if used only for scope lock + { std::lock_guard guard(m_configMutex); if (!m_hnz_conf->is_complete()) { return; @@ -606,9 +605,7 @@ bool HNZ::operation(const std::string& operation, int count, PLUGIN_PARAMETER** std::string beforeLog = HnzUtility::NamePlugin + " - HNZ::operation -"; HnzUtility::log_info("%s Operation %s: %s", beforeLog.c_str(), operation.c_str(), paramsToStr(params, count).c_str()); - // Workaround until the following ticket is fixed: https://github.com/fledge-iot/fledge/issues/1239 - // if (operation == "HNZCommand") { - if (endsWith(operation, "Command")) { + if (operation == "HNZCommand") { int res = processCommandOperation(count, params); if(res == 0) { // Only return on success so that all parameters are displayed by final error log in case of syntax error @@ -644,10 +641,6 @@ int HNZ::processCommandOperation(int count, PLUGIN_PARAMETER** params) { const std::string& paramValue = params[i]->value; if (commandParams.count(paramName) > 0) { commandParams[paramName] = paramValue; - // Workaround until the following ticket is fixed: https://github.com/fledge-iot/fledge/issues/1240 - if(paramValue.at(0) == '"'){ - commandParams[paramName] = paramValue.substr(1,paramValue.length()-2); - } } else { HnzUtility::log_warn("%s Unknown parameter '%s' in HNZCommand", beforeLog.c_str(), paramName.c_str()); diff --git a/src/hnzconnection.cpp b/src/hnzconnection.cpp index e8b2fd4..f9992db 100644 --- a/src/hnzconnection.cpp +++ b/src/hnzconnection.cpp @@ -71,8 +71,7 @@ void HNZConnection::stop() { // Stop the path used (close the TCP connection and stop the threads that // manage HNZ connections) - if (m_active_path != nullptr || m_passive_path != nullptr) { - // Parent if is mostly here for scope lock + { std::lock_guard lock(m_path_mutex); if (m_active_path != nullptr) m_active_path->disconnect(); if (m_passive_path != nullptr) m_passive_path->disconnect(); @@ -90,8 +89,8 @@ void HNZConnection::stop() { } void HNZConnection::checkGICompleted(bool success) { - std::lock_guard lock(m_path_mutex); std::string beforeLog = HnzUtility::NamePlugin + " - HNZConnection::checkGICompleted -"; + std::lock_guard lock(m_path_mutex); // GI is a success if (success) { @@ -140,8 +139,7 @@ void HNZConnection::m_manageMessages() { m_update_current_time(); // Manage repeat/timeout for each path - if (m_active_path || m_passive_path) { - // Parent if is mostly here for scope lock + { std::lock_guard lock(m_path_mutex); m_check_timer(m_active_path); m_check_timer(m_passive_path); diff --git a/src/hnzpath.cpp b/src/hnzpath.cpp index e0ba50c..7ed3036 100644 --- a/src/hnzpath.cpp +++ b/src/hnzpath.cpp @@ -64,9 +64,6 @@ vector convertPayloadToVector(unsigned char* data, int size) { * Helper method to convert payload into something readable for logs. */ std::string convert_data_to_str(unsigned char* data, int len) { - if (data == nullptr) { - return ""; - } std::stringstream stream; for (int i = 0; i < len; i++) { if (i > 0) { @@ -126,6 +123,7 @@ void HNZPath::connect() { if (m_connected) { HnzUtility::log_info(beforeLog + " Connected to " + m_ip + " (" + to_string(m_port) + ")."); go_to_connection(); + std::lock_guard lock(m_connection_thread_mutex); if (m_connection_thread == nullptr) { // Start the thread that manage the HNZ connection m_connection_thread = std::make_shared(&HNZPath::m_manageHNZProtocolConnection, this); @@ -154,13 +152,13 @@ void HNZPath::disconnect() { m_connected = false; m_hnz_client->stop(); + HnzUtility::log_debug(beforeLog + " HNZ client stopped"); + + std::lock_guard lock(m_connection_thread_mutex); if (m_connection_thread != nullptr) { - // To avoid to be here at the same time, we put m_connection_thread = - // nullptr - std::shared_ptr temp = m_connection_thread; + HnzUtility::log_debug(beforeLog + " Waiting for the connection thread..."); + m_connection_thread->join(); m_connection_thread = nullptr; - HnzUtility::log_debug(beforeLog + " Waiting for the connection thread"); - temp->join(); } HnzUtility::log_info(beforeLog + " stopped !"); @@ -174,20 +172,20 @@ void HNZPath::m_manageHNZProtocolConnection() { HnzUtility::log_debug(beforeLog + " HNZ Connection Management thread running"); do { - now = time(nullptr); - - switch (m_protocol_state) { - case CONNECTION: - sleep = m_manageHNZProtocolConnecting(now); - break; - case CONNECTED: - sleep = m_manageHNZProtocolConnected(now); - break; - default: - HnzUtility::log_debug(beforeLog + " STOP state"); - m_is_running = false; - sleep = milliseconds(10); - break; + { + // Here m_path_mutex might be locked within the scope of m_protocol_state_mutex lock, so lock both to avoid deadlocks + std::lock(m_protocol_state_mutex, m_hnz_connection->getPathMutex()); // Lock both mutexes simultaneously + std::lock_guard lock(m_protocol_state_mutex, std::adopt_lock); + std::lock_guard lock2(m_hnz_connection->getPathMutex(), std::adopt_lock); + now = time(nullptr); + switch (m_protocol_state) { + case CONNECTION: + sleep = m_manageHNZProtocolConnecting(now); + break; + case CONNECTED: + sleep = m_manageHNZProtocolConnected(now); + break; + } } this_thread::sleep_for(sleep); @@ -220,13 +218,6 @@ milliseconds HNZPath::m_manageHNZProtocolConnecting(long now) { m_connected = false; // Reconnection will be done in HNZ::receive } - } else { - m_protocol_state = CONNECTED; - std::lock_guard lock(m_hnz_connection->getPathMutex()); - if (m_is_active_path) { - m_hnz_connection->updateConnectionStatus(ConnectionStatus::STARTED); - } - sleep = milliseconds(10); } return sleep; } @@ -247,7 +238,10 @@ milliseconds HNZPath::m_manageHNZProtocolConnected(long now) { void HNZPath::go_to_connection() { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::go_to_connection - " + m_name_log; - HnzUtility::log_info(beforeLog + " Going to HNZ connection state... Waiting for a SARM."); + // Here m_path_mutex might be locked within the scope of m_protocol_state_mutex lock, so lock both to avoid deadlocks + std::lock(m_protocol_state_mutex, m_hnz_connection->getPathMutex()); // Lock both mutexes simultaneously + std::lock_guard lock(m_protocol_state_mutex, std::adopt_lock); + std::lock_guard lock2(m_hnz_connection->getPathMutex(), std::adopt_lock); if (m_protocol_state != CONNECTION) { m_protocol_state = CONNECTION; // Send audit for path connection status @@ -296,11 +290,15 @@ void HNZPath::setActivePath(bool active) { void HNZPath::m_go_to_connected() { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_go_to_connected - " + m_name_log; - std::lock_guard lock(m_hnz_connection->getPathMutex()); - m_protocol_state = CONNECTED; - // Send audit for path connection status - std::string activePassive = m_is_active_path ? "active" : "passive"; - HnzUtility::audit_success("SRVFL", m_hnz_connection->getServiceName() + "-" + m_path_letter + "-" + activePassive); + std::lock(m_protocol_state_mutex, m_hnz_connection->getPathMutex()); // Lock both mutexes simultaneously + std::lock_guard lock(m_protocol_state_mutex, std::adopt_lock); + std::lock_guard lock2(m_hnz_connection->getPathMutex(), std::adopt_lock); + if (m_protocol_state != CONNECTED) { + m_protocol_state = CONNECTED; + // Send audit for path connection status + std::string activePassive = m_is_active_path ? "active" : "passive"; + HnzUtility::audit_success("SRVFL", m_hnz_connection->getServiceName() + "-" + m_path_letter + "-" + activePassive); + } if (m_is_active_path) { m_hnz_connection->updateConnectionStatus(ConnectionStatus::STARTED); } @@ -356,6 +354,10 @@ vector> HNZPath::m_analyze_frame(MSG_TRAME* frReceived) { m_receivedSARM(); break; default: + // Here m_path_mutex might be locked within the scope of m_protocol_state_mutex lock, so lock both to avoid deadlocks + std::lock(m_protocol_state_mutex, m_hnz_connection->getPathMutex()); // Lock both mutexes simultaneously + std::lock_guard lock(m_protocol_state_mutex, std::adopt_lock); + std::lock_guard lock2(m_hnz_connection->getPathMutex(), std::adopt_lock); if (m_protocol_state != CONNECTION) { // Get NR, P/F ans NS field int ns = (type >> 1) & 0x07; @@ -365,8 +367,7 @@ vector> HNZPath::m_analyze_frame(MSG_TRAME* frReceived) { // Information frame HnzUtility::log_info(beforeLog + " Received an information frame (ns = " + to_string(ns) + ", p = " + to_string(pf) + ", nr = " + to_string(nr) + ")"); - - std::lock_guard lock(m_hnz_connection->getPathMutex()); + std::lock_guard lock3(m_hnz_connection->getPathMutex()); if (m_is_active_path) { // Only the messages on the active path are extracted. The // passive path does not need them. @@ -420,7 +421,6 @@ vector> HNZPath::m_extract_messages(unsigned char* data, i len = 7; break; case MODULO_CODE: - module10M = (int)data[1]; HnzUtility::log_info(beforeLog + " Received Modulo 10mn"); len = 2; break; @@ -440,6 +440,7 @@ vector> HNZPath::m_extract_messages(unsigned char* data, i len = 2; } else { HnzUtility::log_info(beforeLog + "Received an unknown type"); + len = payloadSize; } break; } @@ -464,17 +465,20 @@ vector> HNZPath::m_extract_messages(unsigned char* data, i } void HNZPath::m_receivedSARM() { + std::lock_guard lock(m_protocol_state_mutex); if (m_protocol_state == CONNECTED) { // Reset HNZ protocol variables go_to_connection(); } sarm_PA_received = true; - sarm_ARP_UA = false; m_sendUA(); - module10M = 0; + if (sarm_ARP_UA) { + m_go_to_connected(); + } } void HNZPath::m_receivedUA() { + std::lock_guard lock(m_protocol_state_mutex); if (m_protocol_state == CONNECTION) { sarm_ARP_UA = true; if (sarm_PA_received) { @@ -609,6 +613,7 @@ bool HNZPath::m_sendRR(bool repetition, int ns, int nr) { bool HNZPath::m_sendInfo(unsigned char* msg, unsigned long size) { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_sendInfo - " + m_name_log; + std::lock_guard lock(m_protocol_state_mutex); if (m_protocol_state != CONNECTED) { HnzUtility::log_debug(beforeLog + " Connection is not yet fully established, discarding message [" + convert_data_to_str(msg, static_cast(size)) + "]"); @@ -633,6 +638,7 @@ bool HNZPath::m_sendInfoImmediately(Message message) { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_sendInfoImmediately - " + m_name_log; unsigned char* msg = &message.payload[0]; int size = message.payload.size(); + std::lock_guard lock(m_protocol_state_mutex); if (m_protocol_state != CONNECTED) { HnzUtility::log_debug(beforeLog + " Connection is not yet fully established, discarding message [" + convert_data_to_str(msg, size) + "]"); diff --git a/tests/server/basic_hnz_server.cpp b/tests/server/basic_hnz_server.cpp index 04e2b75..10e7c6f 100644 --- a/tests/server/basic_hnz_server.cpp +++ b/tests/server/basic_hnz_server.cpp @@ -12,6 +12,11 @@ void BasicHNZServer::startHNZServer() { if (server == nullptr) { server = new HNZServer(); } + { + std::lock_guard guard(m_sarm_ua_mutex); + ua_ok = false; + sarm_ok = false; + } std::lock_guard guard(m_t1_mutex); m_t1 = new thread(&BasicHNZServer::m_start, server, m_port); } @@ -98,6 +103,9 @@ bool BasicHNZServer::stopHNZServer() { } void BasicHNZServer::receiving_loop() { + // Reset NS/NR variables + m_ns = 0; + m_nr = 0; while (is_running) { // Receive an hnz frame, this call is blocking MSG_TRAME *frReceived = (server->receiveFr()); @@ -155,19 +163,10 @@ void BasicHNZServer::receiving_loop() { } void BasicHNZServer::sendSARMLoop() { - // Reset SARM/UA variables in case of reconnect - { - std::lock_guard guard(m_sarm_ua_mutex); - ua_ok = false; - sarm_ok = false; - } bool sarm_ua_ok = false; while (is_running && !sarm_ua_ok) { if(server->isConnected()) { sendSARM(); - std::lock_guard guard(m_sarm_ua_mutex); - ua_ok = false; - sarm_ok = false; } this_thread::sleep_for(chrono::milliseconds(3000)); std::lock_guard guard(m_sarm_ua_mutex); @@ -191,7 +190,13 @@ bool BasicHNZServer::waitForTCPConnection(int timeout_s) { return true; } -bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { +bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/, bool sendSarm /*= true*/, bool delaySarm /*= false*/) { + // Reset SARM/UA variables in case of reconnect + { + std::lock_guard guard(m_sarm_ua_mutex); + ua_ok = false; + sarm_ok = false; + } // Lock to prevent multiple calls to this function in parallel std::lock_guard guard(m_init_mutex); if (receiving_thread) { @@ -210,15 +215,20 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { return false; } // Loop that sending sarm every 3s - thread *m_t2 = new thread(&BasicHNZServer::sendSARMLoop, this); - this_thread::sleep_for(chrono::milliseconds(100)); + thread *m_t2 = nullptr; + if (sendSarm && !delaySarm) { + m_t2 = new thread(&BasicHNZServer::sendSARMLoop, this); + this_thread::sleep_for(chrono::milliseconds(100)); + } // Wait for UA and send UA in response of SARM start = time(NULL); + int nbReconnect = 0; + int maxReconnect = 10; bool lastFrameWasEmpty = false; // Make sure to always exit this loop with is_running==false, not return // to ensure that m_t2 is joined properly while (is_running) { - if (time(NULL) - start > timeout_s) { + if ((time(NULL) - start > timeout_s) || (nbReconnect >= maxReconnect)) { printf("[HNZ Server][%d] SARM/UA timeout\n", m_port); fflush(stdout); is_running = false; break; @@ -237,6 +247,7 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { break; } start = time(NULL); + nbReconnect++; printf("[HNZ Server][%d] Server reconnected!\n", m_port); fflush(stdout); } @@ -268,6 +279,11 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { std::lock_guard guard2(m_sarm_ua_mutex); sarm_ok = true; } + // If SARM delayed, only start sending it after SARM was received + if (delaySarm && (m_t2 == nullptr)) { + m_t2 = new thread(&BasicHNZServer::sendSARMLoop, this); + this_thread::sleep_for(chrono::milliseconds(100)); + } break; default: printf("[HNZ Server][%d] Neither UA nor SARM: %d\n", m_port, static_cast(c)); fflush(stdout); @@ -276,7 +292,7 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { // Store the frame that was received for testing purposes onFrameReceived(frReceived); std::lock_guard guard2(m_sarm_ua_mutex); - if (ua_ok && sarm_ok) break; + if (server->isConnected() && ua_ok && sarm_ok) break; } if (!is_running) { printf("[HNZ Server][%d] Not running after SARM/UA, exit\n", m_port); fflush(stdout); @@ -285,14 +301,14 @@ bool BasicHNZServer::HNZServerIsReady(int timeout_s /*= 16*/) { printf("[HNZ Server][%d] Connection OK !!\n", m_port); fflush(stdout); // Connection established - m_ns = 0; - m_nr = 0; receiving_thread = new thread(&BasicHNZServer::receiving_loop, this); - // Join sarm thread after starting receiving loop as it may wait up to 3 seconds and some messages received will be missed (no RR sent) - m_t2->join(); - delete m_t2; - m_t2 = nullptr; + // Join SARM thread after starting receiving loop as it may wait up to 3 seconds and some messages received will be missed (no RR sent) + if (m_t2 != nullptr) { + m_t2->join(); + delete m_t2; + m_t2 = nullptr; + } return true; } @@ -303,7 +319,7 @@ void BasicHNZServer::sendSARM() { createAndSendFrame(0x05, message, sizeof(message)); } -void BasicHNZServer::sendFrame(vector message, bool repeat, FrameError frameError /*= {}*/) { +void BasicHNZServer::sendFrame(vector message, bool repeat, const FrameError& frameError /*= {}*/) { int p = repeat ? 1 : 0; int nr = m_nr; if (frameError.nr_minus_1) { @@ -319,11 +335,17 @@ void BasicHNZServer::sendFrame(vector message, bool repeat, Frame int num = (nr << 5) + (p << 4) + (ns << 1); message.insert(message.begin(), num); int len = message.size(); - createAndSendFrame(addr, message.data(), len); + int addressByte = addr; + if (frameError.address) { + int address = addressByte >> 2; + address = (address + 1) % 64; + addressByte = (address << 2) + (addressByte & 0x03); + } + createAndSendFrame(addressByte, message.data(), len, frameError); if (!repeat) m_ns++; } -void BasicHNZServer::createAndSendFrame(unsigned char addr, unsigned char *msg, int msgSize) { +void BasicHNZServer::createAndSendFrame(unsigned char addr, unsigned char *msg, int msgSize, const FrameError& frameError /*= {}*/) { // Code extracted from HNZClient::createAndSendFr of libhnz MSG_TRAME* pTrame = new MSG_TRAME; unsigned char msgWithAddr[msgSize + 1]; @@ -333,6 +355,9 @@ void BasicHNZServer::createAndSendFrame(unsigned char addr, unsigned char *msg, memcpy(msgWithAddr + 1, msg, msgSize); server->addMsgToFr(pTrame, msgWithAddr, sizeof(msgWithAddr)); server->setCRC(pTrame); + if (frameError.fcs) { + pTrame->aubTrame[pTrame->usLgBuffer-1]^=0xFF; // Invert all bits on one byte of the FCS to make it invalid + } // Store the frame about to be sent for testing purposes onFrameSent(pTrame); server->sendFr(pTrame); diff --git a/tests/server/basic_hnz_server.h b/tests/server/basic_hnz_server.h index 726148d..5e48377 100644 --- a/tests/server/basic_hnz_server.h +++ b/tests/server/basic_hnz_server.h @@ -23,19 +23,21 @@ class BasicHNZServer { bool waitForTCPConnection(int timeout_s); // Timeout = 16 = (5 * 3) + 1 sec = (SARM retries * SARM delay) + 1 - bool HNZServerIsReady(int timeout_s = 16); + bool HNZServerIsReady(int timeout_s = 16, bool sendSarm = true, bool delaySarm = false); void sendSARM(); struct FrameError { bool nr_minus_1; bool nr_plus_2; + bool fcs; + bool address; // Use constructor instead of default values for bracket initializer to work in C++11 - FrameError(bool _nr_minus_1 = false, bool _nr_plus_2 = false): - nr_minus_1(_nr_minus_1), nr_plus_2(_nr_plus_2) {} + FrameError(bool _nr_minus_1 = false, bool _nr_plus_2 = false, bool _fcs = false, bool _address = false): + nr_minus_1(_nr_minus_1), nr_plus_2(_nr_plus_2), fcs(_fcs), address(_address) {} }; - void sendFrame(vector message, bool repeat, FrameError frameError = {}); - void createAndSendFrame(unsigned char addr, unsigned char *msg, int msgSize); + void sendFrame(vector message, bool repeat, const FrameError& frameError = {}); + void createAndSendFrame(unsigned char addr, unsigned char *msg, int msgSize, const FrameError& frameError = {}); // Note: return the strcutre by value becase a copy must be done by the caller to remain thread safe std::vector> popLastFramesReceived(); std::vector> popLastFramesSent(); diff --git a/tests/test_hnz.cpp b/tests/test_hnz.cpp index 703ed6d..a4d83bb 100644 --- a/tests/test_hnz.cpp +++ b/tests/test_hnz.cpp @@ -189,7 +189,7 @@ class HNZTest : public testing::Test { southEventsReceived = 0; } - static void initConfig(int port, int port2) { + static void initCustomConfig(int port, int port2, const std::string& protocol_stack, const std::string& exchanged_data) { static const std::string configureTemplate = QUOTE({ "enable" : { "value": "true" @@ -201,15 +201,19 @@ class HNZTest : public testing::Test { "value": } }); - const std::string& protocol_stack = protocol_stack_generator(port, port2); std::string configure = std::regex_replace(configureTemplate, std::regex(""), protocol_stack); - configure = std::regex_replace(configure, std::regex(""), exchanged_data_def); + configure = std::regex_replace(configure, std::regex(""), exchanged_data); ConfigCategory config("newConfig", configure); hnz->reconfigure(config); } - static void startHNZ(int port, int port2) { - initConfig(port, port2); + static void initConfig(int port, int port2, const std::string& protocol_stack = "", const std::string& exchanged_data = "") { + const std::string& protocol_stack_conf = protocol_stack.empty() ? protocol_stack_generator(port, port2) : protocol_stack; + const std::string& exchanged_data_conf = exchanged_data.empty() ? exchanged_data_def : exchanged_data; + initCustomConfig(port, port2, protocol_stack_conf, exchanged_data_conf); + } + + static void startHNZ(int port, int port2, const std::string& protocol_stack = "", const std::string& exchanged_data = "") { hnz->start(true); } @@ -664,20 +668,23 @@ class ServersWrapper { startHNZPlugin(); } } - void initHNZPlugin() { - HNZTest::initConfig(m_port1, m_port2); + void initHNZPlugin(const std::string& protocol_stack = "", const std::string& exchanged_data = "") { + HNZTest::initConfig(m_port1, m_port2, protocol_stack, exchanged_data); } - void startHNZPlugin() { - HNZTest::startHNZ(m_port1, m_port2); + void startHNZPlugin(bool config = true, const std::string& protocol_stack = "", const std::string& exchanged_data = "") { + if (config) { + HNZTest::initConfig(m_port1, m_port2, protocol_stack, exchanged_data); + } + HNZTest::startHNZ(m_port1, m_port2, protocol_stack, exchanged_data); } - std::shared_ptr server1() { - if (m_server1 && !m_server1->HNZServerIsReady()) { + std::shared_ptr server1(bool sendSarm = true, bool delaySarm = false) { + if (m_server1 && !m_server1->HNZServerIsReady(16, sendSarm, delaySarm)) { return nullptr; } return m_server1; } - std::shared_ptr server2() { - if (m_server2 && !m_server2->HNZServerIsReady()) { + std::shared_ptr server2(bool sendSarm = true, bool delaySarm = false) { + if (m_server2 && !m_server2->HNZServerIsReady(16, sendSarm, delaySarm)) { return nullptr; } return m_server2; @@ -2177,21 +2184,16 @@ TEST_F(HNZTest, ReconfigureWhileConnectionActive) { } TEST_F(HNZTest, ReconfigureBadConfig) { - ServersWrapper wrapper(0x05, getNextPort()); + int port = getNextPort(); + ServersWrapper wrapper(0x05, port); BasicHNZServer* server = wrapper.server1().get(); ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s..."; validateAllTIQualityUpdate(true, false); if(HasFatalFailure()) return; - debug_print("[HNZ south plugin] Send bad plugin configuration"); + debug_print("[HNZ south plugin] Send bad plugin configuration (exchanged_data)"); clearReadings(); - static const std::string badConfig = QUOTE({ - "exchanged_data" : { - "value": 42 - } - }); - ConfigCategory config("newConfig", badConfig); - hnz->reconfigure(config); + wrapper.initHNZPlugin("", "42"); // Check that connection was lost waitUntil(southEventsReceived, 1, 1000); @@ -2272,6 +2274,27 @@ TEST_F(HNZTest, ReconfigureBadConfig) { }); if(HasFatalFailure()) return; + // Send configuration with bad "connections" array definition + debug_print("[HNZ south plugin] Send bad plugin configuration (protocol_stack)"); + clearReadings(); + const std::string& protocol_stack = "{ \"protocol_stack\" : { \"name\" : \"hnzclient\", \"version\" : " + "\"1.0\", \"transport_layer\" : { \"connections\" : 42 } , " + "\"application_layer\" : { \"repeat_timeout\" : 3000, \"repeat_path_A\" : 3," + "\"remote_station_addr\" : 1, \"max_sarm\" : 5, \"gi_time\" : 1, \"gi_repeat_count\" : 2," + "\"anticipation_ratio\" : 5 }, \"south_monitoring\" : { \"asset\" : \"TEST_STATUS\" } } }"; + wrapper.initHNZPlugin(protocol_stack); + + // Check that connection was lost + waitUntil(southEventsReceived, 1, 1000); + // Check that ingestCallback had been called only one time + ASSERT_EQ(southEventsReceived, 1); + // Validate new connection state + currentReading = popFrontReadingsUntil("TEST_STATUS"); + validateSouthEvent(currentReading, "TEST_STATUS", { + {"connx_status", "not connected"}, + }); + if(HasFatalFailure()) return; + // Manually stop the server here or we may end up in a deadlock in the HNZServer debug_print("[HNZ server] Request server stop 3..."); ASSERT_TRUE(server->stopHNZServer()); @@ -2291,6 +2314,24 @@ TEST_F(HNZTest, UnknownMessage) { // Check that no message was received ASSERT_EQ(ingestCallbackCalled, 0); + // Send an TSCE with wrong FCS + BasicHNZServer::FrameError fe; + fe.fcs = true; + server->sendFrame({0x0B, 0x33, 0x28, 0x00, 0x00}, false, fe); + debug_print("[HNZ Server] TSCE with bad FCS sent"); + this_thread::sleep_for(chrono::milliseconds(1000)); + // Check that no message was received + ASSERT_EQ(ingestCallbackCalled, 0); + + // Send an TSCE with wrong address + BasicHNZServer::FrameError fe2; + fe2.address = true; + server->sendFrame({0x0B, 0x33, 0x28, 0x00, 0x00}, false, fe2); + debug_print("[HNZ Server] TSCE with bad addr sent"); + this_thread::sleep_for(chrono::milliseconds(1000)); + // Check that no message was received + ASSERT_EQ(ingestCallbackCalled, 0); + // Send an unknown TSCE server->sendFrame({0x0B, 0xff, 0x28, 0x00, 0x00}, false); debug_print("[HNZ Server] Unknown TSCE sent"); @@ -2508,12 +2549,17 @@ TEST_F(HNZTest, FrameToStr) { } TEST_F(HNZTest, BackToSARM) { - ServersWrapper wrapper(0x05, getNextPort()); + int port = getNextPort(); + ServersWrapper wrapper(0x05, port); BasicHNZServer* server = wrapper.server1().get(); ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s..."; validateAllTIQualityUpdate(true, false); if(HasFatalFailure()) return; + ///////////////////////////// + // Back to SARM after (repeat_timeout * repeat_path_A) due to missing RR + ///////////////////////////// + // Stop sending automatic ack (RR) in response to messages from south plugin server->disableAcks(true); @@ -2538,6 +2584,70 @@ TEST_F(HNZTest, BackToSARM) { std::vector> frames = server->popLastFramesReceived(); std::shared_ptr SARMframe = findProtocolFrameWithId(frames, 0x0f); ASSERT_NE(SARMframe.get(), nullptr) << "Could not find SARM in frames received: " << BasicHNZServer::framesToStr(frames); + + ///////////////////////////// + // Back to SARM after inacc_timeout while connected + ///////////////////////////// + + // Enable acks again + server->disableAcks(false); + + // Reconfigure plugin with inacc_timeout = 1s + std::string protocol_stack = protocol_stack_generator(port, 0); + protocol_stack = std::regex_replace(protocol_stack, std::regex("\"anticipation_ratio\" : 5"), "\"anticipation_ratio\" : 5, \"inacc_timeout\" : 4"); + wrapper.initHNZPlugin(protocol_stack); + + // Also stop the server as it is unable to reconnect on the fly + debug_print("[HNZ server] Request server stop..."); + ASSERT_TRUE(server->stopHNZServer()); + this_thread::sleep_for(chrono::milliseconds(1000)); + debug_print("[HNZ server] Request server start..."); + server->startHNZServer(); + + // Check that the server is reconnected after reconfigure + server = wrapper.server1().get(); + ASSERT_NE(server, nullptr) << "Something went wrong. Connection 2 is not established in 10s..."; + + // Clear messages received from south plugin + server->popLastFramesReceived(); + // Wait inacc_timeout + debug_print("[HNZ server] Waiting for inacc timeout 2..."); + this_thread::sleep_for(chrono::seconds(10)); + + // Find the SARM frame in the list of frames received by server + frames = server->popLastFramesReceived(); + SARMframe = findProtocolFrameWithId(frames, 0x0f); + ASSERT_NE(SARMframe.get(), nullptr) << "Could not find SARM 2 in frames received: " << BasicHNZServer::framesToStr(frames); + + ///////////////////////////// + // Connection reset after inacc_timeout while connecting + ///////////////////////////// + + // Also stop the server as it is unable to reconnect on the fly + debug_print("[HNZ server] Request server stop 3..."); + ASSERT_TRUE(server->stopHNZServer()); + this_thread::sleep_for(chrono::milliseconds(1000)); + debug_print("[HNZ server] Request server start 3..."); + server->startHNZServer(); + + // Wait for inacc_timeout + debug_print("[HNZ server] Waiting for inacc timeout 3..."); + + // Establish TCP connection without sending any SARM + // Check that the server connection could not be established + BasicHNZServer* tmpServer = wrapper.server1(false).get(); + ASSERT_EQ(tmpServer, nullptr) << "Something went wrong. Connection 3 was established when it shouldn't"; + + // Check that normal connection can still be established later + debug_print("[HNZ server] Request server stop 4..."); + ASSERT_TRUE(server->stopHNZServer()); + this_thread::sleep_for(chrono::milliseconds(1000)); + debug_print("[HNZ server] Request server start 4..."); + server->startHNZServer(); + + // Check that the server is reconnected after reconfigure + server = wrapper.server1().get(); + ASSERT_NE(server, nullptr) << "Something went wrong. Connection 4 is not established in 10s..."; } TEST_F(HNZTest, NoMessageBufferedIfConnectionLost) { @@ -2711,3 +2821,72 @@ TEST_F(HNZTest, MessageRejectedIfInvalidNR) { RRframe = findRR(frames); ASSERT_EQ(RRframe.get(), nullptr) << "RR was sent in response to BULLE 3 with invalid NR: " << BasicHNZServer::frameToStr(RRframe); } + +TEST_F(HNZTest, StartAlreadyStarted) { + ServersWrapper wrapper(0x05, getNextPort()); + BasicHNZServer* server = wrapper.server1().get(); + ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s..."; + validateAllTIQualityUpdate(true, false); + if(HasFatalFailure()) return; + + // Start plugin again (without config init) + debug_print("[HNZ south plugin] Second start"); + wrapper.startHNZPlugin(false); + this_thread::sleep_for(chrono::milliseconds(1000)); + + // Validate that no message was sent + ASSERT_EQ(ingestCallbackCalled, 0); +} + +TEST_F(HNZTest, MultipleMessagesInOne) { + ServersWrapper wrapper(0x05, getNextPort()); + BasicHNZServer* server = wrapper.server1().get(); + ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s..."; + validateAllTIQualityUpdate(true, false); + if(HasFatalFailure()) return; + + hnz->sendCG(); + debug_print("[HNZ south plugin] CG request sent"); + this_thread::sleep_for(chrono::milliseconds(500)); // must not be too close to a multiple of gi_time + + // Find the CG frame in the list of frames received by server and validate it + validateFrame(server->popLastFramesReceived(), {0x13, 0x01}); + if(HasFatalFailure()) return; + + // Send both TSCG in the same frame + server->sendFrame({0x16, 0x33, 0x10, 0x00, 0x04, 0x00, 0x16, 0x39, 0x00, 0x01, 0x00, 0x00}, false); + debug_print("[HNZ Server] TSCG 2 in 1 sent"); + this_thread::sleep_for(chrono::milliseconds(1200)); // gi_time + 200ms + + // All TS were received so no more CG should be sent automatically any more + std::vector> frames = server->popLastFramesReceived(); + std::shared_ptr CGframe = findFrameWithId(frames, 0x13); + ASSERT_EQ(CGframe.get(), nullptr) << "No CG frame should be sent after all TS were received, but found: " << BasicHNZServer::frameToStr(CGframe); + + // Check that ingestCallback had been called + ASSERT_EQ(dataObjectsReceived, 3); + resetCounters(); + std::shared_ptr currentReading; + for (int i = 0; i < 3; i++) { + std::string label("TS" + to_string(i + 1)); + currentReading = popFrontReadingsUntil(label); + validateReading(currentReading, label, { + {"do_type", {"string", "TS"}}, + {"do_station", {"int64_t", "1"}}, + {"do_addr", {"int64_t", addrByTS[label]}}, + {"do_value", {"int64_t", "1"}}, + {"do_valid", {"int64_t", "0"}}, + {"do_cg", {"int64_t", "1"}}, + {"do_outdated", {"int64_t", "0"}}, + }); + if(HasFatalFailure()) return; + } +} + +TEST_F(HNZTest, ConnectIfSARMReceivedAfterUA) { + ServersWrapper wrapper(0x05, getNextPort()); + BasicHNZServer* server = wrapper.server1(true, true).get(); + ASSERT_NE(server, nullptr) << "Something went wrong. Connection is not established in 10s..."; + validateAllTIQualityUpdate(true, false); + if(HasFatalFailure()) return; +} \ No newline at end of file diff --git a/tests/test_hnzconf.cpp b/tests/test_hnzconf.cpp index 4be3eb2..aae6ec7 100644 --- a/tests/test_hnzconf.cpp +++ b/tests/test_hnzconf.cpp @@ -1148,7 +1148,7 @@ TEST(HNZCONF, ExchangedDataImportErrors) { "pivot_type": "test", "protocols": [{ "name": "hnzip", - "address": "4294967296", + "address": "18446744073709551616", "typeid": "test" }] }] From 9d080522155e6ac009ce0306bca1e7341aecc7f3 Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Thu, 12 Sep 2024 17:10:02 +0200 Subject: [PATCH 4/6] refs #PMP2-305 Added back a log at state change. Increased a timeout in the back to SARM test to avoid random fail. Signed-off-by: Florent Peyrusse --- src/hnzpath.cpp | 1 + tests/test_hnz.cpp | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/hnzpath.cpp b/src/hnzpath.cpp index 7ed3036..01b2cc2 100644 --- a/src/hnzpath.cpp +++ b/src/hnzpath.cpp @@ -242,6 +242,7 @@ void HNZPath::go_to_connection() { std::lock(m_protocol_state_mutex, m_hnz_connection->getPathMutex()); // Lock both mutexes simultaneously std::lock_guard lock(m_protocol_state_mutex, std::adopt_lock); std::lock_guard lock2(m_hnz_connection->getPathMutex(), std::adopt_lock); + HnzUtility::log_info(beforeLog + " Going to HNZ connection state... Waiting for a SARM."); if (m_protocol_state != CONNECTION) { m_protocol_state = CONNECTION; // Send audit for path connection status diff --git a/tests/test_hnz.cpp b/tests/test_hnz.cpp index a4d83bb..9f5cf7a 100644 --- a/tests/test_hnz.cpp +++ b/tests/test_hnz.cpp @@ -2577,8 +2577,8 @@ TEST_F(HNZTest, BackToSARM) { // Clear messages received from south plugin server->popLastFramesReceived(); - // Wait (repeat_timeout * repeat_path_A) + 1 = (3 * 3) + 1 = 10s - this_thread::sleep_for(chrono::seconds(10)); + // Wait (repeat_timeout * repeat_path_A) + m_repeat_timeout = (3 * 3) + 3 = 12s + this_thread::sleep_for(chrono::seconds(12)); // Find the SARM frame in the list of frames received by server std::vector> frames = server->popLastFramesReceived(); From b79aa9093dfcf0f8055b2386d53215f3cafb60d3 Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Thu, 12 Sep 2024 18:08:48 +0200 Subject: [PATCH 5/6] refs #PMP2-305 Sonarqube code quality fixes. Signed-off-by: Florent Peyrusse --- include/hnzpath.h | 14 +++++++++++++ src/hnzpath.cpp | 52 ++++++++++++++++++++++++++++++----------------- 2 files changed, 47 insertions(+), 19 deletions(-) diff --git a/include/hnzpath.h b/include/hnzpath.h index 62c0757..f2ad80f 100644 --- a/include/hnzpath.h +++ b/include/hnzpath.h @@ -365,6 +365,20 @@ class HNZPath { * @param beforeLog Prefix for the log messages produced by this function */ void m_registerCommandIfSent(const std::string& type, bool sent, unsigned char address, int value, const std::string& beforeLog); + + /** + * Test if a NR is valid + * @param nr NR of the RTU + * @return True if the NR contained in the message was correct, else false + */ + bool m_isNRValid(int nr); + + /** + * Return the number of frames acquitted by the received NR + * @param nr NR of the RTU + * @return Number of frames acquitted + */ + int m_getNROffset(int nr); }; #endif \ No newline at end of file diff --git a/src/hnzpath.cpp b/src/hnzpath.cpp index 01b2cc2..a9a7f8d 100644 --- a/src/hnzpath.cpp +++ b/src/hnzpath.cpp @@ -492,29 +492,13 @@ void HNZPath::m_receivedBULLE() { m_last_msg_time = time(nullptr); } bool HNZPath::m_receivedRR(int nr, bool repetition) { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_receivedRR - " + m_name_log; - // We want to test (m_NRR <= nr <= m_ns) modulo 8 - // Case 0 (OK): m_NRR == nr <= m_ns if (nr != m_NRR) { - // Case 1 (OK): m_NRR < nr <= m_ns - bool frameOk = (m_NRR < nr) && (nr <= m_ns); - if (m_ns < m_NRR) { - // Case 2 (OK): m_ns < m_NRR < nr (m_ns wrapped left by modulo 8) - if(m_NRR < nr) { - frameOk = true; - } - // Case 3 (OK): nr <= m_ns < m_NRR (m_NRR wrapped right by modulo 8) - else if (nr <= m_ns) { - frameOk = true; - } - // Case 4 (NOK): m_ns < nr < m_NRR (nr out of bounds) - } - // Case 5 (NOK): m_NRR < m_ns < nr (nr out of bounds) - // Case 6 (NOK): nr < m_NRR < m_ns (nr out of bounds) - if (frameOk) { + if (m_isNRValid(nr)) { if (!repetition || (m_repeat > 0)) { // valid NR, message(s) well received // remove them from msg sent list - for (size_t i = 0; i < frameOk; i++) { + int nrOffset = m_getNROffset(nr); + for (size_t i = 0; i < nrOffset; i++) { if (!msg_sent.empty()) msg_sent.pop_front(); } @@ -551,6 +535,36 @@ bool HNZPath::m_receivedRR(int nr, bool repetition) { return true; } +bool HNZPath::m_isNRValid(int nr) { + // We want to test (m_NRR <= nr <= m_ns) modulo 8 + bool frameOk = true; + // Case 0 (OK): m_NRR == nr <= m_ns + if (nr != m_NRR) { + // Case 1 (OK): m_NRR < nr <= m_ns + frameOk = (m_NRR < nr) && (nr <= m_ns); + if (m_ns < m_NRR) { + // Case 2 (OK): m_ns < m_NRR < nr (m_ns wrapped left by modulo 8) + if(m_NRR < nr) { + frameOk = true; + } + // Case 3 (OK): nr <= m_ns < m_NRR (m_NRR wrapped right by modulo 8) + else if (nr <= m_ns) { + frameOk = true; + } + // Case 4 (NOK): m_ns < nr < m_NRR (nr out of bounds) + } + // Case 5 (NOK): m_NRR < m_ns < nr (nr out of bounds) + // Case 6 (NOK): nr < m_NRR < m_ns (nr out of bounds) + } + return frameOk; +} + +int HNZPath::m_getNROffset(int nr) { + // Return the distance between nr and m_NRR as a positive modulo 8 number + int modulo = 8; + return (((nr - m_NRR) % modulo) + modulo) % modulo; +} + void HNZPath::m_sendSARM() { std::string beforeLog = HnzUtility::NamePlugin + " - HNZPath::m_sendSARM - " + m_name_log; unsigned char msg[1]{SARM_CODE}; From c27839dc03bcad84ef4c027b8352a316fef7e31b Mon Sep 17 00:00:00 2001 From: Florent Peyrusse Date: Fri, 13 Sep 2024 10:16:49 +0200 Subject: [PATCH 6/6] refs #PMP2-305 Sonarqube code quality fixes part 2. Signed-off-by: Florent Peyrusse --- include/hnzpath.h | 7 +++---- src/hnzpath.cpp | 53 +++++++++++++++++++++++------------------------ 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/include/hnzpath.h b/include/hnzpath.h index f2ad80f..cb0b4dd 100644 --- a/include/hnzpath.h +++ b/include/hnzpath.h @@ -371,14 +371,13 @@ class HNZPath { * @param nr NR of the RTU * @return True if the NR contained in the message was correct, else false */ - bool m_isNRValid(int nr); + bool m_isNRValid(int nr) const; /** - * Return the number of frames acquitted by the received NR + * Called to update internal values once a message containing a valid NR was received * @param nr NR of the RTU - * @return Number of frames acquitted */ - int m_getNROffset(int nr); + void m_NRAccepted(int nr); }; #endif \ No newline at end of file diff --git a/src/hnzpath.cpp b/src/hnzpath.cpp index a9a7f8d..dc37d08 100644 --- a/src/hnzpath.cpp +++ b/src/hnzpath.cpp @@ -495,29 +495,7 @@ bool HNZPath::m_receivedRR(int nr, bool repetition) { if (nr != m_NRR) { if (m_isNRValid(nr)) { if (!repetition || (m_repeat > 0)) { - // valid NR, message(s) well received - // remove them from msg sent list - int nrOffset = m_getNROffset(nr); - for (size_t i = 0; i < nrOffset; i++) { - if (!msg_sent.empty()) msg_sent.pop_front(); - } - - m_NRR = nr; - m_repeat = 0; - - // Waiting for other RR, set timer - if (!msg_sent.empty()) - last_sent_time = std::chrono::duration_cast( - system_clock::now().time_since_epoch()) - .count(); - - // Sent message in waiting queue - while (!msg_waiting.empty() && - (msg_sent.size() < m_anticipation_ratio)) { - if (m_sendInfoImmediately(msg_waiting.front())) { - msg_waiting.pop_front(); - } - } + m_NRAccepted(nr); } else { HnzUtility::log_warn(beforeLog + " Received an unexpected repeated RR, ignoring it"); return false; @@ -535,7 +513,7 @@ bool HNZPath::m_receivedRR(int nr, bool repetition) { return true; } -bool HNZPath::m_isNRValid(int nr) { +bool HNZPath::m_isNRValid(int nr) const { // We want to test (m_NRR <= nr <= m_ns) modulo 8 bool frameOk = true; // Case 0 (OK): m_NRR == nr <= m_ns @@ -559,10 +537,31 @@ bool HNZPath::m_isNRValid(int nr) { return frameOk; } -int HNZPath::m_getNROffset(int nr) { - // Return the distance between nr and m_NRR as a positive modulo 8 number +void HNZPath::m_NRAccepted(int nr) { + // valid NR, message(s) well received + // remove them from msg sent list int modulo = 8; - return (((nr - m_NRR) % modulo) + modulo) % modulo; + int nrOffset = (((nr - m_NRR) % modulo) + modulo) % modulo; + for (size_t i = 0; i < nrOffset; i++) { + if (!msg_sent.empty()) msg_sent.pop_front(); + } + + m_NRR = nr; + m_repeat = 0; + + // Waiting for other RR, set timer + if (!msg_sent.empty()) + last_sent_time = std::chrono::duration_cast( + system_clock::now().time_since_epoch()) + .count(); + + // Sent message in waiting queue + while (!msg_waiting.empty() && + (msg_sent.size() < m_anticipation_ratio)) { + if (m_sendInfoImmediately(msg_waiting.front())) { + msg_waiting.pop_front(); + } + } } void HNZPath::m_sendSARM() {