Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDL 0234] Proxy Library RPC Generation #1298

Closed
theresalech opened this issue Jun 10, 2019 · 17 comments · Fixed by #1556
Closed

[SDL 0234] Proxy Library RPC Generation #1298

theresalech opened this issue Jun 10, 2019 · 17 comments · Fixed by #1556
Labels
proposal Accepted SDL Evolution Proposal rpc Relating to the RPC layer
Projects

Comments

@theresalech
Copy link
Contributor

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
Copy link
Contributor Author

theresalech commented Dec 2, 2019

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
Copy link

MSavitsk commented Dec 11, 2019

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
Copy link
Contributor

joeljfischer commented Dec 11, 2019

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link

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;
}
  1. initializer for mandatory params if there is/are any in XML (skipped if no <mandatory = true> params)
  2. 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
Copy link
Contributor

vladmu commented Dec 19, 2019

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

@joeljfischer
Copy link
Contributor

@vladmu It appears correct to me

@theresalech theresalech added this to To do in v6.6.0 Jan 21, 2020
@vladmu
Copy link
Contributor

vladmu commented Jan 24, 2020

@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
Copy link
Contributor Author

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

@vladmu
Copy link
Contributor

vladmu commented Jan 28, 2020

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
Copy link
Contributor Author

@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.

@joeljfischer joeljfischer added the rpc Relating to the RPC layer label Jan 28, 2020
@kshala-ford
Copy link
Contributor

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
Copy link
Contributor

joeljfischer commented Feb 18, 2020

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
Copy link
Contributor Author

theresalech commented Mar 11, 2020

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
Copy link
Contributor Author

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.

v6.6.0 automation moved this from Waiting for Review to Done Apr 15, 2020
@joeljfischer joeljfischer mentioned this issue Apr 29, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Accepted SDL Evolution Proposal rpc Relating to the RPC layer
Projects
No open projects
v6.6.0
  
Done
Development

Successfully merging a pull request may close this issue.

6 participants