[SDL 0234] Proxy Library RPC Generation

[theresalech]

Proposal: Proxy Library RPC Generation

This proposal adds automatic RPC generation for iOS and Java from the XML spec via a python script.

Review: smartdevicelink/sdl_evolution#741

Steering Committee Decision:

The Steering Committee voted to approve this proposal with the following revision: there will be parsing code in each repository, and the rpc spec will be a submodule that pins which rpc spec will be used in each version of the library.

The proposal .md file was updated to reflect these revisions on 6/10/19.

[theresalech]

Proposal markdown file has been updated per the revisions included in accepted Evolution Proposal SDL 0234 Revisions - Proxy Library RPC Generation. Please reference issue and proposal markdown file for more details.

[MSavitsk]

Hello to all project maintainers! Due to an overall review of the RPC related part of the iOS library, we have faced the following major obstacles:

Enums:
51 of 56 Enums (Declaration & Implementation files) include naming which differ from those, declared in MOBILE_API.XML
See the comparison of name and ios_name in the following table:

enum.name	enum.param.name	iOS.name
SystemContext	VRSESSION	VoiceRecognitionSession
	VEHICLEDATA_GPS	GPS
	VEHICLEDATA_SPEED	Speed
	VEHICLEDATA_FUELLEVEL	FuelLevel
	VEHICLEDATA_FUELLEVEL_STATE	FuelLevelState
	VEHICLEDATA_FUELCONSUMPTION	FuelConsumption
	VEHICLEDATA_EXTERNTEMP	ExternalTemperature
	VEHICLEDATA_VIN	VIN
	VEHICLEDATA_PRNDL	PRNDL
	VEHICLEDATA_TIREPRESSURE	TirePressure
	VEHICLEDATA_ODOMETER	Odometer
	VEHICLEDATA_BELTSTATUS	BeltStatus
	VEHICLEDATA_ODOMETER	Odometer
	VEHICLEDATA_BODYINFO	BodyInfo
	VEHICLEDATA_DEVICESTATUS	DeviceStatus
	VEHICLEDATA_ECALLINFO	ECallInfo
	VEHICLEDATA_AIRBAGSTATUS	AirbagStatus
	VEHICLEDATA_EMERGENCYEVENT	EmergencyEvent
	VEHICLEDATA_CLUSTERMODESTATUS	ClusterModeStatus
	VEHICLEDATA_MYKEY	MyKey
	VEHICLEDATA_BRAKING	Braking
	VEHICLEDATA_WIPERSTATUS	WiperStatus
	VEHICLEDATA_HEADLAMPSTATUS	HeadlampStatus
	VEHICLEDATA_BATTVOLTAGE	BatteryVoltage
	VEHICLEDATA_ENGINETORQUE	EngineTorque
	VEHICLEDATA_ACCPEDAL	AccelerationPedal
	VEHICLEDATA_STEERINGWHEEL	SteeringWheel
	VEHICLEDATA_TURNSIGNAL	TurnSignal
	VEHICLEDATA_FUELRANGE	FuelRange
	VEHICLEDATA_ENGINEOILLIFE	EngineOilLife
	VEHICLEDATA_ELECTRONICPARKBRAKESTATUS	ElectronicParkBrakeStatus
	VEHICLEDATA_CLOUDAPPVEHICLEID	CloudAppVehicleID
	VEHICLEDATA_OEM_CUSTOM_DATA	OEMVehicleDataType
CharacterSet	TYPE2SET	Type2
	TYPE5SET	Type5
	CID1SET	CID1
	CID2SET	CID2
TextAlignment	LEFT_ALIGNED	Left
	RIGHT_ALIGNED	Right
	CENTERED	Center
DeviceLevelStatus	ZERO_LEVEL_BARS	ZeroBars
	ONE_LEVEL_BARS	OneBar
	TWO_LEVEL_BARS	TwoBars
	THREE_LEVEL_BARS	ThreeBars
	FOUR_LEVEL_BARS	FourBars
ECallConfirmationStatus	CALL_IN_PROGRESS	InProgress
	CALL_CANCELLED	Cancelled
	CALL_COMPLETED	Completed
	CALL_UNSUCCESSFUL	Unsuccessful
	ECALL_CONFIGURED_OFF	ConfiguredOff
	CALL_COMPLETE_DTMF_TIMEOUT	CompleteDTMFTimeout
ServiceUpdateReason	PUBLISHED	SDLServiceUpdatePublished
	REMOVED	SDLServiceUpdateRemoved
	ACTIVATED	SDLServiceUpdateActivated
	DEACTIVATED	SDLServiceUpdateDeactivated
	MANIFEST_UPDATE	SDLServiceUpdateManifestUpdate

Questions:

1. Shall we hardcode the current iOS namings for already existing enum related classes?
2. Which pattern to follow for the new enum related classes naming to be produced by generator?

Structs:
The declaration & implementation files for structs contain the same naming inconsistencies.
See the comparison of name and ios_name in the following table:

struct.name	struct.param.name	iOS.name
Coordinate	-	SDLLocationCoordinate
OASISAddress	-	SDLOasisAddress
SisData	-	SDLSISData
SingleTireStatus	tpms	monitoringSystemStatus
TouchEvent	id	touchEventId
	ts	timeStamp
	c	coord
RdsData	PS	programService
	RT	radioText
	CT	clockText
	PI	programIdentification
	PTY	programType
	TP	trafficProgramIdentification
	TA	trafficAnnouncementIdentification
	REG	region
LightCapabilities	rgbColorSpaceAvailable	colorAvailable
AppServiceManifest	rpcSpecVersion	maxRPCSpecVersion

Questions:

1. Shall we hardcode the current iOS namings for already existing struct related classes?
2. Which pattern to follow for the new struct related classes naming to be produced by generator?

Functions:
Concerning the declaration & implementation files for functions, we’ve faced the only obstacle.
It seems like the (instancetypes) declaration can not be formed on a basis of MOBILE_API.XML.

SDLCreateWindow.h declaration file (with comments) provided as an example:

// #import "SDLCreateWindow.h"

// #import "NSMutableDictionary+Store.h"
// #import "SDLRPCParameterNames.h"
// #import "SDLRPCFunctionNames.h"

// NS_ASSUME_NONNULL_BEGIN

// @implementation SDLCreateWindow

// #pragma clang diagnostic push
// #pragma clang diagnostic ignored "-Wdeprecated-declarations"
// - (instancetype)init {
//         if (self = [super initWithName:SDLRPCFunctionNameCreateWindow]) {
//     }
//     return self;
// }
// #pragma clang diagnostic pop

/ ============================= constructors which can not be formed from an XML input
/ |- (instancetype)initWithId:(NSUInteger)windowId windowName:(NSString *)windowName windowType:(SDLWindowType)windowType {
/ |         self = [self init]; // constructor which can be formed
/ |         if (!self) { // on a basis of a previously generated
/ |         return nil; // declaration file
/ |     }
/ |     self.windowID = @(windowId);
/ |     self.windowName = windowName;
/ |     self.type = windowType;
/ |     return self;
/ |}
/ |
/ |- (instancetype)initWithId:(NSUInteger)windowId windowName:(NSString *)windowName windowType:(SDLWindowType)windowType associatedServiceType:(nullable NSString *)associatedServiceType duplicateUpdatesFromWindowID:(NSUInteger)duplicateUpdatesFromWindowID {
/ |     self = [self initWithId:windowId windowName:windowName windowType:windowType];
/ |     if (!self) {
/ |         return nil;
/ |     }
/ |     self.associatedServiceType = associatedServiceType;
/ |     self.duplicateUpdatesFromWindowID = @(duplicateUpdatesFromWindowID);
/ |     return self;
/ |}
/ =================================================================

// ----------------------------------- block scope with implementation that can be generated from params defined in XML
// #pragma mark - Getters / Setters
//
// - (void)setWindowID:(NSNumber<SDLUInt> *)windowID {
//     [self.parameters sdl_setObject:windowID forName:SDLRPCParameterNameWindowId];
// }
//
// - (NSNumber<SDLUInt> *)windowID {
//     NSError *error = nil;
//     return [self.parameters sdl_objectForName:SDLRPCParameterNameWindowId ofClass:NSNumber.class error:&error];
// }
//
// - (void)setWindowName:(NSString *)windowName {
//     [self.parameters sdl_setObject:windowName forName:SDLRPCParameterNameWindowName];
// }
//
// - (NSString *)windowName {
//     NSError *error = nil;
//     return [self.parameters sdl_objectForName:SDLRPCParameterNameWindowName ofClass:NSString.class error:&error];
// }
//
// - (void)setType:(SDLWindowType)type {
//     [self.parameters sdl_setObject:type forName:SDLRPCParameterNameType];
// }
//
// - (SDLWindowType)type {
//     NSError *error = nil;
//     return [self.parameters sdl_enumForName:SDLRPCParameterNameType error:&error];
// }
//
// - (void)setAssociatedServiceType:(nullable NSString *)associatedServiceType {
//     [self.parameters sdl_setObject:associatedServiceType forName:SDLRPCParameterNameAssociatedServiceType];
// }
//
// - (nullable NSString *)associatedServiceType {
//     NSError *error = nil;
//     return [self.parameters sdl_objectForName:SDLRPCParameterNameAssociatedServiceType ofClass:NSString.class error:&error];
// }
//
// - (void)setDuplicateUpdatesFromWindowID:(nullable NSNumber<SDLUInt> *)duplicateUpdatesFromWindowID {
//     [self.parameters sdl_setObject:duplicateUpdatesFromWindowID forName:SDLRPCParameterNameDuplicateUpdatesFromWindowID];
// }
//
// - (nullable NSNumber<SDLUInt> *)duplicateUpdatesFromWindowID {
//     NSError *error = nil;
//     return [self.parameters sdl_objectForName:SDLRPCParameterNameDuplicateUpdatesFromWindowID ofClass:NSNumber.class error:&error];
// }
// -------------------------------------------------------------------------------------------------------------------------------------------------------------------

// @end
// NS_ASSUME_NONNULL_END

Questions:

1. All the struct and enum related classes with different naming are used as imports in functions. Even if we implement the generator with the naming from MOBILE_API.XML, it might be useful for the future generated classes but again, how to declare imports for the existing structs or enums with different naming?
2. We can hardcode the instancetype declaration for already existing files, but what pattern should we follow for the instancetypes in new files which will be produced by RPC generator?

Assumptions:

1. Existing classes can be saved as they are, if we create a dictionary with the the names from XML and those from iOS library. In this case the new classes will strictly follow the naming convention from XML.
2. We can either generate default instancetype with all of the properties initialized by default or we can just leave it empty (i.e. with some comments) for library maintainers to implement it manually

[joeljfischer]

General Comments

Unless we are going to make this a major version change without deprecation time, we need to hardcode / keep the existing enum / struct / function names for properties. The alternative is to hardcode the existing values but to deprecate them and to make the generated values the replacements.

Option 1: Hardcode existing property names.
Option 2: Hardcode existing property names but deprecate them and replace them with generated property names.

For enum / struct / function future naming, generally it should be named SDL<Enum Name><Value> in camel case.

Function Imports

For function imports, I'm not sure what you mean by "how to declare imports for the existing structs or enums with different naming?". Do you mean the struct / enum classes themselves are misnamed? Can you provide an example of that? I don't see it in your examples.

For function initializers (and I would assume some struct initializers), we are faced with the following issues: (a) converting them to use iOS' parameter types, (b) deprecating & replacing initializers when parameters change.

(a) is difficult, and I don't think it should be attempted. We should probably deprecate the existing initializers (all of them) and replace with generated ones that use the property types. It's not ideal, but it's better than trying to map it.

(b) should be doable using the history parameters. The only initializers this project usually adds now is one with all mandatory parameters and one with all parameters. The difficult part might be not generating formerly deprecated initializers after a major version change in the app library.

@joeygrover for visibility and comments

[joeygrover]

Can't the generator just be told via command line arguments to not overwrite files? In this case the classes would be untouched on first pass. I don't think hardcoding these exceptions into the script makes sense. I would much rather see an option to append a file with new items than to simply overwrite its content entirely. Using the version and history tags as Joel mentioned it would be a better effort to try this approach than to hardcode each corner case into each of the projects.

[joeljfischer]

Can't the generator just be told via command line arguments to not overwrite files? In this case the classes would be untouched on first pass. I don't think hardcoding these exceptions into the script makes sense. I would much rather see an option to append a file with new items than to simply overwrite its content entirely.

I disagree. I think that's a fairly brittle scheme to have the scripts only function properly if they're appending data to existing files. The script then not only has to read and parse the XML, but they also have to read and parse Obj-C, Java, and JavaScript code, determine the differences and append the data correctly.

I think it's much better to hardcode the current corner cases, deprecate them, and have the autogenerated code be the new current versions. In a future major version we can remove the deprecated / hardcoded code and continue on with the autogenerated code.

[joeygrover]

We have different opinions on how this generator should be used. I do not see the need to recreate class files every RPC spec increase. It should only be used as a tool for a base set of code that is included in the library. As this code will still have to be separated into individual PRs and code changes, only the affected RPCs should be changed at that time. This is generation at the code base level, not runtime; therefore it does not need to be used more than once for a single change.

[MSavitsk]

HI Joel!

Sorry for this improper question "how to declare imports for the existing structs or enums with different naming?" it relates not to imports but to class typing in properties.

For instance, SDLVrCapabilities.h file contains a class definition

#import "SDLEnum.h"

typedef SDLEnum SDLVRCapabilities SDL_SWIFT_ENUM;   // from XML VrCapabilities <=> SDLVRCapabilities

extern SDLVRCapabilities const SDLVRCapabilitiesText;

and then it is used as a type in SDLRegisterAppInterfaceResponse.h

@property (nullable, strong, nonatomic) NSArray<SDLVRCapabilities> *vrCapabilities;

So we already got the answer concerning the naming pattern, and it will be used for new classes to be produced by generator.

Concerning the overall logic, it seems that Option 1 is great for generator implementation, as it follows the same logic as the generators in other libraries.

Thanks for the answer concerning function & struct initializers, the new classes will follow the next logic:

1. default initializer in every class

- (instancetype)init {
    if (self = [super initWithName:SDLRPCFunctionNameRegisterAppInterface]) {
    }
    return self;
}

2. initializer for mandatory params if there is/are any in XML (skipped if no <mandatory = true> params)
3. initializer for all params if there is/are any which is not mandatory in XML (skipped if no <mandatory = false> params)

Is it right?

[vladmu]

@theresalech @joeygrover @joeljfischer could you confirm we have correct assumptions in the last comment?

[joeljfischer]

@vladmu It appears correct to me

[vladmu]

@joeljfischer @joeygrover @theresalech sorry for the repeatable question. We just re-read this thread and seem not to find the final agreement regarding the hardcoding of corner cases. We see different minds here, but maybe it would be better to follow @joeygrover solution and postpone this hardcode at least for the current iteration the same way proposed by @theresalech for documentation of Java RPC? Please assist us with the final direction regarding this? Thank you!

[theresalech]

Hi @vladmu - yes you can hardcode for this implementation as you are for Java Suite. Thanks!

[vladmu]

Hi @vladmu - yes you can hardcode for this implementation as you are for Java Suite. Thanks!

Hi @theresalech did you mean postpone instead of hardcode here?

[theresalech]

@vladmu yes, my apologies for the typo! Please follow the same direction as Java Suite and simply use the documentation that is present in the RPC Spec.

[ghost]

Sorry if I jump into a topic form three weeks ago but I want to bring up one topic from the proposal and from our steering committee meetings:

The proposal states:

The generator could produce bad RPC classes, however, the library authors will ensure that this is not the case and that the first version of the generated code matches what already exists, and the unit tests will continue to pass.

and in our meeting we agreed to proceed with generated code in the libraries for every RPC which is new or modified. We also discussed how to ensure the quality of the generated code. The conclusion was that the existing unit tests will help as an automated black box test of all the generated code.

I am hesitant in accepting this proposal if it introduces that many breaking changes. I brought up some concern in my PR comment #1556 (comment) .

My suggestion would be to investigate how to extract code out of the original RPC class into objc extensions or categories for code that cannot be generated. Those extensions would need to go into SmartDeviceLink.h and into the pbxproj file. Actually I am not sure if the pbxproj file is in scope of the code generator but if it isn't it's a big downside to this feature.

I suggest to postpone this issue into 6.7.0 release allowing more time to work on the implementation. It doesn't affect the other repositories if we won't merge the PR because this proposal isn't a "feature" proposal but rather a maintenance proposal. I understand it may require a SC vote as this proposal was accepted in the timeline. However, I believe it's more important to implement the proposal the right way, not the fastest way.

[joeljfischer]

Hi Kujtim,

and in our meeting we agreed to proceed with generated code in the libraries for every RPC which is new or modified. We also discussed how to ensure the quality of the generated code. The conclusion was that the existing unit tests will help as an automated black box test of all the generated code.

Due to the explosion of complexity and code that is involved with mapping every exception that has ever been made in the iOS project compared to the RPC spec, the decision was made a few weeks ago that we should instead not pursue those mappings. This has, as you noted, several implications:

1. We will not overwrite the existing RPCs. All newly created structs, enums, requests, responses, and notifications will be generated, but new parameters to existing RPCs will have to be manually handled. The script's no-overwrite (or similar) command line switch will be used.
2. Existing unit tests will not be able to be used to verify the output.

The idea is that a future major version could involve a switch-over to using all generated code instead of the current code. It's not perfect for the current version, but it is helpful for adding new RPCs.

Actually I am not sure if the pbxproj file is in scope of the code generator but if it isn't it's a big downside to this feature.

It is not currently in scope. The developer has to add it manually. Neither is are the podspecs or the module header file. That is a downside, but I personally consider it a relatively minor one.

My suggestion would be to investigate how to extract code out of the original RPC class into objc extensions or categories for code that cannot be generated.

In most cases, that is not possible, due to conflicts with newly generated code (e.g. parameters are named different things, documentation differences, etc. etc.).

I disagree with your assessment that this proposal not be implemented until later. I agree that it must be implemented the right way, which is why the maintainers have agreed with the PR authors on the changes needed. As you have noted, you haven't been involved with this discussion for several weeks, so it's understandable that you missed those conversations. However, the proposal and PR authors agreed on these changes as the best path forward. We cannot make a major version change that would be involved with overwriting existing RPC classes, and to map out all the differences would be a huge time and complexity-sink that the PM and PR author agreed was feasible but unmaintainable and undesirable.

[theresalech]

Proposal markdown file has been updated per the revisions included in accepted Evolution Proposal SDL 0234 Revisions - Proxy Library RPC Generation. The revisions include not implementing custom mappings in JavaSuite and iOS libraries. Please reference the issue and proposal markdown file for more details.

[theresalech]

Proposal markdown file has been updated per the revisions included in accepted Evolution Proposal SDL 0234 Revisions - Proxy Library RPC Generation. The revisions include adding a rule that takes into account a set of keywords that currently exist in any of the three client side libraries (iOS, Java Suite, and JavaScript suite), and avoiding creating method signatures that collide. Please reference the issue and proposal markdown file for more details.