fix: send Service Data ACK frames only for version 2-4

The protocol spec document
https://github.com/smartdevicelink/protocol_spec says that the
frame is deprecated. Looking at its commit history the frame has
been marked as deprecated before introducing protocol version 5.

Also, looking at the comment in protocol_handler_impl.cc the
frame wasn't meant to send out in protocol version 1.

Therefore this fix will limit the use of Service Data ACK
for protocol version 2 through 4.

2 files changed, 78 insertions, 21 deletions

diff --git a/src/components/protocol_handler/src/protocol_handler_impl.cc b/src/components/protocol_handler/src/protocol_handler_impl.cc
index ec4663764a..4f7ca507b6 100644
--- a/src/components/protocol_handler/src/protocol_handler_impl.cc
+++ b/src/components/protocol_handler/src/protocol_handler_impl.cc
@@ -1944,33 +1944,34 @@ void ProtocolHandlerImpl::SendFramesNumber(uint32_t connection_key,
   LOG4CXX_DEBUG(
       logger_, "SendFramesNumber MobileNaviAck for session " << connection_key);
 
-  // TODO(EZamakhov): add protocol version check - to avoid send for
-  // PROTOCOL_VERSION_1
   transport_manager::ConnectionUID connection_id = 0;
   uint8_t session_id = 0;
   session_observer_.PairFromKey(connection_key, &connection_id, &session_id);
   uint8_t protocol_version;
   if (session_observer_.ProtocolVersionUsed(
           connection_id, session_id, protocol_version)) {
-    ProtocolFramePtr ptr(
-        new protocol_handler::ProtocolPacket(connection_id,
-                                             protocol_version,
-                                             PROTECTION_OFF,
-                                             FRAME_TYPE_CONTROL,
-                                             SERVICE_TYPE_NAVI,
-                                             FRAME_DATA_SERVICE_DATA_ACK,
-                                             session_id,
-                                             0,
-                                             message_counters_[session_id]++));
-
-    // Flow control data shall be 4 bytes according Ford Protocol
-    DCHECK(sizeof(number_of_frames) == 4);
-    number_of_frames = LE_TO_BE32(number_of_frames);
-    ptr->set_data(reinterpret_cast<const uint8_t*>(&number_of_frames),
-                  sizeof(number_of_frames));
-    raw_ford_messages_to_mobile_.PostMessage(
-        impl::RawFordMessageToMobile(ptr, false));
-    LOG4CXX_DEBUG(logger_, "SendFramesNumber finished successfully");
+    if (protocol_version > PROTOCOL_VERSION_1 &&
+        protocol_version < PROTOCOL_VERSION_5) {
+      ProtocolFramePtr ptr(new protocol_handler::ProtocolPacket(
+          connection_id,
+          protocol_version,
+          PROTECTION_OFF,
+          FRAME_TYPE_CONTROL,
+          SERVICE_TYPE_NAVI,
+          FRAME_DATA_SERVICE_DATA_ACK,
+          session_id,
+          0,
+          message_counters_[session_id]++));
+
+      // Flow control data shall be 4 bytes according Ford Protocol
+      DCHECK(sizeof(number_of_frames) == 4);
+      number_of_frames = LE_TO_BE32(number_of_frames);
+      ptr->set_data(reinterpret_cast<const uint8_t*>(&number_of_frames),
+                    sizeof(number_of_frames));
+      raw_ford_messages_to_mobile_.PostMessage(
+          impl::RawFordMessageToMobile(ptr, false));
+      LOG4CXX_DEBUG(logger_, "SendFramesNumber finished successfully");
+    }
   } else {
     LOG4CXX_WARN(
         logger_,
diff --git a/src/components/protocol_handler/test/protocol_handler_tm_test.cc b/src/components/protocol_handler/test/protocol_handler_tm_test.cc
index 89b9faedaa..d7850e4df1 100644
--- a/src/components/protocol_handler/test/protocol_handler_tm_test.cc
+++ b/src/components/protocol_handler/test/protocol_handler_tm_test.cc
@@ -65,6 +65,8 @@ using protocol_handler::PROTECTION_OFF;
 using protocol_handler::PROTOCOL_VERSION_1;
 using protocol_handler::PROTOCOL_VERSION_2;
 using protocol_handler::PROTOCOL_VERSION_3;
+using protocol_handler::PROTOCOL_VERSION_4;
+using protocol_handler::PROTOCOL_VERSION_5;
 using protocol_handler::PROTOCOL_VERSION_MAX;
 using protocol_handler::FRAME_TYPE_CONTROL;
 using protocol_handler::FRAME_TYPE_SINGLE;
@@ -79,6 +81,7 @@ using protocol_handler::FRAME_DATA_END_SERVICE_ACK;
 using protocol_handler::FRAME_DATA_END_SERVICE;
 using protocol_handler::FRAME_DATA_HEART_BEAT;
 using protocol_handler::FRAME_DATA_HEART_BEAT_ACK;
+using protocol_handler::FRAME_DATA_SERVICE_DATA_ACK;
 using protocol_handler::FRAME_DATA_SINGLE;
 using protocol_handler::FRAME_DATA_FIRST;
 using protocol_handler::FRAME_DATA_LAST_CONSECUTIVE;
@@ -2143,6 +2146,59 @@ TEST_F(ProtocolHandlerImplTest, SendMessageToMobileApp_SendMultiframeMessage) {
   EXPECT_TRUE(waiter->WaitFor(times, kAsyncExpectationsTimeout));
 }
 
+TEST_F(ProtocolHandlerImplTest, SendServiceDataAck_PreVersion5) {
+  ::utils::SharedPtr<TestAsyncWaiter> waiter =
+      utils::MakeShared<TestAsyncWaiter>();
+  uint32_t times = 0;
+
+  AddSession(waiter, times);
+
+  EXPECT_CALL(session_observer_mock, PairFromKey(connection_key, _, _))
+      .WillOnce(
+          DoAll(SetArgPointee<1>(connection_id), SetArgPointee<2>(session_id)));
+  EXPECT_CALL(session_observer_mock, ProtocolVersionUsed(connection_id, _, _))
+      .WillRepeatedly(
+          DoAll(SetArgReferee<2>(PROTOCOL_VERSION_4), Return(true)));
+
+  EXPECT_CALL(transport_manager_mock,
+              SendMessageToDevice(ExpectedMessage(FRAME_TYPE_CONTROL,
+                                                  FRAME_DATA_SERVICE_DATA_ACK,
+                                                  PROTECTION_OFF,
+                                                  kMobileNav)))
+      .WillOnce(DoAll(NotifyTestAsyncWaiter(waiter), Return(E_SUCCESS)));
+  times++;
+
+  protocol_handler_impl->SendFramesNumber(connection_key, 0);
+
+  EXPECT_TRUE(waiter->WaitFor(times, kAsyncExpectationsTimeout));
+}
+
+TEST_F(ProtocolHandlerImplTest, SendServiceDataAck_AfterVersion5) {
+  ::utils::SharedPtr<TestAsyncWaiter> waiter =
+      utils::MakeShared<TestAsyncWaiter>();
+  uint32_t times = 0;
+
+  AddSession(waiter, times);
+
+  EXPECT_CALL(session_observer_mock, PairFromKey(connection_key, _, _))
+      .WillOnce(
+          DoAll(SetArgPointee<1>(connection_id), SetArgPointee<2>(session_id)));
+  EXPECT_CALL(session_observer_mock, ProtocolVersionUsed(connection_id, _, _))
+      .WillRepeatedly(
+          DoAll(SetArgReferee<2>(PROTOCOL_VERSION_5), Return(true)));
+
+  // It is expected that Service Data ACK is NOT sent for version 5+
+  EXPECT_CALL(transport_manager_mock,
+              SendMessageToDevice(ExpectedMessage(FRAME_TYPE_CONTROL,
+                                                  FRAME_DATA_SERVICE_DATA_ACK,
+                                                  PROTECTION_OFF,
+                                                  kMobileNav))).Times(0);
+
+  protocol_handler_impl->SendFramesNumber(connection_key, 0);
+
+  EXPECT_TRUE(waiter->WaitFor(times, kAsyncExpectationsTimeout));
+}
+
 }  // namespace protocol_handler_test
 }  // namespace components
 }  // namespace test