[SDL-0293] Enable OEM exclusive apps support #374

ymalovanyi · 2021-01-14T12:53:45Z

Fixes #367

Risk

This PR makes minor API changes.

Testing Plan

I have verified that I have not introduced new warnings in this PR
I have run the unit tests with this PR
I have tested this PR against Core and verified behavior

Unit Tests

Added unit tests cover [SDL-0293] Enable OEM exclusive apps support changes

Core Tests

Check the Vehicle Details arrived through StartSessionACK packet and processed correctly
Check the Vehicle Details exist in RAI response and processed correctly
Check that Vehicle Details in RAI response is used when StartSessionACK has no Vehicle Details
Check that application continue work as usual if no onVehicleTypeReceived callback set in SdlManagerListener

Core version / branch / commit hash / module tested against: LuxoftSDL/sdl_core#sdl_0293_enable_oem_exclusive_apps_support
HMI name / version / branch / commit hash / module tested against: LuxoftSDL/sdl_hmi#sdl_0293_enable_oem_exclusive_apps_support

Summary

Applied [SDL-0293] Enable OEM exclusive apps support changes

Changelog

Breaking Changes

N/A

Enhancements

Added onVehicleTypeReceived, setOnVehicleTypeReceved methods to propagate Vehicle Type info to the application layer and check Vehicle Type on _SdlProtocolBase.js and _LifecycleManager.js levels
Added required getters/setters in related RPC classes

Bug Fixes

N/A

Tasks Remaining:

N/A

CLA

I have signed the CLA

a-prykho · 2021-01-14T17:16:55Z

@santhanamk could you please start Ford review? Thanks

santhanamk

@ymalovanyi I have reviewed the PR, and have left a few comments/suggestions. Please take a look whenever you get the chance.

santhanamk · 2021-01-19T17:01:47Z

lib/js/src/protocol/enums/_ControlFrameTags.js

@@ -72,6 +72,12 @@ _ControlFrameTags.RPC = Object.freeze({
        VIDEO_SERVICE_TRANSPORTS: 'videoServiceTransports',
        /** Auth token to be used for log in into services **/
        AUTH_TOKEN: 'authToken',
+        VEHICLE_MAKE: 'make',
+        VEHICLE_MODEL: 'model',
+        VEHICLE_MODEL_YEAR: 'modelYear',


@ymalovanyi On line 77, does the tag name need to be model year like it is defined in the proposal or is modelYear okay?

It seems this is just a typo in the proposal, on testing alongside the core we received modelYear thus included it in camelcase instead of space-separated.

santhanamk · 2021-01-19T17:07:57Z

lib/js/src/protocol/_SdlProtocolBase.js

@@ -406,6 +438,23 @@ class _SdlProtocolBase {
        const version = sdlPacket.getVersion();
        const serviceType = sdlPacket.getServiceType();
        if (version >= 5) {


@ymalovanyi On line 440, does this need to be >= 6 instead of >= 5? In the proposal the example method onPacketRead has it as > = 6.

Our initial implementation was exactly with >= 6 according to the proposal, but testing alongside the core we never met that condition, it seems the protocol version is not increased with this proposal. Therefore we rolled back to 5.

santhanamk · 2021-01-19T17:22:19Z

lib/js/src/protocol/_SdlProtocolListener.js

+     * @param {String} systemHardwareVersion - hardware version of the system.
+     * @returns {Boolean} Return true if this session should continue, false if the session should end
+     */
+    onVehicleTypeReceived (vehicleType, systemSoftwareVersion, systemHardwareVersion) {


@ymalovanyi In the proposal the method onVehicleTypeReceived is defined like: onVehicleTypeReceived (sdlManager, vehicleType) {}.... Does your definition starting on line 210 need to match with what the proposal has?

The sdlManager parameter is not required in the context of the current library implementation, we assume this was added in the proposal by mistake. systemSoftwareVersion and systemHardwareVersion parameters now under discussion here smartdevicelink/sdl_evolution#1108 this PR already includes changes of that revision.

santhanamk · 2021-01-19T17:24:21Z

lib/js/src/rpc/messages/RegisterAppInterfaceResponse.js

+     * Set the SystemHardwareVersion
+     * @since SmartDeviceLink 7.1.0
+     * @param {String} version - The hardware version of the system - The desired SystemHardwareVersion.
+     * {'string_min_length': 1, 'string_max_length': 500}


@ymalovanyi Are these defined somewhere? For example, the string_min_length 1 string_max_length 500? It would be nice to cross verify.

This is autogenerated code, no RPC classes in the library for now those cross-verify such parameters.

santhanamk · 2021-01-19T18:48:30Z

lib/js/src/manager/lifecycle/_LifecycleManager.js

@@ -484,6 +510,17 @@ class _LifecycleManager {
            this._cleanProxy();
        }

+        if (this._rpcSpecVersion.isNewerThan(new Version(7, 0, 0)) === 1) {


@ymalovanyi Do the unit tests in LifecycleManagerTests.js need to be updated?

@santhanamk Added tests for new methods.

santhanamk · 2021-01-19T18:50:17Z

lib/js/src/session/_SdlSession.js

+     * @param {String} systemHardwareVersion - hardware version of the system.
+     * @returns {Boolean} Return true if this session should continue, false if the session should end
+     */
+    onVehicleTypeReceived (vehicleType, systemSoftwareVersion, systemHardwareVersion) {


@ymalovanyi In the proposal this method is defined as onVehicleTypeReceived (sdlManager, vehicleType) {}. Does your method definition need to change to match with the proposal?

#374 (comment)

santhanamk

Thank you @vladmu for the clarifications. I have approved this PR.

…xclusive_app_support

…peReceived and adjust the code to match the current 0293 proposal state * added didReceiveVehicleType flag, to avoid double firing onVehicleTypeReceived in RAI response

vladmu · 2021-01-22T07:58:22Z

@santhanamk I apologize, I should ask you to re-review the PR. I have made some changes to reflect the current state of the proposal instead of unapproved revision.

santhanamk

@vladmu @ymalovanyi I have reviewed the PR. I have one suggestion. Please take a look whenever you get a chance.

santhanamk · 2021-01-22T19:40:57Z

lib/js/src/protocol/_SdlProtocolBase.js

@@ -38,14 +38,14 @@ import { _FrameType } from './enums/_FrameType.js';
 import { _MessageFrameAssembler } from './_MessageFrameAssembler.js';
 import { _SdlPacket } from './_SdlPacket.js';
 import { _ControlFrameTags } from './enums/_ControlFrameTags.js';
-import { _BitConverter } from './../util/_BitConverter.js';
+import { _BitConverter } from '../util/_BitConverter.js';


@ymalovanyi @vladmu Do you need unit tests for this? For example, sdl java suite has SdlProtocolTests

@santhanamk we don't need the test for this particular line as this is just a shortenest path identical to previous but aligned with the style of other imports.

@vladmu I meant do you need unit tests overall for the changes made to this file?

@santhanamk it is hard to define expectations of such tests in the order of the library and this class as we don't have any tests of previous functionality covered here. We appreciate any help in this case.

@vladmu Ok no problem. It was an optional request, as these were not existing before.

santhanamk

@ymalovanyi @vladmu I have approved this PR.

santhanamk · 2021-01-22T22:32:09Z

Hi @jordynmackool This PR is ready for Livio review.

renonick87 · 2021-01-26T22:32:36Z

lib/js/src/protocol/_SdlProtocolBase.js

+        // TODO: awaiting 0293 revisions to process those fields
+        const systemHardwareVersion = sdlPacket.getTag(_ControlFrameTags.RPC.StartServiceACK.VEHICLE_SYSTEM_HARDWARE_VERSION);
+        const systemSoftwareVersion = sdlPacket.getTag(_ControlFrameTags.RPC.StartServiceACK.VEHICLE_SYSTEM_SOFTWARE_VERSION);


This method seems unfinished since these values are never used.

@renonick87 no, the method is finished and reflects the current state of the proposal. Without revisions, we receive those fields here but never use or process them. The TODO was added to reflect that the processing needs to be added after accepting the revisions.

renonick87 · 2021-01-26T22:37:27Z

lib/js/src/protocol/_SdlProtocolBase.js

+    /**
+     * Gets the Vehicle details received in StartService ACK protocol message
+     * @returns {VehicleType|null} - A RPC VehicleType struct received from the packet if exists null otherwise.
+     */
+    _getVehicleType(sdlPacket) {


The documentation here is missing the @param tag for the sdlPacket parameter. There also needs to be a space before the function parentheses on line 277. Use the command npm run lint to confirm.

renonick87 · 2021-01-26T22:39:04Z

lib/js/src/protocol/_SdlProtocolBase.js

+              vehicleType
+              && this._sdlProtocolListener !== null
+              && typeof this._sdlProtocolListener.onVehicleTypeReceived === 'function'


The indentation for lines 441-443 is incorrect. There is an indentation of 14 spaces but it should be 16. Use the command npm run lint to confirm.

renonick87 · 2021-01-26T22:42:47Z

lib/js/src/session/_SdlSession.js

+     * A way to determine if VehicleType already received
+     * @returns {Boolean} Return true if received
+     */
+    didReceiveVehicleType() {


There needs to be a space before the function parentheses on line 177. Use the command npm run lint to confirm.

renonick87

@ymalovanyi I've done an initial review of this PR but will hold off on a more thorough review until after the core and HMI PRs are ready to be tested against. See my comments for requested changes and please run npm run lint to ensure that all contributions pass lint validation.

jordynmackool · 2021-01-28T21:17:11Z

@ymalovanyi the proposal markdown file has been updated per the revisions included in the accepted with revisions review issue: SDL 0293 Revisions - Enable OEM exclusive apps support.

Please make the needed updates to this PR and then tag @renonick87 to re-review when ready. Thanks!

vladmu · 2021-01-29T18:42:46Z

@renonick87 all required adjustments were made, the PR is ready for review.

renonick87 · 2021-02-10T15:41:06Z

@ymalovanyi, the Java Suite team has written a new PR, smartdevicelink/sdl_java_suite#1618, for this proposal. Please update this PR or open a new one to match.

vladmu · 2021-02-10T15:43:25Z

@renonick87 I did review the new Java implementation and this JS seems to look the same at first glance. Did you compare on your own what is missed here?

jordynmackool · 2021-02-10T18:59:00Z

@ymalovanyi @vladmu The Java Suite PR 1681 has been approved on our side and tested with Core for this feature. We have not compared the differences between that PR and this PR. To ensure app library alignment, please review the approved Java Suite library PR and make the necessary updates to this PR to align. Once all the needed updates are complete, please tag @renonick87 for a re-review. Thanks!

renonick87 · 2021-02-10T21:22:10Z

@vladmu The PRs does seem similar at first glance but there are differences in the exact implementation. To name a few differences: all logic in the ProtocolBase has been moved, an existing listener has been updated in the SdlSessionListener, and the protocol version is updated to 5.4. There are likely other differences that I didn't cover so please review the Java PR and then update this PR to ensure that it matches the Java implementation.

…xclusive_app_support

vladmu · 2021-02-12T08:33:47Z

@renonick87 I have aligned the code to smartdevicelink/sdl_java_suite#1618 PR and tested this against the core and hmi. Please review.

renonick87

@vladmu See my review comments for some requested revisions.

renonick87 · 2021-02-16T22:24:01Z

lib/js/src/protocol/_SdlProtocolBase.js

@@ -216,7 +216,7 @@ class _SdlProtocolBase {
     */
    _setVersion (version) {
        if (version > 5) {
-            this._protocolVersion = new Version('5.1.0'); // protect for future, proxy only supports v5 or lower
+            this._protocolVersion = new Version('5.4.0'); // protect for future, proxy only supports v5 or lower


This does not need to be changed. This method is for setting the major version based on the packet header. The more accurate protocol version is set in the _handleStartServiceACK method based on the packet payload.

renonick87 · 2021-02-16T22:24:07Z

lib/js/src/manager/lifecycle/_LifecycleManager.js

@@ -484,6 +518,16 @@ class _LifecycleManager {
            this._cleanProxy();
        }

+        if (!this._didCheckSystemInfo) {
+            this._didCheckSystemInfo = true;
+            const systemInfo = new SystemInfo(registerAppInterfaceResponse.getVehicleType(), registerAppInterfaceResponse.getSystemSoftwareVersion()) ;


There should be a check before creating the SystemInfo object that either vehicleType or systemSoftwareVersion exist.

if (vehicleType || systemSoftwareVersion)

vladmu · 2021-02-17T07:14:16Z

@renonick87 I've processed all your comments. Please review.

0293 oem exclusive app support

0496013

vladmu mentioned this pull request Jan 14, 2021

[Accepted with Revisions] SDL 0293 Revisions - Enable OEM exclusive apps support smartdevicelink/sdl_evolution#1108

Closed

santhanamk reviewed Jan 19, 2021

View reviewed changes

santhanamk approved these changes Jan 21, 2021

View reviewed changes

vladmu added 3 commits January 22, 2021 07:35

Merge remote-tracking branch 'origin/develop' into feature/0293_oem_e…

c0f1b38

…xclusive_app_support

* reverted system[Hardware|Software]Version parameters in onVehicleTy…

0497c80

…peReceived and adjust the code to match the current 0293 proposal state * added didReceiveVehicleType flag, to avoid double firing onVehicleTypeReceived in RAI response

minor fixes

a2d7ab1

Update tests for LifecycleManagerTests.js

af981f8

santhanamk reviewed Jan 22, 2021

View reviewed changes

santhanamk approved these changes Jan 22, 2021

View reviewed changes

ymalovanyi marked this pull request as ready for review January 25, 2021 09:56

renonick87 reviewed Jan 26, 2021

View reviewed changes

Fix linter warnings

8f4069d

BogMW and others added 2 commits January 29, 2021 20:01

[SDL-0293] New revision adjustments

f855ca4

[SDL-0293] - minor fix

76c5350

vladmu added 2 commits February 10, 2021 23:36

Merge remote-tracking branch 'origin/develop' into feature/0293_oem_e…

cccb753

…xclusive_app_support

Alignment with Java suite implementation

8317fb6

vladmu added 3 commits February 12, 2021 09:01

Merge remote-tracking branch 'origin/develop' into feature/0293_oem_e…

59ac61a

…xclusive_app_support

Alignment with Java suite implementation

b874414

Alignment with Java suite implementation - fixes

a0a9ed0

renonick87 requested changes Feb 16, 2021

View reviewed changes

Review changes

66edef5

renonick87 approved these changes Feb 17, 2021

View reviewed changes

renonick87 merged commit 2c9f9e2 into smartdevicelink:develop Mar 2, 2021

[SDL-0293] Enable OEM exclusive apps support #374

[SDL-0293] Enable OEM exclusive apps support #374

Conversation

ymalovanyi commented Jan 14, 2021 • edited Loading

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

Tasks Remaining:

CLA

a-prykho commented Jan 14, 2021

santhanamk 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

Choose a reason for hiding this comment

santhanamk left a comment

Choose a reason for hiding this comment

vladmu commented Jan 22, 2021

santhanamk 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

santhanamk left a comment

Choose a reason for hiding this comment

santhanamk commented Jan 22, 2021

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

renonick87 left a comment

Choose a reason for hiding this comment

jordynmackool commented Jan 28, 2021

vladmu commented Jan 29, 2021

renonick87 commented Feb 10, 2021

vladmu commented Feb 10, 2021 • edited Loading

jordynmackool commented Feb 10, 2021 • edited Loading

renonick87 commented Feb 10, 2021

vladmu commented Feb 12, 2021

renonick87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladmu commented Feb 17, 2021

ymalovanyi commented Jan 14, 2021 •

edited

Loading

vladmu commented Feb 10, 2021 •

edited

Loading

jordynmackool commented Feb 10, 2021 •

edited

Loading