Implement SDL-0221 Remote Control - Allow Multiple Modules per Module Type #1361

SatbirTanda · 2019-07-29T11:25:16Z

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Added Unit Tests

Summary

Create 2 new RPCs (GetInteriorVehicleDataConsent & ReleaseInteriorVehicleDataModule) and add moduleId and moduleType param to existing classes. Deprecate SDLSeatLocation and a few initializers.

CLA

I have signed the CLA

Deprecate SupportedSeat & SDLSupportedSeat Id

Parse moduleId params

Fix errors

Fix SDL warning

Fix logic errors

theresalech · 2019-07-29T15:18:45Z

@SatbirTanda as there is a currently an Evolution Proposal in review regarding this feature (here), we will need to wait for that proposal to be voted on by the SDLC Steering Committee before reviewing this PR. The vote is scheduled to take place 2019-07-30.

SatbirTanda · 2019-08-05T23:11:45Z

@theresalech ready for review

justingluck93

Also please make sure to add SeatLocationCapability to the SDLSystemCapabilityManager in order for a developer to access this information.

justingluck93 · 2019-08-22T13:32:38Z

SmartDeviceLink/SDLAudioControlCapabilities.h

+/**
+ *  Information about a RC module, including its id.
+ *
+ *  SDLModuleInfo


Please mark this as optional in the documention

justingluck93 · 2019-08-22T13:36:10Z

SmartDeviceLink/SDLButtonCapabilities.h

@@ -50,7 +50,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 *  SDLModuleInfo


Mark documentation to state this is optional

justingluck93 · 2019-08-22T13:39:10Z

SmartDeviceLink/SDLButtonPress.h

@@ -28,7 +28,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 *  SDLModuleInfo


Please be sure to mark all optimal parameters as optional if needed.

justingluck93 · 2019-08-22T13:44:47Z

SmartDeviceLink/SDLClimateControlCapabilities.h

@@ -162,7 +162,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 *  SDLModuleInfo


Please add that this is optional in documentation

see SDLSystemCapabilityManager.h for examples

justingluck93 · 2019-08-22T13:52:01Z

SmartDeviceLink/SDLClimateControlCapabilities.m

@@ -42,30 +42,6 @@ - (instancetype)initWithModuleName:(NSString *)moduleName fanSpeedAvailable:(BOO
    return self;
 }



There should be an init that includes the new parameter with all properties like you did in the other capabilities files.

justingluck93 · 2019-08-22T14:31:14Z

SmartDeviceLink/SDLHMISettingsControlCapabilities.h

@@ -86,7 +86,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 *  SDLModuleInfo


Please update the documentation to state this is optional

justingluck93 · 2019-08-22T14:33:30Z

SmartDeviceLink/SDLLightControlCapabilities.h

@@ -51,7 +51,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 *  SDLModuleInfo


Please update documentation to state this is optional

justingluck93 · 2019-08-22T16:26:32Z

SmartDeviceLink/SDLClimateControlCapabilities.m

@@ -42,30 +42,6 @@ - (instancetype)initWithModuleName:(NSString *)moduleName fanSpeedAvailable:(BOO
    return self;
 }



I believe this class have an init that includes moduleInfo like you did in SDLAudioControlCapabilities.h

justingluck93 · 2019-08-22T16:27:15Z

SmartDeviceLink/SDLRadioControlCapabilities.h

@@ -197,7 +197,7 @@ NS_ASSUME_NONNULL_BEGIN
 *
 *  SDLModuleInfo
 */
-@property (strong, nonatomic) SDLModuleInfo *moduleInfo;
+@property (nullable, strong, nonatomic) SDLModuleInfo *moduleInfo;



I believe this class should have an init that includes moduleInfo like you did in SDLAudioControlCapabilities.h

justingluck93 · 2019-08-22T16:46:41Z

SmartDeviceLink/SDLButtonPress.h

@@ -28,7 +28,7 @@ NS_ASSUME_NONNULL_BEGIN
 *


Why was a convince init not added that includes the menuID?

SatbirTanda · 2019-08-22T23:21:13Z

@theresalech ready for rereview

justingluck93

Just some small minor things left, looks good overall

justingluck93 · 2019-08-23T13:10:05Z

SmartDeviceLink/SDLButtonPress.h

@@ -15,14 +15,23 @@ NS_ASSUME_NONNULL_BEGIN

 @interface SDLButtonPress : SDLRPCRequest

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType;
+- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType __deprecated_msg(("Use initWithButtonName:moduleType:moduleId instead"));;


Suggested change

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType __deprecated_msg(("Use initWithButtonName:moduleType:moduleId instead"));;

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType)moduleType __deprecated_msg(("Use initWithButtonName:moduleType:moduleId: instead"));;

justingluck93 · 2019-08-23T13:10:19Z

SmartDeviceLink/SDLButtonPress.h

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType;
+- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType __deprecated_msg(("Use initWithButtonName:moduleType:moduleId instead"));;
+
+- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType moduleId:(nullable NSString *)moduleId;


Suggested change

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType moduleId:(nullable NSString *)moduleId;

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType)moduleType moduleId:(nullable NSString *)moduleId;

justingluck93 · 2019-08-23T13:11:22Z

SmartDeviceLink/SDLButtonPress.m

@@ -32,6 +32,19 @@ - (instancetype)initWithButtonName:(SDLButtonName) buttonName moduleType:(SDLMod
    return self;
 }

+- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType moduleId:(nullable NSString *)moduleId {


Suggested change

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType moduleId:(nullable NSString *)moduleId {

- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType)moduleType moduleId:(nullable NSString *)moduleId {

SatbirTanda · 2019-08-23T20:38:29Z

@theresalech ready for review

justingluck93

Everything looks good from a code review. I still have to test this feature against core.

justingluck93

While testing I found one more thing that needs to be addressed, all the file imports you added to SmartDeviceLink.podspec also needs to be added to SmartDeviceLink-iOS.podspec. Please make those changes. Please also double check that all needed files have be added to those two files.

SatbirTanda · 2019-08-26T22:58:06Z

@theresalech ready for re-review

justingluck93

One more minor update, it appears Request Handlers are not working for the new RPCs. To fix this please add SDLDidReceiveGetInteriorVehicleDataConsentResponse and SDLDidReceiveReleaseInteriorVehicleDataModuleResponse to SDLNotificationConstants.ms allResponseNamesarray

Make recommended fix

SatbirTanda · 2019-08-28T19:46:00Z

@theresalech ready for review

justingluck93 · 2019-08-28T19:53:12Z

Changes look good. Still need to test against core when core issues are fixed.

AStasiuk · 2019-08-29T04:51:00Z

@justingluck93, there are no opened issues in core right now. Could you please specify about which issues are talking about?

justingluck93 · 2019-08-29T05:00:03Z

The issues is with an open PR smartdevicelink/sdl_core#2984 not anything currently in core. It deals with getting consent to control a module.

AStasiuk · 2019-08-29T05:46:49Z

@justingluck93 , could you please specify preconditions, steps for reproduction and SDL logs.
Suggested to put all this info in smartdevicelink/sdl_core#2984

IGapchuk · 2019-09-02T15:02:36Z

@SatbirTanda Please be informed, that SDL Core's behavior has been changed during SDL Core's changes review. Please take a look at the following comments:

Feature/Remote Control - Allow Multiple Modules per Module Type sdl_core#2984 (comment)
Feature/Remote Control - Allow Multiple Modules per Module Type sdl_core#2984 (comment)
Feature/Remote Control - Allow Multiple Modules per Module Type sdl_core#2984 (comment)

that describe the new behavior for getting consent to control a module for all accessMode flows:

1. If accessMode=AUTO_ALLOW, return true by default
2. If accessMode=AUTO_DENY, check if another app is using the resource
   a. If the resource is IN_USE, return false
   b. If the resource is FREE, return true
3. If accessMode=ASK_DRIVER, check if the consent is cached
 a. If it is cached, return that result
 b. If it is NOT cached, check if another app is using the resource
    i) If the resource is FREE(and the user in the resource’s serviceArea), return true
    ii) If the resource is IN_USE, prompt the driver for consent 
   b. If it is NOT cached, then prompt the driver for consent

Also in these comments, you can find references to commits with appropriate changes in SDL Core.
If you will have comments on these changes, please feel free to provide us with them.

justingluck93 · 2019-09-03T14:24:24Z

One last thing. I just got information that the Mobile api needs to be changed slightly.

GetInteriorVehicleDataConsent response class needs to be updated.

The allowed property is now NOT mandatory.

<param name=“allowed” type=“Boolean” array=“true” mandatory=“*true*“>
was changed to
<param name=“allowed” type=“Boolean” array=“true” mandatory=“*false*“>

Please make this change in all needed places.

justingluck93

Just some minor changes due to a change in the mobile API

justingluck93 · 2019-09-03T14:38:34Z

SmartDeviceLink/SDLGetInteriorVehicleDataConsentResponse.h

+
+ Required
+ */
+@property (strong, nonatomic) NSArray<NSNumber<SDLBool> *> *allowed;


This should now be changed to

Suggested change

@property (strong, nonatomic) NSArray<NSNumber<SDLBool> *> *allowed;

@property (strong, nonatomic, nullable) NSArray<NSNumber<SDLBool> *> *allowed;

justingluck93 · 2019-09-03T14:38:55Z

SmartDeviceLink/SDLGetInteriorVehicleDataConsentResponse.m

+}
+#pragma clang diagnostic pop
+
+- (void)setAllowed:(NSArray<NSNumber<SDLBool> *> *)allowed {


Suggested change

- (void)setAllowed:(NSArray<NSNumber<SDLBool> *> *)allowed {

- (void)setAllowed:(nullable NSArray<NSNumber<SDLBool> *> *)allowed {

justingluck93 · 2019-09-03T14:39:05Z

SmartDeviceLink/SDLGetInteriorVehicleDataConsentResponse.m

+    [self.parameters sdl_setObject:allowed forName:SDLRPCParameterNameAllowed];
+}
+
+- (NSArray<NSNumber<SDLBool> *> *)allowed {


Suggested change

- (NSArray<NSNumber<SDLBool> *> *)allowed {

- (nullable NSArray<NSNumber<SDLBool> *> *)allowed {

Make allowed param to be optional

…ules

SatbirTanda · 2019-09-03T20:05:20Z

@theresalech ready for review

SatbirTanda added 17 commits July 29, 2019 04:46

Create SDLModuleInfo & SDLGrid Structs

c38d5ca

Add Module Info to Capabilities

0a09991

Deprecate SupportedSeat & SDLSupportedSeat Id

Create SeatLocation, SeatLocationCapability

a2b7b1c

Parse moduleId params

Add required properties

a4318c3

Add SDLGetInteriorVehicleDataConsent Request

16ccfb0

Add SDLGetInteriorVehicleDataConsentResponse

b30e4e2

Add SDLReleaseInteriorVehicleDataModule Request

9d110ff

Add SDLReleaseInteriorVehicleDataModuleResponse

fd8e433

Make Structs, Req, Responses public

9ff8f0c

Fix errors

Add modueId to initalizer of SDLGetInteriorVehicleData

62d782d

Add Capability tests

01b673e

Properly decreate enum

252fbb3

Fix SDL warning

Add Tests

26db52e

Fix logic errors

Add SDLSeatLocationCapabilitySpec

293a6f9

Add SDLSeatLocationSpec

e426109

Add SDLGetInteriorVehicleDataConsent Tests

fcfb1bf

Add SDLReleaseInteriorVehicleDataModule Tests

1a533e3

SatbirTanda force-pushed the feature/#1272/multiple_modules branch from 3a29126 to 1a533e3 Compare July 29, 2019 12:05

Fix merge conflicts

70d39f8

NicoleYarroch assigned SatbirTanda Jul 29, 2019

NicoleYarroch added enhancement proposal Accepted SDL Evolution Proposal labels Jul 29, 2019

NicoleYarroch added this to the 6.4.0 milestone Jul 29, 2019

joeljfischer requested review from NicoleYarroch and removed request for NicoleYarroch July 29, 2019 13:21

Silence warnings

f09d255

theresalech requested a review from justingluck93 August 6, 2019 13:01

justingluck93 suggested changes Aug 22, 2019

View reviewed changes

SatbirTanda added 3 commits August 22, 2019 14:54

Fix documentation

87addfe

Add new initializers and deprecate old ones

f9d73d0

Add SDLSeatLocationCapability to SDLSystemCapabilityManager

36ceb96

justingluck93 suggested changes Aug 23, 2019

View reviewed changes

Make recommended fixes

3e0d2cd

justingluck93 reviewed Aug 26, 2019

View reviewed changes

justingluck93 suggested changes Aug 26, 2019

View reviewed changes

Update SmartDeviceLink-iOS.podspec

5289257

justingluck93 suggested changes Aug 28, 2019

View reviewed changes

Update SDLNotificationConstants.m

46cb648

Make recommended fix

justingluck93 suggested changes Sep 3, 2019

View reviewed changes

SatbirTanda added 2 commits September 3, 2019 12:26

Update SDLGetInteriorVehicleDataConsentResponse

b6db8a7

Make allowed param to be optional

Merge branch 'develop' into feature/smartdevicelink#1272/multiple_mod…

44b2929

…ules

justingluck93 approved these changes Sep 4, 2019

View reviewed changes

joeljfischer merged commit 8b78386 into smartdevicelink:develop Sep 4, 2019

theresalech mentioned this pull request Sep 5, 2019

[SDL 0221] Remote Control - Allow Multiple Modules per Module Type #1272

Closed

		@@ -42,30 +42,6 @@ - (instancetype)initWithModuleName:(NSString *)moduleName fanSpeedAvailable:(BOO
		return self;
		}

	- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType) moduleType __deprecated_msg(("Use initWithButtonName:moduleType:moduleId instead"));;
	- (instancetype)initWithButtonName:(SDLButtonName)buttonName moduleType:(SDLModuleType)moduleType __deprecated_msg(("Use initWithButtonName:moduleType:moduleId: instead"));;

	@property (strong, nonatomic) NSArray<NSNumber<SDLBool> > allowed;
	@property (strong, nonatomic, nullable) NSArray<NSNumber<SDLBool> > allowed;

	- (void)setAllowed:(NSArray<NSNumber<SDLBool> > )allowed {
	- (void)setAllowed:(nullable NSArray<NSNumber<SDLBool> > )allowed {

	- (NSArray<NSNumber<SDLBool> > )allowed {
	- (nullable NSArray<NSNumber<SDLBool> > )allowed {

Implement SDL-0221 Remote Control - Allow Multiple Modules per Module Type #1361

Implement SDL-0221 Remote Control - Allow Multiple Modules per Module Type #1361

Conversation

SatbirTanda commented Jul 29, 2019

Risk

Testing Plan

Summary

CLA

theresalech commented Jul 29, 2019

SatbirTanda commented Aug 5, 2019

justingluck93 left a 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

SatbirTanda commented Aug 22, 2019 • edited Loading

justingluck93 left a comment

Choose a reason for hiding this comment

justingluck93 Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SatbirTanda commented Aug 23, 2019

justingluck93 left a comment • edited Loading

Choose a reason for hiding this comment

justingluck93 left a comment • edited Loading

Choose a reason for hiding this comment

SatbirTanda commented Aug 26, 2019

justingluck93 left a comment

Choose a reason for hiding this comment

SatbirTanda commented Aug 28, 2019

justingluck93 commented Aug 28, 2019

AStasiuk commented Aug 29, 2019

justingluck93 commented Aug 29, 2019

AStasiuk commented Aug 29, 2019

IGapchuk commented Sep 2, 2019

justingluck93 commented Sep 3, 2019

justingluck93 left a comment • 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

SatbirTanda commented Sep 3, 2019

SatbirTanda commented Aug 22, 2019 •

edited

Loading

justingluck93 Aug 23, 2019 •

edited

Loading

justingluck93 left a comment •

edited

Loading

justingluck93 left a comment •

edited

Loading

justingluck93 left a comment •

edited

Loading