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

[kosdp]

We found that MOBILE_API.xml enum implementation for sdl_java_suite library (available in path sdl_java_suite/base/src/main/java/com/smartdevicelink/proxy/rpc/enums/) has major inconsistency for enums which have "internal_name" attribute for enum elements. For example, when there is no "internal_name" attribute for enum elements then "name" attribute becomes a name for enum element in Java implementation.

For example, for DisplayMode enum

    <enum name="DisplayMode" since="5.0">
        <element name="DAY"/>
        <element name="NIGHT"/>
        <element name="AUTO"/>
    </enum>

public enum DisplayMode {
	DAY,
	NIGHT,
	AUTO,
	; ... }

But when "internal_name" attribute is present in XML enum, then name of Java enum element is chosen in different ways which varies from one implementation to another.

For example for Dimension enum

    <enum name="Dimension" since="2.0">
        <description>The supported dimensions of the GPS</description>
        <element name="NO_FIX" internal_name="Dimension_NO_FIX">
            <description>No GPS at all</description>
        </element>
        <element name="2D" internal_name="Dimension_2D">
            <description>Longitude and latitude</description>
        </element>
        <element name="3D" internal_name="Dimension_3D">
            <description>Longitude and latitude and altitude</description>
        </element>
    </enum>

public enum Dimension {
    NO_FIX("NO_FIX"),
    _2D("2D"),
    _3D("3D")
    ; ... }

And for SamplingRate enum:

    <enum name="SamplingRate" since="2.0">
        <description>Describes different sampling options for PerformAudioPassThru.</description>
        <element name="8KHZ" internal_name="SamplingRate_8KHZ">
            <description>Sampling rate of 8000 Hz.</description>
        </element>
        <element name="16KHZ" internal_name="SamplingRate_16KHZ">
            <description>Sampling rate of 16000 Hz.</description>
        </element>
        <element name="22KHZ" internal_name="SamplingRate_22KHZ">
            <description>Sampling rate of 22050 Hz.</description>
        </element>
        <element name="44KHZ" internal_name="SamplingRate_44KHZ">
            <description>Sampling rate of 44100 Hz.</description>
        </element>
    </enum>

public enum SamplingRate {
	_8KHZ("8KHZ"),
	_16KHZ("16KHZ"),
	_22KHZ("22KHZ"),
	_44KHZ("44KHZ")
        ; ... }

For SoftButtonType enum:

    <enum name="SoftButtonType" since="2.0">
        <description>Contains information about the SoftButton capabilities.</description>
        <element name="TEXT" internal_name="SBT_TEXT"/>
        <element name="IMAGE" internal_name="SBT_IMAGE"/>
        <element name="BOTH" internal_name="SBT_BOTH"/>
    </enum>

public enum SoftButtonType {
	SBT_TEXT("TEXT"),
	SBT_IMAGE("IMAGE"),
	SBT_BOTH("BOTH")
        ; ... }

In above examples "name" attribute from XML goes to Java enum constructor parameter but "internal_name" translated to Java enum element name in different ways. Sometimes it's just by removing enum name from internal_name (SamplingRate_44KHZ -> _44KHZ). But sometimes e.g for NO_FIX (Dimension_NO_FIX -> NO_FIX). Also sometimes there is no changes at all like in last example.

Could you please advise what approach should we choose for RPC Java code generator in this case?

This behavior is found in many enum Java implementation: BitsPerSample, Dimension, HMILevel, Language, PredefinedLayout, PredefinedWindows, SamplingRate, SoftButtonType, SystemContext, TriggerSource, DisplayType, MetadataType, SystemCapabilityType.

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

[vladmu]

Dear maintainers,

Additionally to the previous question regarding the naming in enums, we have the new one (see the bottom of this long comment) regarding the inconsistency between JavaDoc comments of existing Proxy RPC classes and descriptions provided in MOBILE XML.

Let's consider the AddCommand function as example, here is the XML:

<function name="AddCommand" functionID="AddCommandID" messagetype="request" since="1.0">
    <description>
        Adds a command to the in application menu.
        Either menuParams or vrCommands must be provided.
    </description>

    <param name="cmdID" type="Integer" minvalue="0" maxvalue="2000000000" mandatory="true">
        <description>unique ID of the command to add.</description>
    </param>

    <param name="menuParams" type="MenuParams" mandatory="false">
        <description>Optional sub value containing menu parameters</description>
    </param>

    <param name="vrCommands" type="String" minsize="1" maxsize="100" maxlength="99" array="true" mandatory="false">
        <description>
            An array of strings to be used as VR synonyms for this command.
            If this array is provided, it may not be empty.
        </description>
    </param>

    <param name="cmdIcon" type="Image" mandatory="false" since="2.0">
        <description>
            Image struct determining whether static or dynamic icon.
            If omitted on supported displays, no (or the default if applicable) icon shall be displayed.
        </description>
    </param>

</function>

As you could see the description of this function is just

    Adds a command to the in application menu.
    Either menuParams or vrCommands must be provided.

Meantime in the code, the comment is following

This class will add a command to the application's Command Menu

Note: A command will be added to the end of the list of elements in the Command Menu under the following conditions:

• When a Command is added with no MenuParams value provided
• When a MenuParams value is provided with a MenuParam.position value greater than or equal to the number of menu items currently defined in the menu specified by the MenuParam.parentID value

The set of choices which the application builds using AddCommand can be a mixture of:

• Choices having only VR synonym definitions, but no MenuParams definitions
• Choices having only MenuParams definitions, but no VR synonym definitions
• Choices having both MenuParams and VR synonym definitions

HMILevel needs to be FULL, LIMITED or BACKGROUD

Parameter List
...

Response

Indicates that the corresponding request has failed or succeeded, if the response returns with a SUCCESS result code, this means a command was added to the Command Menu successfully.

Non-default Result Codes:

INVALID_ID

DUPLICATE_NAME

Then let's go deeper and consider the JavaDoc for params of this function, eg. cmdID param in XML has the description:

unique ID of the command to add.

But the code has the following for getter:

	/**
	 * <p>
	 * Returns an <i>Integer</i> object representing the Command ID that you want to add
	 * </p>
	 * 
	 * @return Integer -an integer representation a Unique Command ID
	 */

And for setter:

	/**
	 * <p>Sets an Unique Command ID that identifies the command. Is returned in an
	 * <i>{@linkplain OnCommand}</i> notification to identify the command
	 * selected by the user</p>
	 * 
	 * 
	 * @param cmdID
	 *            an integer object representing a Command ID
	 *            <p>
	 *            <b>Notes:</b> Min Value: 0; Max Value: 2000000000</p>
	 */

As you could see, there are many additional texts not provided by XML used in the class. All methods in the AddCommand and in many other classes have this.

It is clear that all those texts were added manually and from the documentation perspective that's superb to have that, but from the perspective of the code generation we need your advice:

Should we customize the generator to follow all existing texts in the current files? Or is it possible to consider that as edge cases and just generate the code with only texts we have in XML descriptions?

@theresalech @joeygrover @joeljfischer could you advise?

[joeljfischer]

This is a tough question, but I would err on the side of replacing the comments with the existing RPC spec comments.

[joeygrover]

It will be very unfortunate to lose all the additional documentation we have written that is specific to the Java methods and classes. There are plenty of tables and custom information we added that makes sense in other places in the library. Overwriting them with the subpar RPC spec definitions is a step backwards. Again, I would rather see the ability to append an RPC class for future modifications than simply have to either chose to overwrite the entire thing or skip it.

[joeljfischer]

I agree, we have done the same in the iOS classes. Would a better way forward be to move these comments into the RPC spec itself?

[joeygrover]

Some of the info I think could be moved, but some is definitely custom per platform.

[vladmu]

@joeljfischer @joeygrover I agree with your thoughts. However, moving comments into the RPC spec is out of the scope of this proposal. The ability to append an RPC class is not a good idea in a case when the generation is done based only on XML. And this ability requires us to create an additional parser for existing code and increases the complexity and maintenance. We implemented the possibility to put customization into scripts over the generation process, and this is potentially the best way because it removes the dependency from the existing code. That customization could be fully or partially moved into XML in the future. Could you advise with the correct actions in the current scope, should we leave all those comments? It is possible but requires more time to do.

[vladmu]

@theresalech @joeljfischer @joeygrover could you assist us with the question above? What is the final direction regarding the custom comments?

[theresalech]

@vladmu for this iteration of the RPC Generation scripts, we can simply use the documentation that is present in the RPC Spec.

[vladmu]

@theresalech @joeljfischer @joeygrover we have found missing parameters in 3 enums that exist in XML but don't exist in the current code:

FunctionID.RESERVED
AppInterfaceUnregisteredReason.UNSUPPORTED_HMI_RESOURCE
Result.CHAR_LIMIT_EXCEEDED

This leads to the failed result when we try to check the generated code with existing unit tests. Is that ok if the result is failed in this particular case, or do we need to fix tests and existing code to include missing parameters and their getters/setters? Or do we need to remove those parameters from the generated code to match the current state of Enum classes and make tests passed?

[bilal-alsharifi]

iOS sdl library also includes the aforementioned params. I think they should be added to the java library to align with the RPC Spec and iOS.

[joeljfischer]

@bilal-alsharifi @vladmu That's outside of the scope of this PR. It's okay that they're not in there, but document it in the PR and we will open an issue to fix it in the next release.

[joeljfischer]

@vladmu Please notify us when the PR is ready for review

[vladmu]

but document it in the PR and we will open an issue to fix it in the next release.

@joeljfischer did you mean we need to create the separate PR with the fix, or we need just to put the comment into our current PR with the generator regarding this problem?

[joeljfischer]

The second, you just need to note the problem in the current PR. Livio can create the issue and PR fix for the missing RPCs.

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

[theresalech]

@vladmu please advise when PR #1273 has been updated with the keyword rule accepted in the revisions proposal, and we will review. Thanks!

[ghost]

@theresalech PR #1273 was updated with keyword rule, please review