Implement SDL 0071 Remote Control Baseline (no zones, no driver/passenger, immediate control) #692

jamescs · 2017-08-17T02:13:38Z

Fixes #650

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Added unit tests for each RPC.

Summary

Implements SDL 0071

Changelog

Enhancements

[Enhancement info]

Tasks Remaining:

Add convenience initializers for new RPC's
Add code documentation

CLA

I have signed the CLA

joeljfischer

Not a full review, only of the first bunch of files, but I found some general stuff to watch for.

joeljfischer · 2017-08-17T17:16:42Z

SmartDeviceLink-iOS.podspec

@@ -47,6 +47,7 @@ ss.public_header_files = [
 'SmartDeviceLink/SDLAddSubMenu.h',
 'SmartDeviceLink/SDLAlert.h',
 'SmartDeviceLink/SDLAlertManeuver.h',
+'SmartDeviceLink/SDLButtonPress.h',


The podspecs need to list every public file, based on looking at the pbxproj changes, most of the new public files are not included.

joeljfischer · 2017-08-17T17:18:55Z

SmartDeviceLink/SDLButtonName.h

+/**
+ * @abstract Represents AC max button *
+ */
+extern SDLButtonName const SDLButtonNameAcMax;


"Ac" here would be "AC" because it's an acronym. Acronyms should be all the same (upper or lower).

joeljfischer · 2017-08-17T17:19:57Z

SmartDeviceLink/SDLButtonName.h

+/**
+ * @abstract Represents AC button *
+ */
+extern SDLButtonName const SDLButtonNameAc;


Also should be "AC"

joeljfischer · 2017-08-17T17:22:45Z

SmartDeviceLink/SDLClimateControlCapabilities.h

+@interface SDLClimateControlCapabilities : SDLRPCStruct
+
+/**
+ * @abstractThe short friendly name of the climate control module.


I don't believe we need the @abstract anymore in the latest versions of Xcode.

joeljfischer · 2017-08-17T17:25:24Z

SmartDeviceLink/SDLClimateControlCapabilities.m

+}
+
+- (void)setDefrostZone:(nullable NSArray <SDLDefrostZone>*)defrostZone {
+    [store sdl_setObject:defrostZone forName:SDLNameDefrostZone];


Careful of random extra spaces.

joeljfischer · 2017-08-17T17:25:59Z

SmartDeviceLink/SDLClimateControlCapabilities.m

+}
+
+- (nullable NSArray<SDLDefrostZone> *)defrostZone {
+    return [store sdl_objectForName:SDLNameDefrostZone];


I believe this is supposed to use sdl_objectsForName:ofClass:

SDLDefrostZone is an SDLEnum, I believe this one may be ok based on the description found here:
#713

joeljfischer · 2017-08-17T17:26:29Z

SmartDeviceLink/SDLClimateControlData.m

+}
+
+- (nullable SDLTemperature *)desiredTemperature {
+    return [store sdl_objectForName:SDLNameDesiredTemperature];


I believe this is supposed to use sdlObjectForName:ofClass:

joeljfischer · 2017-08-17T17:29:31Z

SmartDeviceLink/SDLFunctionID.m

@@ -28,7 +28,7 @@ - (instancetype)init {
    if (!self) {
        return nil;
    }
-
+    


Careful of adding extra whitespace. I'd recommend turning on Xcode->Preferences->Text Editing->Automatically trim trailing whitespace (and also Including whitespace-only lines)

joeljfischer · 2017-08-18T13:11:28Z

@jamescs Note that this branch now has conflicts, sorry!

mrapitis · 2017-08-22T15:35:15Z

SmartDeviceLink/SDLNames.m

@@ -47,6 +56,7 @@
 SDLName const SDLNameButtonCapabilities = @"buttonCapabilities";
 SDLName const SDLNameButtonEventMode = @"buttonEventMode";
 SDLName const SDLNameButtonName = @"buttonName";
+SDLName const SDLNameButtonPress = @"buttonPress";


Can we please change @"buttonPress"; to @"ButtonPress"; to match spec.

mrapitis · 2017-08-28T16:12:13Z

SmartDeviceLink/SDLVentilationMode.h

+/**
+ * @abstract A SDLDefrostZone with the value of *NORTH*
+ */
+extern SDLVentilationMode const SDLVentilationModeNorth;


Please change North to None

mrapitis · 2017-08-28T16:12:25Z

SmartDeviceLink/SDLVentilationMode.m

+SDLVentilationMode const SDLVentilationModeUpper = @"UPPER";
+SDLVentilationMode const SDLVentilationModeLower = @"LOWER";
+SDLVentilationMode const SDLVentilationModeBoth = @"BOTH";
+SDLVentilationMode const SDLVentilationModeNorth = @"NONE";


Please change North to None

mrapitis · 2017-08-28T16:13:14Z

SmartDeviceLink/SDLRadioState.h

+ *  @abstract Represents Radio state as ACQUIRING
+ *
+ */
+extern SDLRadioState const SDLRadioBandAcquiring;


Please change SDLRadioBandAcquiring to SDLRadioStateAcquiring

mrapitis · 2017-08-28T16:13:56Z

SmartDeviceLink/SDLRadioState.m

+
+#import "SDLRadioState.h"
+
+SDLRadioState const SDLRadioBandAcquiring = @"ACQUIRING";


Please change SDLRadioBandAcquiring to SDLRadioStateAcquiring

# Conflicts: # SmartDeviceLink-iOS.podspec # SmartDeviceLink-iOS.xcodeproj/project.pbxproj # SmartDeviceLink.podspec # SmartDeviceLink/SDLFunctionID.m # SmartDeviceLink/SDLNames.h # SmartDeviceLink/SDLNames.m # SmartDeviceLink/SmartDeviceLink.h

joeljfischer · 2017-09-14T13:57:34Z

SmartDeviceLink-iOS.podspec

@@ -164,6 +172,8 @@ ss.public_header_files = [
 'SmartDeviceLink/SDLOnDriverDistraction.h',
 'SmartDeviceLink/SDLOnEncodedSyncPData.h',
 'SmartDeviceLink/SDLOnHashChange.h',
+'SmartDeviceLink/SDLOnInteriorVehicleData.h',
+'SmartDeviceLink/SDLOnInteriorVehicleData',


Appears to be an extra.

joeljfischer · 2017-09-14T14:05:13Z

SmartDeviceLink-iOS.podspec

@@ -283,6 +303,7 @@ ss.public_header_files = [
 'SmartDeviceLink/SDLTransportDelegate.h',
 'SmartDeviceLink/SDLTriggerSource.h',
 'SmartDeviceLink/SDLTTSChunk.h',
+'SmartDeviceLink/SDLTTSChunkFactory.h',


This file no longer exists.

Note that this and the above comment do not appear on the below SmartDeviceLink.podspec.

joeljfischer · 2017-09-14T14:10:27Z

SmartDeviceLink/SDLButtonName.h

+ */
+extern SDLButtonName const SDLButtonNameAC;
+
+


Sometimes this uses one newline, sometimes it two, it should be one.

joeljfischer · 2017-09-14T14:11:51Z

SmartDeviceLink/SDLButtonName.h

+extern SDLButtonName const SDLButtonNameShuffle;
+
+/**
+ * @abstract Represents a Repeat button *


Remove the *

joeljfischer · 2017-09-14T14:16:39Z

SmartDeviceLink/SDLButtonPress.h

+
+@interface SDLButtonPress : SDLRPCRequest
+
+


Make the 2 lines and 1 line consistent with other files. If you stick with 1 line, you should be okay.

joeljfischer · 2017-09-14T18:46:15Z

SmartDeviceLink/SDLTemperature.m

+}
+
+-(instancetype) initWithUnit: (SDLTemperatureUnit) unit andValue:(NSNumber<SDLFloat> *) value {
+    self = [self init];


joeljfischer · 2017-09-14T18:49:57Z

SmartDeviceLinkTests/RPCSpecs/NotificationSpecs/SDLOnInteriorVehicleDataSpec.m

+#import "SDLModuleData.h"
+
+QuickSpecBegin(SDLOnInteriorVehicleDataSpec)
+SDLModuleData* someModuleData = [[SDLModuleData alloc] init];


This should be in a describe in a beforeEach so that it's reset every time.

joeljfischer · 2017-09-14T18:54:59Z

SmartDeviceLinkTests/RPCSpecs/ResponseSpecs/SDLGetInteriorVehicleDataResponseSpec.m

+#import "SDLNames.h"
+
+QuickSpecBegin(SDLGetInteriorVehicleDataResponseSpec)
+SDLModuleData* someModuleData = [[SDLModuleData alloc] init];


Should be in a describe block in a beforeEach

joeljfischer · 2017-09-14T18:55:52Z

SmartDeviceLinkTests/RPCSpecs/ResponseSpecs/SDLSetInteriorVehicleDataResponseSpec.m

+
+QuickSpecBegin(SDLSetInteriorVehicleDataResponseSpec)
+
+SDLModuleData *someModuleData = [[SDLModuleData alloc] init];


Should be in a describe block in a beforeEach

joeljfischer · 2017-09-14T18:58:39Z

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLClimateControlDataSpec.m

+#import "SDLNames.h"
+
+QuickSpecBegin(SDLClimateControlDataSpec)
+__block SDLTemperature* currentTemp = [[SDLTemperature alloc] init];


Should be in a describe block in a beforeEach

joeljfischer · 2017-09-20T19:38:53Z

@mrapitis I'm seeing a merge conflict again

mrapitis · 2017-09-20T19:48:31Z

@joeljfischer thanks, it looks like project.pbxproj is conflicting. We will merge the base and resolve the conflict.

Conflicts: SmartDeviceLink-iOS.xcodeproj/project.pbxproj

mrapitis · 2017-09-20T20:23:41Z

@joeljfischer base branch has been merged and conflicting file resolved. thanks.

codecov-io · 2017-09-21T00:35:56Z

Codecov Report

Merging #692 into release/5.0.0 will increase coverage by 0.02%.
The diff coverage is 55.05%.

@@                Coverage Diff                @@
##           release/5.0.0     #692      +/-   ##
=================================================
+ Coverage          54.43%   54.45%   +0.02%     
=================================================
  Files                318      333      +15     
  Lines               7299     7586     +287     
  Branches             662      677      +15     
=================================================
+ Hits                3973     4131     +158     
- Misses              3085     3213     +128     
- Partials             241      242       +1

# Conflicts: # SmartDeviceLink-iOS.xcodeproj/project.pbxproj

joeljfischer · 2017-09-21T17:28:27Z

SmartDeviceLink/SDLRdsData.h

+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface SDLRdsData : SDLRPCStruct


Because RDS is an acronym, this class should be SDLRDSData

joeljfischer · 2017-09-21T17:33:28Z

SmartDeviceLink/SDLRadioControlData.h

+
+@interface SDLRadioControlData : SDLRPCStruct
+
+- (instancetype)initWithFrequencyInteger:(nullable NSNumber<SDLInt> *)frequencyInteger frequencyFraction:(nullable NSNumber<SDLInt> *)frequencyFraction band:(nullable SDLRadioBand)band rdsData:(nullable SDLRdsData *)rdsData availableHDs:(nullable NSNumber<SDLInt> *)availableHDs hdChannel:(nullable NSNumber<SDLInt> *)hdChannel signalStrength:(nullable NSNumber<SDLInt> *)signalStrength signalChangeThreshold:(nullable NSNumber<SDLInt> *)signalChangeThreshold radioEnable:(nullable NSNumber<SDLBool> *)radioEnable state:(nullable SDLRadioState)state;


This initializer should only contain properties such that are actually currently settable by developers (e.g. rdsData is readonly).

joeljfischer · 2017-09-21T17:51:49Z

SmartDeviceLink/SDLRadioControlData.h

+ *
+ *SDLRdsData
+ */
+@property (nullable, strong, nonatomic) SDLRdsData *rdsData;


A number of these properties are readonly, and the documentation should note it is such.

joeljfischer · 2017-09-21T17:53:41Z

SmartDeviceLink/SDLClimateControlData.h

+ *
+ * Optional
+ */
+@property (nullable, strong, nonatomic) SDLTemperature *currentTemperature;


This property is readonly and the documentation should mark it as such.

joeljfischer · 2017-09-21T17:54:08Z

SmartDeviceLink/SDLClimateControlData.h

+
+@interface SDLClimateControlData : SDLRPCStruct
+
+- (instancetype)initWithFanSpeed:(nullable NSNumber<SDLInt> *)fanSpeed currentTemperature:(nullable SDLTemperature *)currentTemperature desiredTemperature:(nullable SDLTemperature *)desiredTemperature acEnable:(nullable NSNumber<SDLBool> *)acEnable circulateAirEnable:(nullable NSNumber<SDLBool> *)circulateAirEnable autoModeEnable:(nullable NSNumber<SDLBool> *)autoModeEnable defrostZone:(nullable SDLDefrostZone)defrostZone dualModeEnable:(nullable NSNumber<SDLBool> *)dualModeEnable acMaxEnable:(nullable NSNumber<SDLBool> *)acMaxEnable ventilationMode:(nullable SDLVentilationMode)ventilationMode;


This initializer should not contain the currentTemperature due to it being a readonly parameter

joeljfischer · 2017-09-21T17:56:18Z

SmartDeviceLink/SDLNames.m

@@ -56,6 +66,10 @@
 SDLName const SDLNameCharacterSet = @"characterSet";
 SDLName const SDLNameChoiceId = @"choiceID";
 SDLName const SDLNameChoiceSet = @"choiceSet";
+SDLName const SDLNameCirculateAirEnable = @"circulateAirEnableAvailable";


This needs to be circulateAirEnable

joeljfischer · 2017-09-21T20:35:29Z

SmartDeviceLink/SDLRadioControlData.h

@@ -16,7 +16,7 @@ NS_ASSUME_NONNULL_BEGIN

 @interface SDLRadioControlData : SDLRPCStruct

- (instancetype)initWithFrequencyInteger:(nullable NSNumber<SDLInt> *)frequencyInteger frequencyFraction:(nullable NSNumber<SDLInt> *)frequencyFraction band:(nullable SDLRadioBand)band rdsData:(nullable SDLRdsData *)rdsData availableHDs:(nullable NSNumber<SDLInt> *)availableHDs hdChannel:(nullable NSNumber<SDLInt> *)hdChannel signalStrength:(nullable NSNumber<SDLInt> *)signalStrength signalChangeThreshold:(nullable NSNumber<SDLInt> *)signalChangeThreshold radioEnable:(nullable NSNumber<SDLBool> *)radioEnable state:(nullable SDLRadioState)state;
+- (instancetype)initWithFrequencyInteger:(nullable NSNumber<SDLInt> *)frequencyInteger frequencyFraction:(nullable NSNumber<SDLInt> *)frequencyFraction band:(nullable SDLRadioBand)band availableHDs:(nullable NSNumber<SDLInt> *)availableHDs hdChannel:(nullable NSNumber<SDLInt> *)hdChannel signalStrength:(nullable NSNumber<SDLInt> *)signalStrength signalChangeThreshold:(nullable NSNumber<SDLInt> *)signalChangeThreshold radioEnable:(nullable NSNumber<SDLBool> *)radioEnable state:(nullable SDLRadioState)state;


availableHDs, signalStrength signalChangeThreshold state are read only and should be removed from this initializer

It also appears that a few documentation notes about read only are missing.

joeljfischer · 2017-09-25T14:47:19Z

SmartDeviceLinkTests/RPCSpecs/RequestSpecs/SDLGetSystemCapabilitiesSpec.m

@@ -39,6 +39,13 @@
        expect(testRequest.systemCapabilityType).to(beNil());
    });

+    it(@"Should get correctly when initialized with systemCapabilityType", ^ {


This appears to be an entirely identical test to the below test.

joeljfischer · 2017-09-25T14:50:06Z

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLClimateControlCapabilitiesSpec.m

@@ -75,6 +75,21 @@
        expect(testStruct.ventilationModeAvailable).to(equal(@NO));
        expect(testStruct.ventilationMode).to(equal([@[SDLVentilationModeUpper] copy]));
    });
+
+    it(@"Should get correctly when initialized with module data and other climate control capabilite's parameters", ^ {


Spelling "capabilite's" -> "capabilities"

joeljfischer · 2017-09-25T14:52:52Z

SmartDeviceLinkTests/RPCSpecs/StructSpecs/SDLModuleDataSpec.m

+        expect(testStruct.radioControlData).to(equal(someRadioData));
+    });
+
+    it(@"Should get correctly when initialized with ClimateControlData", ^ {


This test and the above test should also test the other parameters and make sure they're nil (see above for how to do that), or whatever they should be.

james and others added 3 commits August 14, 2017 13:04

Added remote control RPCs and unit tests

95dbde5

Fixed merge error

5333b66

Merged public release/5.0.0 branch, resolved merge conflicts

2580db0

jamescs changed the title ~~Feature/rc rpcs~~ WIP: Implment 0071 SDL Remote Control Baseline (no zones, no driver/passenger, immediate control) Aug 17, 2017

jamescs changed the title ~~WIP: Implment 0071 SDL Remote Control Baseline (no zones, no driver/passenger, immediate control)~~ WIP: Implement 0071 SDL Remote Control Baseline (no zones, no driver/passenger, immediate control) Aug 17, 2017

jamescs changed the title ~~WIP: Implement 0071 SDL Remote Control Baseline (no zones, no driver/passenger, immediate control)~~ WIP: Implement SDL 0071 Remote Control Baseline (no zones, no driver/passenger, immediate control) Aug 17, 2017

joeljfischer self-requested a review August 17, 2017 13:05

joeljfischer added the proposal Accepted SDL Evolution Proposal label Aug 17, 2017

joeljfischer added this to the 5.0.0 milestone Aug 17, 2017

joeljfischer added this to In Progress in v5.0.0 Aug 17, 2017

joeljfischer removed this from In Progress in v5.0.0 Aug 17, 2017

joeljfischer suggested changes Aug 17, 2017

View reviewed changes

Added convenience initializers for RC RPCs

77cd2ba

joeljfischer assigned jamescs and unassigned jamescs Aug 18, 2017

mrapitis reviewed Aug 22, 2017

View reviewed changes

mrapitis reviewed Aug 28, 2017

View reviewed changes

odvcencio and others added 8 commits August 28, 2017 14:20

changes to convenience inits, changes based on comments

cdded32

Added missing files.

3bb870e

Modified return method.

c3af59f

Updated podspec files.

d99bb62

Updated getter methods

1d6b339

Updated unit tests

1f3297e

Changes for adding missed files in compile sources under Build Phases.

8989716

jamescs changed the title ~~WIP: Implement SDL 0071 Remote Control Baseline (no zones, no driver/passenger, immediate control)~~ Implement SDL 0071 Remote Control Baseline (no zones, no driver/passenger, immediate control) Sep 14, 2017

joeljfischer suggested changes Sep 14, 2017

View reviewed changes

piyushkhosla added 3 commits September 15, 2017 18:27

Implementing code review comments.

7ea1d7c

Updating init method for buttonpress

daffff9

Removing Unwanted spaces and other review comments.

e39566d

joeljfischer mentioned this pull request Sep 19, 2017

Feature/haptic manager #739

Closed

mrapitis added 2 commits September 19, 2017 11:04

Formatting cleanup.

5d9ef1d

minor changes for review comments

7c09d47

Merge branch 'release/5.0.0' into feature/rc_rpcs

fefedc1

Conflicts: SmartDeviceLink-iOS.xcodeproj/project.pbxproj

mrapitis added 3 commits September 21, 2017 09:16

Adding missing documentation and targeted review comments.

cdb84c4

Minor changes around docs.

6673c23

Merge branch 'release/5.0.0' into feature/rc_rpcs

befbbdd

# Conflicts: # SmartDeviceLink-iOS.xcodeproj/project.pbxproj

joeljfischer suggested changes Sep 21, 2017

View reviewed changes

updates for review comments

bd26632

joeljfischer suggested changes Sep 21, 2017

View reviewed changes

mrapitis and others added 4 commits September 21, 2017 18:14

updates targeting review comments

b61c766

added warning tag for readonly properties

0290aae

updated rpc unit tests with custom initializers

6e75f93

Adding test cases for custom initializer.

2684b57

joeljfischer suggested changes Sep 25, 2017

View reviewed changes

updates for review comments

09aa284

joeljfischer approved these changes Sep 25, 2017

View reviewed changes

joeljfischer merged commit 941d8f8 into smartdevicelink:release/5.0.0 Sep 25, 2017


		#import "SDLRadioState.h"

		SDLRadioState const SDLRadioBandAcquiring = @"ACQUIRING";


		QuickSpecBegin(SDLSetInteriorVehicleDataResponseSpec)

		SDLModuleData *someModuleData = [[SDLModuleData alloc] init];


		@interface SDLRadioControlData : SDLRPCStruct

		- (instancetype)initWithFrequencyInteger:(nullable NSNumber<SDLInt> )frequencyInteger frequencyFraction:(nullable NSNumber<SDLInt> )frequencyFraction band:(nullable SDLRadioBand)band rdsData:(nullable SDLRdsData )rdsData availableHDs:(nullable NSNumber<SDLInt> )availableHDs hdChannel:(nullable NSNumber<SDLInt> )hdChannel signalStrength:(nullable NSNumber<SDLInt> )signalStrength signalChangeThreshold:(nullable NSNumber<SDLInt> )signalChangeThreshold radioEnable:(nullable NSNumber<SDLBool> )radioEnable state:(nullable SDLRadioState)state;


		@interface SDLClimateControlData : SDLRPCStruct

		- (instancetype)initWithFanSpeed:(nullable NSNumber<SDLInt> )fanSpeed currentTemperature:(nullable SDLTemperature )currentTemperature desiredTemperature:(nullable SDLTemperature )desiredTemperature acEnable:(nullable NSNumber<SDLBool> )acEnable circulateAirEnable:(nullable NSNumber<SDLBool> )circulateAirEnable autoModeEnable:(nullable NSNumber<SDLBool> )autoModeEnable defrostZone:(nullable SDLDefrostZone)defrostZone dualModeEnable:(nullable NSNumber<SDLBool> )dualModeEnable acMaxEnable:(nullable NSNumber<SDLBool> )acMaxEnable ventilationMode:(nullable SDLVentilationMode)ventilationMode;

Implement SDL 0071 Remote Control Baseline (no zones, no driver/passenger, immediate control) #692

Implement SDL 0071 Remote Control Baseline (no zones, no driver/passenger, immediate control) #692

Conversation

jamescs commented Aug 17, 2017 • edited

Risk

Testing Plan

Summary

Changelog

Enhancements

Tasks Remaining:

CLA

joeljfischer 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

joeljfischer commented Aug 18, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeljfischer commented Sep 20, 2017

mrapitis commented Sep 20, 2017

mrapitis commented Sep 20, 2017

codecov-io commented Sep 21, 2017 • edited

Codecov Report

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

jamescs commented Aug 17, 2017 •

edited

codecov-io commented Sep 21, 2017 •

edited