Feature/button subscription response from hmi #2731

mked-luxoft · 2018-10-30T14:28:33Z

Fixes #2508

This PR is [ready] for review.

Risk

This PR makes [major] API changes.

Testing Plan

ATF tests are provided

Summary

In this PR OnButtonSubscription notification is replaced with
(Un)SubscribeButton request/response.

Changelog

API was updated, as well as new request/response classes were introduces in SDL core.
Pending subscription resolution mechanism was introduced in case HMI responds to request after default timeout.

Bug Fixes

#967 is fixed in scope of this feature

CLA

I have signed the CLA

moved shared logic of OnButtonEvent and OnButtonPress to ButtonNotificationToMobile base class

IGapchuk · 2018-11-01T09:04:29Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/subscribe_button_request.cc

+}
+
+SubscribeButtonRequest::~SubscribeButtonRequest() {
+  unsubscribe_from_event(hmi_apis::FunctionID::Buttons_SubscribeButton);


@mked-luxoft Base class EventObserver already contains unsubscribing from all events in his distructor.

@IGapchuk fixed in 2b001c6

AByzhynar · 2018-11-05T11:41:16Z

src/components/application_manager/include/application_manager/application_impl.h

+   * @brief Get map of pending button subscription requests correlation ids
+   * to button names
+   * @return pending button subscriptions map
+   */


@mked-luxoft Remove descriptions from impl class

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:41:41Z

src/components/application_manager/include/application_manager/application.h

+   * to button names
+   * @return pending button unsubscriptions map
+   */
+  const virtual std::map<int32_t, hmi_apis::Common_ButtonName::eType>&


@mked-luxoft Move const qualifier to return type

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:43:05Z

src/components/application_manager/include/application_manager/application_impl.h

@@ -493,6 +537,10 @@ class ApplicationImpl : public virtual Application,
  std::string bundle_id_;
  AppFilesMap app_files_;
  std::set<mobile_apis::ButtonName::eType> subscribed_buttons_;
+  std::map<int32_t, hmi_apis::Common_ButtonName::eType>
+      pending_subscription_buttons_;


@mked-luxoft May be pending_button_subscriptions? As well as related function names

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:44:18Z

...nts/application_manager/include/application_manager/commands/button_notification_to_mobile.h

+#define SRC_COMPONENTS_APPLICATION_MANAGER_INCLUDE_APPLICATION_MANAGER_COMMANDS_BUTTON_NOTIFICATION_TO_MOBILE_H_
+
+#include "application_manager/application.h"
+#include "command_notification_impl.h"


@mked-luxoft Move it to first place of all includes

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:44:30Z

...nts/application_manager/include/application_manager/commands/button_notification_to_mobile.h

+#include "interfaces/MOBILE_API.h"
+
+namespace application_manager {
+


@mked-luxoft
Redundant empty line

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:44:53Z

...nts/application_manager/include/application_manager/commands/button_notification_to_mobile.h

+
+/**
+ * @brief Class is intended to encapsulate shared button notification logic in
+ *base class


@mked-luxoft missed indent

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:47:04Z

...nts/application_manager/include/application_manager/commands/button_notification_to_mobile.h

+ **/
+class ButtonNotificationToMobile
+    : public app_mngr::commands::CommandNotificationImpl {
+ protected:


@mked-luxoft A class definition should usually start with a public: section, followed by protected:, then private:. Omit sections that would be empty.

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:51:18Z

...er/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/hmi/subscribe_button_request.h

+namespace app_mngr = application_manager;
+
+namespace commands {
+


@mked-luxoft
Redundant empty line

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:52:26Z

...er/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/hmi/subscribe_button_request.h

+
+}  // namespace hmi
+}  // namespace commands
+


@mked-luxoft
Redundant empty line

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:52:59Z

...r/rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/hmi/subscribe_button_response.h

+}  // namespace hmi
+
+}  // namespace commands
+


@mked-luxoft
Redundant empty line

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:54:50Z

...rpc_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/subscribe_button_request.h

 #include "application_manager/application_impl.h"
+#include "application_manager/commands/command_request_impl.h"


@mked-luxoft Wrong includes order. It was right before

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:55:47Z

...c_plugins/sdl_rpc_plugin/include/sdl_rpc_plugin/commands/mobile/unsubscribe_button_request.h

-   * @brief Sends ButtonSubscription notification
-   * to notify HMI that app unsubscribed from the button.
+   * @brief Interface method that is called whenever new event received
+   * @param event The received event
   */


@mked-luxoft REmove description form impl class

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T11:58:49Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/subscribe_button_request.cc

+      static_cast<hmi_apis::Common_Result::eType>(
+          message[strings::params][hmi_response::code].asInt());
+
+  const bool is_in_pending = app->PendingSubscriptionButtons().find(


@mked-luxoft I would recommend to split it to several expressions.

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T12:00:56Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/subscribe_button_request.cc

+                                 event.smart_object_correlation_id()) !=
+                             app->PendingSubscriptionButtons().end();
+
+  if (hmi_apis::Common_Result::SUCCESS == hmi_result && is_in_pending) {


@mked-luxoft What if HMI result will be some other positive like WARNING ?

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T12:09:07Z

...ion_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/register_app_interface_request.cc

-  CreateHMINotification(FunctionID::Buttons_OnButtonSubscription, msg_params);
+
+  SendHMIRequest(
+      hmi_apis::FunctionID::Buttons_SubscribeButton, &msg_params, false);


@mked-luxoft We are not waiting for HMI response here...What if HMI will not subscribe app for CUSTOM BUTTON?

@AByzhynar, are you referring to handling events from HMI in on_event method? if so, i think that it will be handled in implementation of SubscribeButtonRequest class, since this request will be constructed and run as a result of call to SendHMIRequest

@mked-luxoft Yes. But what code will be responsible as you explicitly stated that we are not going to subscribe for HMI response?

@AByzhynar if i understand correctly, we are not subscribing to HMI response in RAIRequest instance. However, we are still subscribing to HMI responses in SubscribeButtonRequest::Run(). Hence, we are handling these responses.

@mked-luxoft Thanks. 👍

LitvinenkoIra · 2018-11-05T12:09:22Z

...cle_info_plugin/include/vehicle_info_plugin/commands/mobile/subscribe_vehicle_data_request.h

@@ -37,8 +37,8 @@
 #ifndef SRC_COMPONENTS_APPLICATION_MANAGER_RPC_PLUGINS_SDL_RPC_PLUGIN_INCLUDE_SDL_RPC_PLUGIN_COMMANDS_MOBILE_SUBSCRIBE_VEHICLE_DATA_REQUEST_H_
 #define SRC_COMPONENTS_APPLICATION_MANAGER_RPC_PLUGINS_SDL_RPC_PLUGIN_INCLUDE_SDL_RPC_PLUGIN_COMMANDS_MOBILE_SUBSCRIBE_VEHICLE_DATA_REQUEST_H_


@mked-luxoft please fix header guards
SRC_COMPONENTS_APPLICATION_MANAGER_RPC_PLUGINS_VEHICLE_INFO_PLUGIN_INCLUDE_VEHICLE_INFO_PLUGIN_COMMANDS_MOBILE_SUBSCRIBE_VEHICLE_DATA_REQUEST_H_

@LitvinenkoIra fixed in 2b001c6

LitvinenkoIra · 2018-11-05T12:12:35Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+  const uint32_t btn_id = static_cast<uint32_t>(
+      (*message_)[strings::msg_params][hmi_response::button_name].asInt());
+
+  LOG4CXX_DEBUG(logger_, "reveived button id: " << btn_id);


@mked-luxoft typo received button id:

@LitvinenkoIra fixed in 2b001c6

AByzhynar · 2018-11-05T12:20:43Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+
+ButtonNotificationToMobile::~ButtonNotificationToMobile() {}
+
+bool ButtonNotificationToMobile::IsAppIDExist() const {


@mked-luxoft Does exist or is existed...

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T12:23:30Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+  }
+
+  // custom_button_id is mandatory for CUSTOM_BUTTON notification
+  if (false ==


@mked-luxoft == false is redundant. Use (! keyExists)

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T12:23:56Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+  custom_btn_id =
+      (*message_)[strings::msg_params][hmi_response::custom_button_id].asUInt();
+
+  if (false == app->IsSubscribedToSoftButton(custom_btn_id)) {


@mked-luxoft Same as above

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T12:26:34Z

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

-  return notification_ptr;
+  if (!app) {
+    LOG4CXX_ERROR(logger_, "application NULL pointer");
+    return nullptr;


@mked-luxoft Are you sure that nullptr is good here? May return SmartObject ptr with SmartType_Null?

@AByzhynar fixed in 2b001c6

AByzhynar · 2018-11-05T12:28:12Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+
+  std::vector<ApplicationSharedPtr>::const_iterator it =
+      subscribed_apps.begin();
+  for (; subscribed_apps.end() != it; ++it) {


@mked-luxoft Range based loop?

@AByzhynar fixed in 2b001c6

ZhdanovP · 2018-11-05T12:36:23Z

src/components/application_manager/include/application_manager/application.h

+   * to button names
+   * @return pending button subscriptions map
+   */
+  const virtual std::map<int32_t, hmi_apis::Common_ButtonName::eType>&


@mked-luxoft I think we can use typedef here like a MapButtons

@ZhdanovP fixed in 704a3e5

AByzhynar · 2018-11-05T16:35:29Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+    return;
+  }
+
+  uint32_t custom_btn_id = 0;


@mked-luxoft What do you need intermediate initialization here for?

@AByzhynar fixed in 704a3e5

AByzhynar · 2018-11-05T16:36:33Z

src/components/application_manager/src/commands/button_notification_to_mobile.cc

+
+bool ButtonNotificationToMobile::DoesAppIDExist() const {
+  LOG4CXX_AUTO_TRACE(logger_);
+  using namespace application_manager;


@mked-luxoft using namespace application_manager; -> using application_manager::strings;? No?

@AByzhynar fixed in 704a3e5

-added typedef to pending button subscriptions container -removed redundant initialization -changed to using namespace application_manager::strings where relevant

ZhdanovP · 2018-11-06T10:34:56Z

.../application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/subscribe_button_request.cc

+  ApplicationSharedPtr app =
+      application_manager_.application_by_hmi_app(app_id);
+
+  if (!app) {


@mked-luxoft Will be performed the Button unsubscribing during the ApplicationManagerImpl::UnregisterApplication call ?

@ZhdanovP i think so, but don't beleive that there's a direct call in ApplicationManagerImpl::UnregisterApplication and i'm not sure whether there should be one.

@mked-luxoft I think it should be done directly like a UnsubscribeAppFromWayPoints but Iam not sure that this is critical now

LuxoftAKutsan · 2018-11-06T11:20:48Z

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

-  * ALL means all do, NONE means none do
-  */
+   * @brief Stores whether each choice in a set has the vrCommands parameter
+   * MIXED means some choices have vrCommands and others don't


@mked-luxoft is it auto formating of clang-style check? If not ( i think not) please remove.

LuxoftAKutsan · 2018-11-06T11:24:25Z

...ation_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/on_button_event_notification.cc

-                              rpc_service,
-                              hmi_capabilities,
-                              policy_handler) {}
+    : ButtonNotificationToMobile(message,


@mked-luxoft please provide some havy reason to move part of logic of OnButtonEventNotification to additional layer of inheritance ButtonNotificationToMobile

@mked-luxoft and describe it in PR and in comments

LuxoftAKutsan · 2018-11-06T11:25:51Z

src/components/application_manager/src/application_impl.cc

+
+void ApplicationImpl::RemovePendingSubscriptionButton(
+    const int32_t correlation_id) {
+  pending_button_subscriptions_.erase(correlation_id);


@mked-luxoft I would make it thread safe

LuxoftAKutsan

Please make implementation thread safe.

LuxoftAKutsan · 2018-11-06T11:27:12Z

src/components/interfaces/HMI_API.xml

@@ -3424,6 +3424,35 @@
        </description>
      </param>
    </function>
+    <function name="SubscribeButton" messagetype="request">


@mked-luxoft did you remove notification?

-removed OnButtonSubscription notification -added comments to ButtonNotificationToMobile class

AByzhynar and others added 14 commits September 20, 2018 15:13

Initial commit

4a99b9f

update MessageHelper for SubscribeButton request

7a4c092

Fix & update unit tests and mocks

395cba7

Missed mandatory parameter appID in request SubscribeButton was added.

e883d0c

Mandatory parameter appID in request UnsubscribeButton was added.

35efda1

implement event handling in SubscribeButton request

a352bcc

added pending button subscription resolving

b7cd2e4

added ButtonNotificationToMobile class

8c9491b

moved shared logic of OnButtonEvent and OnButtonPress to ButtonNotificationToMobile base class

add preset buttons to list of not allowed for non-media app

6a3e706

updated resumption logic for button subscription

7362754

update unit test

df3f80d

fix code style

a5533ec

Made pending button subscription container getter const

422a87f

Added documentation for ButtonNotificationToMobile class

b43db3b

mked-luxoft requested review from Itileda, AByzhynar, LuxoftAKutsan, LitvinenkoIra, SNiukalov, EKuliiev, KVGrygoriev, ZhdanovP and IGapchuk October 30, 2018 14:28

IGapchuk reviewed Nov 1, 2018

View reviewed changes

AByzhynar suggested changes Nov 5, 2018

View reviewed changes

LitvinenkoIra reviewed Nov 5, 2018

View reviewed changes

fixed multiple button resumption

f62e435

AByzhynar suggested changes Nov 5, 2018

View reviewed changes

ZhdanovP reviewed Nov 5, 2018

View reviewed changes

KhrystynaDubovyk mentioned this pull request Nov 5, 2018

Feature/Button Subscription response from HMI smartdevicelink/sdl_hmi_integration_guidelines#132

Closed

1 task

answer PR comments

2b001c6

mked-luxoft force-pushed the feature/Button_Subscription_response_from_HMI branch from abfab39 to 2b001c6 Compare November 5, 2018 16:02

AByzhynar suggested changes Nov 5, 2018

View reviewed changes

answer PR comments

704a3e5

-added typedef to pending button subscriptions container -removed redundant initialization -changed to using namespace application_manager::strings where relevant

AByzhynar approved these changes Nov 6, 2018

View reviewed changes

ZhdanovP reviewed Nov 6, 2018

View reviewed changes

LuxoftAKutsan reviewed Nov 6, 2018

View reviewed changes

LuxoftAKutsan approved these changes Nov 6, 2018

View reviewed changes

LuxoftAKutsan reviewed Nov 6, 2018

View reviewed changes

ZhdanovP approved these changes Nov 6, 2018

View reviewed changes

LitvinenkoIra approved these changes Nov 6, 2018

View reviewed changes

mked-luxoft added 2 commits November 6, 2018 16:30

answer PR comments

e02d38a

-removed OnButtonSubscription notification -added comments to ButtonNotificationToMobile class

added locks for pending subscriptions

a86e422

mked-luxoft closed this Nov 6, 2018

		#include "interfaces/MOBILE_API.h"

		namespace application_manager {

		namespace app_mngr = application_manager;

		namespace commands {

		#include "application_manager/application_impl.h"
		#include "application_manager/commands/command_request_impl.h"

		@@ -37,8 +37,8 @@
		#ifndef SRC_COMPONENTS_APPLICATION_MANAGER_RPC_PLUGINS_SDL_RPC_PLUGIN_INCLUDE_SDL_RPC_PLUGIN_COMMANDS_MOBILE_SUBSCRIBE_VEHICLE_DATA_REQUEST_H_
		#define SRC_COMPONENTS_APPLICATION_MANAGER_RPC_PLUGINS_SDL_RPC_PLUGIN_INCLUDE_SDL_RPC_PLUGIN_COMMANDS_MOBILE_SUBSCRIBE_VEHICLE_DATA_REQUEST_H_


		ButtonNotificationToMobile::~ButtonNotificationToMobile() {}

		bool ButtonNotificationToMobile::IsAppIDExist() const {

Feature/button subscription response from hmi #2731

Feature/button subscription response from hmi #2731

Conversation

mked-luxoft commented Oct 30, 2018 • edited Loading

Risk

Testing Plan

Summary

Changelog

Bug Fixes

CLA

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

mked-luxoft Nov 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 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

LitvinenkoIra Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mked-luxoft Nov 5, 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

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

mked-luxoft commented Oct 30, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

LitvinenkoIra Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading

mked-luxoft Nov 5, 2018 •

edited

Loading