Fix does not send UnsubscribeVehicleData request to HMI after last unregister app. #2

IGapchuk · 2018-09-01T11:25:54Z

Fixes 2282

This PR is ready for review.

Risk

This PR makes no API changes.

Summary

When last application unregistered, SDL was not sending to HMI UnsubscribeVehicleData request.

This PR provides fix for that defect: when application unregister , SDL checks for exists other registered applications. If they exist, SDL compares their subscriptions for VehicleDataInfo with subscription of unregister application.
If unregister application only itself subscribe to some VehicleDataInfo, that VehicleDataInfo type will be added for unsubscribing.
If unregister application subscribed for some VehicleDataInfo and some register application subscribed for same VehicleDataInfo, that VehicleDataInfo will not added to unsubscribe.
When checking will be finished and all VehicleDataInfo for unregistering have collected, SDL sends to HMI UnsubscribeVehicleData request.

CLA

I have signed the CLA

last unregister app.

mked-luxoft · 2018-09-03T09:30:03Z

src/components/application_manager/src/application_manager_impl.cc

+          HMICommandFactory::CreateCommand(message_to_hmi, *this);
+      if (!command) {
+        LOG4CXX_WARN(logger_, "Failed to create command from smart object");
+      }


@IGapchuk i think there should be a return statement here, otherwise command->Init() or command->Run() below may result in a crash

@mked-luxoft That changes was revert.

mked-luxoft · 2018-09-03T09:32:24Z

src/components/application_manager/src/message_helper/message_helper.cc

 #include <map>
+#include <memory>
+#include <utility>


@IGapchuk is memory header needed here?

@mked-luxoft Fixed in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:24:18Z

src/components/application_manager/include/application_manager/application_manager_impl.h

+   * to unsubscribe.
+   */
+  application_manager::VehicleInfoSubscriptions SelectVehicleDataForUnsubscribe(
+      const app_mngr::ApplicationSharedPtr& application);


@IGapchuk any reasons to pass shared pointer by reference? Also please take a look at typedef ApplicationConstSharedPtr

@AKalinich-Luxoft Fix in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:25:05Z

src/components/application_manager/include/application_manager/message_helper.h

@@ -259,6 +259,23 @@ class MessageHelper {
  static smart_objects::SmartObjectList CreateAddCommandRequestToHMI(
      ApplicationConstSharedPtr app, ApplicationManager& app_mngr);

+  static smart_objects::SmartObjectSPtr CreateMessageForHMI(
+      hmi_apis::messageType::eType message_type, const uint32_t correlation_id);


@IGapchuk first param also could be const

@AKalinich-Luxoft Fix in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:25:23Z

src/components/application_manager/include/application_manager/message_helper.h

@@ -259,6 +259,23 @@ class MessageHelper {
  static smart_objects::SmartObjectList CreateAddCommandRequestToHMI(
      ApplicationConstSharedPtr app, ApplicationManager& app_mngr);

+  static smart_objects::SmartObjectSPtr CreateMessageForHMI(


@IGapchuk please provide description for this function

@AKalinich-Luxoft Added in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:26:14Z

src/components/application_manager/include/application_manager/message_helper.h

+  static smart_objects::SmartObjectSPtr
+  CreateUnsubscribeVehicleDataMessageForHMI(
+      const VehicleInfoSubscriptions& vehicle_data,
+      const application_manager::ApplicationSharedPtr& app);


@IGapchuk any reasons to pass shared pointer by reference?

@AKalinich-Luxoft Fix in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:27:00Z

src/components/application_manager/src/application_manager_impl.cc

@@ -2624,7 +2624,51 @@ void ApplicationManagerImpl::UnregisterApplication(
    } else {
      resume_controller().RemoveApplicationFromSaved(app_to_remove);
    }
+
+    application_manager::VehicleInfoSubscriptions vehicle_data_for_unsubscribe =


@IGapchuk could be const

@AKalinich-Luxoft Fix in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:29:45Z

src/components/application_manager/src/application_manager_impl.cc

+      MessageHelper::PrintSmartObject(*message_to_hmi);
+#endif
+
+      CommandSharedPtr command =


@IGapchuk why just don't use ManageHMICommand(message_to_hmi) call?

AKalinich-Luxoft · 2018-09-03T09:31:10Z

src/components/application_manager/src/commands/hmi/vi_unsubscribe_vehicle_data_request.cc

@@ -44,7 +44,6 @@ VIUnsubscribeVehicleDataRequest::~VIUnsubscribeVehicleDataRequest() {}

 void VIUnsubscribeVehicleDataRequest::Run() {
  LOG4CXX_AUTO_TRACE(logger_);
-


@IGapchuk not related changes

@AKalinich-Luxoft Revert in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:32:00Z

src/components/application_manager/src/message_helper/message_helper.cc

+smart_objects::SmartObjectSPtr MessageHelper::CreateMessageForHMI(
+    hmi_apis::messageType::eType message_type, const uint32_t correlation_id) {
+  auto message = new smart_objects::SmartObject(smart_objects::SmartType_Map);
+  auto& ref = *message;


@IGapchuk suggest to use more descriptive names like message_so and message_ref

@AKalinich-Luxoft Renamed in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:33:09Z

src/components/application_manager/src/mobile_command_factory.cc

@@ -143,7 +143,6 @@ CommandSharedPtr MobileCommandFactory::CreateCommand(
    commands::Command::CommandOrigin origin,
    ApplicationManager& application_manager) {
  CommandSharedPtr command;
-


@IGapchuk unrelated changes

@AKalinich-Luxoft Reverted in 36b0ead.

AKalinich-Luxoft · 2018-09-03T09:33:21Z

src/components/application_manager/src/message_helper/message_helper.cc

@@ -1328,9 +1371,11 @@ void MessageHelper::SendOnAppUnregNotificationToHMI(
      hmi_apis::FunctionID::BasicCommunication_OnAppUnregistered;

  message[strings::params][strings::message_type] = MessageType::kNotification;
-  // we put hmi_app_id because applicaton list does not contain application on


@IGapchuk unrelated changes

@AKalinich-Luxoft Reverted in 36b0ead.

mked-luxoft · 2018-09-03T09:38:01Z

src/components/application_manager/include/application_manager/message_helper.h

@@ -259,6 +259,23 @@ class MessageHelper {
  static smart_objects::SmartObjectList CreateAddCommandRequestToHMI(
      ApplicationConstSharedPtr app, ApplicationManager& app_mngr);

+  static smart_objects::SmartObjectSPtr CreateMessageForHMI(


@IGapchuk i think some sort of a short description is lacking here

@mked-luxoft Added in 36b0ead.

mked-luxoft · 2018-09-03T09:45:12Z

src/components/application_manager/src/application_manager_impl.cc

+        LOG4CXX_WARN(logger_, "Failed to create command from smart object");
+      }
+
+      int32_t message_type =


@IGapchuk minor thing, but i suppose it's better to make this variable const, since we do not modify it anywhere below

@mked-luxoft That changes was revert.

AByzhynar · 2018-09-05T09:54:34Z

@IGapchuk Check destination branch and repo

AByzhynar · 2018-09-05T09:55:08Z

@IGapchuk Your branches names are invalid

AByzhynar · 2018-09-05T09:56:01Z

@IGapchuk Update PR description with detailed info where is the problem and why do you think your fix solves it.

AByzhynar · 2018-11-09T12:02:12Z

@IGapchuk Any updates?

Fix does not send UnsubscribeVehicleData request to HMI after

fa179cb

last unregister app.

IGapchuk changed the base branch from master to branch_4.2.1 September 1, 2018 11:27

Add checking if needed to send UnsibscribeVehicle request.

5e67cdc

IGapchuk requested review from mked-luxoft and AKalinich-Luxoft September 3, 2018 09:00

mked-luxoft reviewed Sep 3, 2018

View reviewed changes

AKalinich-Luxoft requested changes Sep 3, 2018

View reviewed changes

mked-luxoft reviewed Sep 3, 2018

View reviewed changes

IGapchuk force-pushed the 4.2.1.fix/sdl_not_send_UnsubscribeVehicleData branch 2 times, most recently from 11b0429 to 5e67cdc Compare September 3, 2018 12:21

Answer to PR.

36b0ead

AKalinich-Luxoft approved these changes Sep 3, 2018

View reviewed changes

IGapchuk requested review from AByzhynar, Ypostolov and ZhdanovP September 3, 2018 12:30

mked-luxoft approved these changes Sep 4, 2018

View reviewed changes

ZhdanovP approved these changes Nov 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix does not send UnsubscribeVehicleData request to HMI after last unregister app. #2

Fix does not send UnsubscribeVehicleData request to HMI after last unregister app. #2

IGapchuk commented Sep 1, 2018 •

edited

Loading

mked-luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

mked-luxoft Sep 3, 2018

IGapchuk Sep 3, 2018

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

AKalinich-Luxoft Sep 3, 2018

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018

AKalinich-Luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

mked-luxoft Sep 3, 2018

IGapchuk Sep 3, 2018 •

edited

Loading

mked-luxoft Sep 3, 2018

IGapchuk Sep 3, 2018

AByzhynar commented Sep 5, 2018

AByzhynar commented Sep 5, 2018

AByzhynar commented Sep 5, 2018

AByzhynar commented Nov 9, 2018

		@@ -44,7 +44,6 @@ VIUnsubscribeVehicleDataRequest::~VIUnsubscribeVehicleDataRequest() {}

		void VIUnsubscribeVehicleDataRequest::Run() {
		LOG4CXX_AUTO_TRACE(logger_);

Fix does not send UnsubscribeVehicleData request to HMI after last unregister app. #2

Are you sure you want to change the base?

Fix does not send UnsubscribeVehicleData request to HMI after last unregister app. #2

Conversation

IGapchuk commented Sep 1, 2018 • edited Loading

Risk

Summary

CLA

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IGapchuk Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar commented Sep 5, 2018

AByzhynar commented Sep 5, 2018

AByzhynar commented Sep 5, 2018

AByzhynar commented Nov 9, 2018

IGapchuk commented Sep 1, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading

IGapchuk Sep 3, 2018 •

edited

Loading