Feature/New remote control modules and parameters #2207

LitvinenkoIra · 2018-05-23T09:02:05Z

Technical task : #1798
This PR is ready for review.

This pull request contains the implementation of new remote control modules and parameters
(proposal: https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0099-new-remote-control-modules-and-parameters.md)
For testing use the following test set:
https://github.com/smartdevicelink/sdl_atf_test_scripts/blob/feature/rc_new_modules/test_sets/rc_AUDIO_LIGHT_HMI_SETTINGS.txt

Pull request with ATF scripts: smartdevicelink/sdl_atf_test_scripts#1909

For manual testing use:
HMI from : smartdevicelink/sdl_hmi#52
mobile application SPT from HockeyApp 20180213-Nightly-Android

Risk

This PR makes [major] API changes.

CLA

I have signed the CLA

yang1070 · 2018-05-29T16:12:16Z

...n_manager/rpc_plugins/rc_rpc_plugin/include/rc_rpc_plugin/resource_allocation_manager_impl.h

@@ -118,6 +118,10 @@ class ResourceAllocationManagerImpl : public ResourceAllocationManager {
  RCAppExtensionPtr GetApplicationExtention(
      application_manager::ApplicationSharedPtr application) FINAL;

+  bool is_rc_enabled() const FINAL;


not related to new module?

yang1070 · 2018-05-29T16:12:54Z

...n_manager/rpc_plugins/rc_rpc_plugin/include/rc_rpc_plugin/resource_allocation_manager_impl.h

@@ -118,6 +118,10 @@ class ResourceAllocationManagerImpl : public ResourceAllocationManager {
  RCAppExtensionPtr GetApplicationExtention(
      application_manager::ApplicationSharedPtr application) FINAL;

+  bool is_rc_enabled() const FINAL;
+
+  void set_rc_enabled(const bool value) FINAL;


not related to new module?

yang1070 · 2018-05-29T16:13:17Z

...n_manager/rpc_plugins/rc_rpc_plugin/include/rc_rpc_plugin/resource_allocation_manager_impl.h

@@ -204,6 +208,7 @@ class ResourceAllocationManagerImpl : public ResourceAllocationManager {
  hmi_apis::Common_RCAccessMode::eType current_access_mode_;
  application_manager::ApplicationManager& app_mngr_;
  application_manager::rpc_service::RPCService& rpc_service_;
+  bool is_rc_enabled_;


shall not be new code

yang1070 · 2018-05-29T17:25:24Z

...cation_manager/rpc_plugins/rc_rpc_plugin/include/rc_rpc_plugin/resource_allocation_manager.h

@@ -144,6 +144,10 @@ class ResourceAllocationManager {
  virtual RCAppExtensionPtr GetApplicationExtention(
      application_manager::ApplicationSharedPtr application) = 0;

+  virtual bool is_rc_enabled() const = 0;


not new module related

yang1070 · 2018-05-29T17:44:18Z

...ger/rpc_plugins/rc_rpc_plugin/src/commands/hmi/rc_on_remote_control_settings_notification.cc

@@ -131,6 +131,7 @@ void RCOnRemoteControlSettingsNotification::Run() {
    hmi_apis::Common_RCAccessMode::eType access_mode =
        hmi_apis::Common_RCAccessMode::INVALID_ENUM;
    LOG4CXX_DEBUG(logger_, "Allowing RC Functionality");
+    resource_allocation_manager_.set_rc_enabled(true);


that 's #2197

yang1070 · 2018-05-29T18:01:35Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

+    if (message_params::kLightState == request_parameter) {
+      return CheckLightNameByCapabilities(
+          module_caps[strings::kSupportedLights],
+          control_data[request_parameter][0]);


control_data[request_parameter][0], this only checks the first element of LightState array ? How about there is more than 1 element?

@yang1070 Fixed 7211a36

yang1070 · 2018-05-29T18:06:40Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

+  bool module_type_and_data_match = false;
+  for (const auto& data : data_mapping) {
+    if (data.first == module_type) {
+      module_type_and_data_match = module_data.keyExists(data.second);


yang1070 · 2018-05-29T18:07:57Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

+
+    if (app_wants_to_set_audio_src && !app->IsAllowedToChangeAudioSource()) {
+      LOG4CXX_WARN(logger_, "App is not allowed to change audio source");
+      SetResourceState(ModuleType(), ResourceState::FREE);


why need SetResourceState(ModuleType(), ResourceState::FREE);?

@yang1070 because we execute the command only if the resource successfully acquired https://github.com/smartdevicelink/sdl_core/blob/feature/rc_new_modules/src/components/application_manager/rpc_plugins/rc_rpc_plugin/src/commands/rc_command_request.cc#L144

and release it when we received response from HMI
https://github.com/smartdevicelink/sdl_core/blob/feature/rc_new_modules/src/components/application_manager/rpc_plugins/rc_rpc_plugin/src/commands/rc_command_request.cc#L192

so in case when smthg goes wrong (when we execute the command) and we did not send a request to HMI, we need to release the resource before sending a response to mobile.

Let's consider the scenario
app1 set the volume, it succeeds, AUDIO module is assigned to app1
app1 set some value of equalizer, it succeeds, AUDIO module is still assigned to app1
app1 tried to set the audio source, and does not pass the check of line 374, AUDIO shall still be allocated to app1, AUDIO module shall not be set to free in line 376, it shall be unchanged.

@yang1070
I tested this case, so in current implementation:

app1 set the volume, it succeeds, AUDIO module is assigned to app1 (SDL don't receive response from HMI)

app1 set some value of equalizer and receive from SDL response "IN_USE"

then when we receive response from HMI for step 1 -> AUDIO module is released

app1 tried to set the audio source (and acquire AUDIO module), and does not pass the check of line 374 -> AUDIO module becomes free.

maybe we should change this logic, but actually this is an old RC functionality, here is a link for our requirements for resource allocation
https://github.com/smartdevicelink/sdl_requirements/blob/master/detailed_docs/RC/resource_allocation.md

yang1070 · 2018-05-29T18:08:50Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

+      return module_data[data.second];
+    }
+  }
+  NOTREACHED();


what is NOTREACHED()?

@yang1070 DCHECK(!"Unreachable code")

yang1070 · 2018-05-29T18:13:43Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

  }
+  application_manager_.set_current_audio_source(


shall this be set only after sdl forward the request to HMI and get a successful response?

@yang1070 Fixed c88c032

yang1070 · 2018-05-29T18:25:23Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

 bool SetInteriorVehicleDataRequest::AreReadOnlyParamsPresent(
    const smart_objects::SmartObject& module_data) {
  LOG4CXX_AUTO_TRACE(logger_);
  const smart_objects::SmartObject& module_type_params =
      ControlData(module_data);
  auto it = module_type_params.map_begin();
-  std::vector<std::string> ro_params = GetModuleReadOnlyParams(ModuleType());
+  const std::string module_type = ModuleType();
+  std::vector<std::string> ro_params = GetModuleReadOnlyParams(module_type);


it is strange kChannelName is not returned in GetModuleReadOnlyParams()

@yang1070 because channelName this is a field in the EqualizerSettings structure https://github.com/smartdevicelink/sdl_core/blob/feature/rc_new_modules/src/components/interfaces/MOBILE_API.xml#L2907

so we have a separate function that checks read only params for AUDIO module
https://github.com/smartdevicelink/sdl_core/blob/feature/rc_new_modules/src/components/application_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc#L471

yang1070 · 2018-05-29T18:28:22Z

src/components/application_manager/rpc_plugins/rc_rpc_plugin/src/commands/rc_command_request.cc

@@ -143,6 +143,14 @@ void RCCommandRequest::Run() {
    SendResponse(false, mobile_apis::Result::DISALLOWED, "");
    return;
  }
+  if (!resource_allocation_manager_.is_rc_enabled()) {


shall be PR #2197

yang1070 · 2018-05-29T18:30:28Z

src/components/application_manager/src/hmi_state.cc

@@ -227,7 +227,8 @@ mobile_apis::HMILevel::eType AudioSource::hmi_level() const {
  // Should be investigated (used in multiple places here), since looks weird
  if (Compare<HMILevel::eType, EQ, ONE>(parent()->hmi_level(),
                                        HMILevel::HMI_BACKGROUND,
-                                        HMILevel::HMI_NONE)) {
+                                        HMILevel::HMI_NONE,
+                                        HMILevel::HMI_FULL)) {


why need this change?

@yang1070 This is a fix for the case when a media remote control app is in FULL and it sends a SetInteriorVehicleData to change the audio source to Radio and has keepContext=true, HMI sends AUDIO_SOURCE event to core, SDL shall keep the app’s HMI in FULL and set audioStreamingState to NOT_AUDIBLE.
(before SDL brings app from FULL state to BACKGROUND)

@LitvinenkoIra Got it. Thanks,

yang1070 · 2018-05-29T18:34:48Z

src/components/interfaces/HMI_API.xml

+         <description>
+         This parameter shall not be present in any getter responses or notifications.
+         This parameter is optional in a setter request. The default value is false.
+         If it is true, the system not only changes the audio source but also brings the default application


I have it wrong (the opposite) in the proposal.
Shall be
If it is false, the system not only changes the audio source but also brings the default application ...
If it is true, the system changes the audio source, but still keeps the current application in foreground.

yang1070 · 2018-05-29T18:44:41Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

+
+  // audio
+  mapping["source"] = "sourceAvailable";
+  mapping["keepContext"] = "sourceAvailable";


I think adding a keepContextAvailable to the capabilities in mobile api and hmi api xml might be a good way to resolve this
mapping["keepContext"] = "keepContextAvailable";

+ <struct name="AudioControlCapabilities"> + <param name="moduleName" type="String" maxlength="100" mandatory="true"> + <description> + The short friendly name of the light control module. + It should not be used to identify a module by mobile application. + </description> + </param> + <param name="sourceAvailable" type="Boolean" mandatory="false"> + <description>Availability of the control of audio source. </description> + </param> + <param name="keepContextAvailable" type="Boolean" mandatory="false"> + <description>Availability of the keepContext paramter. </description> + </param> + <param name="volumeAvailable" type="Boolean" mandatory="false"> + <description>Availability of the control of audio volume.</description> + </param> + <param name="equalizerAvailable" type="Boolean" mandatory="false"> + <description>Availability of the control of Equalizer Settings.</description> + </param> + <param name="equalizerMaxChannelId" type="Integer" minvalue="1" maxvalue="100" mandatory="false"> + <description>Must be included if equalizerAvailable=true, and assume all IDs starting from 1 to this value are valid</description> + </param> + </struct>

yang1070 · 2018-05-29T18:46:09Z

...onents/application_manager/rpc_plugins/rc_rpc_plugin/src/resource_allocation_manager_impl.cc

@@ -219,6 +220,14 @@ void ResourceAllocationManagerImpl::SetResourceAquired(
  allocated_resources_[module_type] = app_id;
 }

+bool ResourceAllocationManagerImpl::is_rc_enabled() const {


yang1070 · 2018-05-29T18:46:52Z

src/components/application_manager/src/application_impl.cc

@@ -292,6 +292,13 @@ const HmiStatePtr ApplicationImpl::RegularHmiState() const {
  return state_.GetState(HmiState::STATE_ID_REGULAR);
 }

+bool ApplicationImpl::IsAllowedToChangeAudioSource() const {
+  if (!is_remote_control_supported() || !is_media_application()) {


return (is_remote_control_supported() && is_media_application());

yang1070 · 2018-05-29T18:51:52Z

src/components/interfaces/HMI_API.xml

+     <param name="sourceAvailable" type="Boolean" mandatory="false">
+         <description>Availability of the control of audio source. </description>
+     </param>
+     <param name="volumeAvailable" type="Boolean" mandatory="false">


add

+ <param name="keepContextAvailable" type="Boolean" mandatory="false"> + <description>Availability of the keepContext paramter. </description> + </param>

yang1070 · 2018-05-29T18:52:57Z

src/components/interfaces/MOBILE_API.xml

+        </param>
+        <param name="sourceAvailable" type="Boolean" mandatory="false">
+            <description>Availability of the control of audio source. </description>
+        </param>


add

+ <param name="keepContextAvailable" type="Boolean" mandatory="false"> + <description>Availability of the keepContext paramter. </description> + </param>

yang1070 · 2018-05-29T18:55:15Z

src/components/interfaces/MOBILE_API.xml

+            <description>
+            This parameter shall not be present in any getter responses or notifications.
+            This parameter is optional in a setter request. The default value is false.
+            If it is true, the system not only changes the audio source but also brings the default application


I have it wrong in the proposal.
shall be

If it is false, the system not only changes the audio source but also brings the default application ... If it is true, the system changes the audio source, but still keeps the current application in foreground.

ghost · 2018-08-15T19:03:06Z

@yang1070 @jacobkeeler All comments were fixed.

jacobkeeler · 2018-08-15T19:11:36Z

src/components/interfaces/MOBILE_API.xml

-                Availability of the reading of current temperature.
-                True: Available, False: Not Available, Not present: Not Available.
-            </description>
+        <param name="currentTemperatureAvailable" type="Boolean" mandatory="false" since="5.0">


Why was description removed?

@jacobkeeler According to your first comment in #2207 (review)

I was referring to the parameter as a whole, not the description. currentTemperatureAvailable should not be added in this PR.

@jacobkeeler yes, this is not related to the proposal, I suppose it was done as bugfix.

This parameter is added with 36f37bb (within RemoteControllbaseline)
#1708

Then it was removed with ccd116f

The proposal https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0071-remote-control-baseline.md also does not contain this parameter. But I suppose that it is the bug, because currentTemperature parameters exist in ClimateControlData. I will remove it from this PR with separate commit 6575ede

jacobkeeler · 2018-08-16T00:26:27Z

src/components/interfaces/HMI_API.xml

   <element name="SEAT"/>
+   <element name="RADIO"/>


Should not rearrange existing order of ModuleTypes

@jacobkeeler rearrange removed

jacobkeeler · 2018-08-16T00:27:34Z

src/components/interfaces/MOBILE_API.xml

        <element name="RADIO"/>
-        <element name="SEAT" since="5.0"/>


Should not rearrange order of existing modules

@jacobkeeler rearrange removed

Mobile API and HMI capabilities for the rc new modules Fix RC defects - Fixed Mobile & HMI API: removed redundant default value - FIxed GetInteriorVehicleData request behavior - Fixed OnInteriorDataNotification behavior Fix header guards and add missed includes Fix control data checking by capabilities Answered to review comments Commit

Save current audio source and check it in SetInteriorVehicleDataRequest Save current audio source in app manager Fix SDL behavior when app wants to change audio source

…event to core UTs to verify that SDL keeps app's HMI level in FULL in case when HMI sends AUDIO_SOURCE even

…ilities

jacobkeeler · 2018-08-16T14:01:33Z

src/components/interfaces/HMI_API.xml

-       Availability of the reading of current temperature.
-       True: Available, False: Not Available, Not present: Not Available.
-     </description>
-   </param>


If this already exists in the HMI API, it shouldn't be removed from here. This discrepancy should be handled in another PR.

jacobkeeler · 2018-08-16T14:11:58Z

src/components/interfaces/HMI_API.xml

+     <param name="volume" type="Integer" mandatory="false" minvalue="0" maxvalue="100">
+         <description>Reflects the volume of audio, from 0%-100%.</description>
+     </param>
+     <param name="equalizerSettings" type="EqualizerSettings" minsize="1" maxsize="100" mandatory="false" array ="true">


unnecessary space in array ="true"

jacobkeeler · 2018-08-16T14:14:39Z

src/components/interfaces/MOBILE_API.xml

+        <param name="blue" type="Integer" minvalue="0" maxvalue="255" mandatory="true" />
+    </struct>
+
+    <struct name="TemplateColorScheme" since="5.0">


No reason to move this struct.

@jacobkeeler there is the reason to move RGBColor, and I just would like to keep it near the TemplateColorScheme because they are related to the same area. Should I move TemplateColorScheme back?

Already taken care of

AStasiuk · 2018-08-16T15:11:39Z

This PR was successfully rebased, all feature related ATF tests are PASSED.
@yang1070 could you please review provided changes?

yang1070 · 2018-08-16T20:33:38Z

The change looks good to me.

BSolonenko force-pushed the feature/rc_refactoring branch 2 times, most recently from 4a97805 to 4ce2e58 Compare May 23, 2018 13:58

LitvinenkoIra force-pushed the feature/rc_refactoring branch from 4ce2e58 to 379e61b Compare May 24, 2018 11:05

LitvinenkoIra force-pushed the feature/rc_new_modules branch from da0b8e0 to e611e63 Compare May 24, 2018 12:14

AStasiuk requested review from askirk, mrapitis, yang1070 and robinmk May 25, 2018 19:43

LitvinenkoIra mentioned this pull request May 29, 2018

RPC design refactoring #2196

Closed

1 task

yang1070 reviewed May 29, 2018

View reviewed changes

LitvinenkoIra force-pushed the feature/rc_refactoring branch from 72cc17e to 6f85170 Compare May 30, 2018 08:53

jacobkeeler reviewed Aug 15, 2018

View reviewed changes

jacobkeeler requested changes Aug 16, 2018

View reviewed changes

LuxoftAKutsan force-pushed the feature/rc_new_modules branch from 89ce800 to 0cc67a5 Compare August 16, 2018 10:20

ghost force-pushed the feature/rc_new_modules branch from 0cc67a5 to 1107103 Compare August 16, 2018 11:43

LuxoftAKutsan and others added 11 commits August 16, 2018 16:16

Save current audio source

64de936

Save current audio source and check it in SetInteriorVehicleDataRequest Save current audio source in app manager Fix SDL behavior when app wants to change audio source

Fix cutting off READ ONLY param channelName

cdfcf25

Fixed resend keepContext parameter in GetInteriorVehicleData response

11ad0a4

Fix keep app's HMI level in FULL in case when HMI sends AUDIO_SOURCE …

067a915

…event to core UTs to verify that SDL keeps app's HMI level in FULL in case when HMI sends AUDIO_SOURCE even

Fix comments after review

a93f82e

Check for projection app

d136289

Fix capabilities validation

7d531eb

Fix audio source checking

e9136bf

Add API version to logging in case if message from mobile was not parsed

77c92d2

Remove currentTemperatureAvailable parameter from ClimateControlCapab…

742dbbd

…ilities

LuxoftAKutsan force-pushed the feature/rc_new_modules branch from 6575ede to 742dbbd Compare August 16, 2018 13:17

jacobkeeler reviewed Aug 16, 2018

View reviewed changes

Fix issues after rebase

5add6c2

jacobkeeler force-pushed the feature/rc_new_modules branch from daef596 to 5add6c2 Compare August 16, 2018 15:20

Increase size of modules to fix UTs

80b868e

jacobkeeler approved these changes Aug 16, 2018

View reviewed changes

jacobkeeler merged commit 9bdf1c8 into develop Aug 17, 2018

jacobkeeler mentioned this pull request Aug 24, 2018

[SDL 0099] New remote control modules (LIGHT, AUDIO, HMI_SETTINGS) and parameters (SIS Data) #1798

Closed

This was referenced Sep 27, 2018

Add AUDIO, LIGHT, HMI_SETTINGS to RC smartdevicelink/rpc_spec#120

Merged

Remove keepContextAvailable from API #2646

Closed

Feature/New remote control modules and parameters #2207

Feature/New remote control modules and parameters #2207

Conversation

LitvinenkoIra commented May 23, 2018 • edited by LuxoftAKutsan Loading

Risk

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

Choose a reason for hiding this comment

yang1070 May 30, 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

LitvinenkoIra May 30, 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

yang1070 May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yang1070 May 29, 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

ghost commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkeeler Aug 15, 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

jacobkeeler Aug 16, 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

AStasiuk commented Aug 16, 2018

yang1070 commented Aug 16, 2018

LitvinenkoIra commented May 23, 2018 •

edited by LuxoftAKutsan

Loading

yang1070 May 30, 2018 •

edited

Loading

LitvinenkoIra May 30, 2018 •

edited

Loading

yang1070 May 29, 2018 •

edited

Loading

yang1070 May 29, 2018 •

edited

Loading

jacobkeeler Aug 15, 2018 •

edited

Loading

jacobkeeler Aug 16, 2018 •

edited

Loading