[SDL 0269] New vehicle data ClimateData #350

vladmu · 2020-11-18T10:37:20Z

Fixes #337

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 0269] New vehicle data ClimateData changes

Core Tests

each of the properties in the struct of ClimateData can be received from the HMI
subscribing and unsubscribing to the HMI's ClimateData works
the application can receive updates via OnVehicleData RPCs.

Core version / branch / commit hash / module tested against: smartdevicelink/sdl_core#3586
HMI name / version / branch / commit hash / module tested against: smartdevicelink/sdl_hmi#468

Summary

Applied [SDL 0269] New vehicle data ClimateData changes

Changelog

Breaking Changes

N/A

Enhancements

New struct ClimateData
Added required getters/setters in related RPC classes

Bug Fixes

N/A

Tasks Remaining:

N/A

CLA

I have signed the CLA

vladmu · 2020-11-18T10:38:42Z

@santhanamk the PR is ready for Ford review.

santhanamk

Hi @vladmu . Code looks good. I have given some feedback. Please see if you need to make those changes.

Some changes I suggested are optional, and are out of scope of this PR. You don't have to make those changes if you dont want to. They are very minor changes. I have marked those as "Optional".

santhanamk · 2020-11-20T02:07:23Z

lib/js/src/rpc/enums/VehicleDataType.js

+        return VehicleDataType._MAP.VEHICLEDATA_CLIMATEDATA;
+    }
+
    /**


Are you able to apply this history tag to VEHICLEDATA_EXTERNTEMP?

<history> <element name="VEHICLEDATA_EXTERNTEMP" since="2.0" until="X.x" /> </history>

I think you may just need to add * @since SmartDeviceLink 2.0.0 * @deprecated in SmartDeviceLink 7.1.0

around line 110 above static get VEHICLEDATA_EXTERNTEMP ().

It is possible, but I'm not sure we need to add something here manually. The code you see is a result of the code generator tool.

santhanamk · 2020-11-20T02:10:49Z

tests/Test.js

 const Grid = SDL.rpc.structs.Grid;
 const WindowStatus = SDL.rpc.structs.WindowStatus;
+const Temperature = SDL.rpc.structs.Temperature;



Can you setup the ClimateData struct?

on line 28, you can add const ClimateData = SDL.rpc.structs.ClimateData;

Fixed in d1e6617

santhanamk · 2020-11-20T02:12:07Z

tests/Test.js

+    .setValueParam(Test.GENERAL_NUMBER);
+
+const JSON_TEMPERATURE = Test.JSON_TEMPERATURE = GENERAL_TEMPERATURE.getParameters();
+


You can instantiate the ClimateData objects (GENERAL_CLIMATE_DATA and JSON_CLIMATE_DATA) around line 429. It would be something similar to lines 402-406 for the Window State setup (GENERAL_WINDOW_STATE and JSON_WINDOW_STATE)

Fixed in d1e6617

santhanamk · 2020-11-20T02:17:02Z

tests/node/rpc/enums/VehicleDataTypeTests.js

        Validator.assertNotNullUndefined(enumVehicledataFuellevelState, 'VEHICLEDATA_FUELLEVEL_STATE returned null.');
        Validator.assertNotNullUndefined(enumVehicledataFuelconsumption, 'VEHICLEDATA_FUELCONSUMPTION returned null.');
-        Validator.assertNotNullUndefined(enumVehicledataExterntemp, 'VEHICLEDATA_EXTERNTEMP returned null.');
+        Validator.assertNotNullUndefined(enumVehicledataClimatedata, 'VEHICLEDATA_FUELCONSUMPTION returned null.');


line 90 should be Validator.assertNotNullUndefined(enumVehicledataClimatedata, 'VEHICLEDATA_CLIMATEDATA returned null.');

Fixed in d1e6617

santhanamk · 2020-11-20T02:17:38Z

tests/node/rpc/enums/VehicleDataTypeTests.js

        Validator.assertNotNullUndefined(enumVehicledataFuelconsumption, 'VEHICLEDATA_FUELCONSUMPTION returned null.');
-        Validator.assertNotNullUndefined(enumVehicledataExterntemp, 'VEHICLEDATA_EXTERNTEMP returned null.');
+        Validator.assertNotNullUndefined(enumVehicledataClimatedata, 'VEHICLEDATA_FUELCONSUMPTION returned null.');
+        Validator.assertNotNullUndefined(enumVehicledataExterntemp, 'VEHICLEDATA_CLIMATEDATA returned null.');


line 91 should be Validator.assertNotNullUndefined(enumVehicledataExterntemp, 'VEHICLEDATA_EXTERNTEMP returned null.');

Fixed in d1e6617

santhanamk · 2020-11-20T02:28:41Z

tests/node/rpc/messages/SubscribeVehicleDataResponseTests.js


        // Valid Tests
        Validator.validateVehicleDataResult(this.stabilityControlsStatus, testStabilityControlsStatus);
        Validator.validateVehicleDataResult(this.vehicleDataResult, testHandsOffSteering);


Optional: On line 84 you can change vehicleDataResult to handsOffSteering.

Based on previous experience, it is better to avoid changes not related to PR. Will add if PMs request this in the review.

santhanamk · 2020-11-20T02:33:05Z

tests/node/rpc/messages/UnsubscribeVehicleDataResponseTests.js

+// structs
 const VehicleDataResult = SDL.rpc.structs.VehicleDataResult;

 const BaseRpcTests = require('./BaseRpcTests');


Optional: On line 24 you could change this.vehicleDataResult to this.handsOffSteering.

Based on previous experience, it is better to avoid changes not related to PR. Will add if PMs request this in the review.

santhanamk · 2020-11-20T02:33:36Z

tests/node/rpc/messages/UnsubscribeVehicleDataResponseTests.js

 const FunctionID = SDL.rpc.enums.FunctionID;
 const MessageType = SDL.rpc.enums.MessageType;
 const VehicleDataType = SDL.rpc.enums.VehicleDataType;
+


Optional: On line 26 you could change JSON_VEHICLEDATARESULT to JSON_HANDSOFFSTEERING

Based on previous experience, it is better to avoid changes not related to PR. Will add if PMs request this in the review.

santhanamk · 2020-11-20T02:34:18Z

tests/node/rpc/messages/UnsubscribeVehicleDataResponseTests.js

        this.createMessage = function () {
            return new UnsubscribeVehicleDataResponse()
                .setStabilityControlsStatus(this.stabilityControlsStatus)
                .setHandsOffSteering(this.vehicleDataResult)


Optional: On line 45 you can change vehicleDataResult to handsOffSteering.

Based on previous experience, it is better to avoid changes not related to PR. Will add if PMs request this in the review.

santhanamk · 2020-11-20T02:34:46Z

tests/node/rpc/messages/UnsubscribeVehicleDataResponseTests.js


        // Valid Tests
        Validator.validateVehicleDataResult(this.stabilityControlsStatus, testStabilityControlsStatus);
        Validator.validateVehicleDataResult(this.vehicleDataResult, testHandsOffSteering);


Optional: On line 84 you can change vehicleDataResult to handsOffSteering.

Based on previous experience, it is better to avoid changes not related to PR. Will add if PMs request this in the review.

vladmu · 2020-11-25T11:51:28Z

@santhanamk I processed all your comments. Please re-review.

santhanamk

Hi @vladmu . I reviewed the code review changes, and the code looks good. Thanks.

vladmu · 2020-12-01T15:20:14Z

@santhanamk, thank you for the approval! @crokita, Ford approved this PR, please review.

jordynmackool · 2020-12-10T20:33:36Z

Hi @vladmu and @santhanamk, since the core PR for this feature is not complete, it seems that this PR would have not been tested fully. To ensure the review process goes smoothly:

Please avoid removing sections from the PR template as all the information requested is needed.
Please provide the Core branch/PR that this was tested against.

Once this PR is fully tested, tag me and Livio will review. Thank you!

vladmu · 2020-12-11T10:42:05Z

Hi @jordynmackool ,

Hi @vladmu and @santhanamk, since the core PR for this feature is not complete, it seems that this PR would have not been tested fully.

Please provide the Core branch/PR that this was tested against.

I would be thankful if you could describe what you mean under full testing here in the border of the current proposal? Previous similar work did not require any additional testing except unit tests. Please see #258
Only unit tests coverage was included in the testing plan, as defined in the description, therefore no Core branch/PR provided. For processing your request correctly we appreciate your help regarding the definition of what kind of tests with Core you're expecting here.

Please avoid removing sections from the PR template as all the information requested is needed.

It seems not all sections could be relevant to code changes provided, should we use N/A to mark those sections instead of removing?

jordynmackool · 2020-12-11T15:31:03Z

Hi @vladmu,
All previous PRs should have not been merged unless they were tested against Core or the HMI (if applicable). Testing the PR branch against Core ensures that the feature as a whole works, not just the PR itself.

If an item in the template is not applicable, please mark it as N/A so we know that it was considered. We just updated the PR template to align with the other libraries so we are all on the same page in the future.

vladmu · 2020-12-11T15:39:05Z

Hi @vladmu,
All previous PRs should have not been merged unless they were tested against Core or the HMI (if applicable). Testing the PR branch against Core ensures that the feature as a whole work, not just the PR itself.

Thank you @jordynmackool ,

Do you have the instructions on how to test RPC changes in mobile libraries with Core? Should we develop additional applications for this? What exactly you mean under "testing RPC changes against Core"? What result is acceptable by you as "fully tested RPC changes" except unit tests? Do you have "a definition of done" for such work?

crokita · 2020-12-11T17:44:49Z

@vladmu Hello. I can provide some clarity on what would be expected for a PR like this.

Since this PR introduces a new set of vehicle data, I would expect testing against an HMI which supports this new data, and checking that each of the properties in the struct of ClimateData can be received from the HMI. I would also expect a check that subscribing and unsubscribing to the HMI's ClimateData works and that the application can receive updates via OnVehicleData RPCs.

vladmu · 2020-12-11T20:35:19Z

@vladmu Hello. I can provide some clarity on what would be expected for a PR like this.

Since this PR introduces a new set of vehicle data, I would expect testing against an HMI which supports this new data, and checking that each of the properties in the struct of ClimateData can be received from the HMI. I would also expect a check that subscribing and unsubscribing to the HMI's ClimateData works and that the application can receive updates via OnVehicleData RPCs.

Hi @crokita ,

Thank you for the explanation, but as far as we understand, that's impossible without creating at least minimal testing application which could provide described functions and allow to get and analyze the result. Do you have tools for such testing without creating the additional app? Or creating testing application for each RPC PRs is expected and is a part of the proposal implementation? Then we will need to extent timelines and effort to fit your expectations.

crokita · 2020-12-14T19:50:28Z

@vladmu Ah. With regards to testing, this current sdl_hmi PR and core PR would be of interest: smartdevicelink/sdl_hmi#468, and smartdevicelink/sdl_core#3586

We tend to rely on other projects for their implementations to be ready before we're able to do testing with feature PRs.

…ew_vehicle_data_climateData

vladmu · 2020-12-18T17:31:16Z

@jordynmackool @crokita the description was fixed to include all sections from the template and the code was tested against the Core and HMI (corresponded links also included in the description). Please review.

renonick87 · 2021-01-26T16:09:02Z

@vladmu Please run the generator again with the latest changes to the RPC spec PR. Please also merge in the develop branch and resolve any merge conflicts.

…ew_vehicle_data_climateData # Conflicts: # tests/Test.js # tests/node/rpc/messages/GetVehicleDataResponseTests.js # tests/node/rpc/messages/GetVehicleDataTests.js # tests/node/rpc/messages/OnVehicleDataTests.js # tests/node/rpc/messages/SubscribeVehicleDataResponseTests.js # tests/node/rpc/messages/SubscribeVehicleDataTests.js # tests/node/rpc/messages/UnsubscribeVehicleDataResponseTests.js # tests/node/rpc/messages/UnsubscribeVehicleDataTests.js

vladmu · 2021-01-26T18:49:29Z

@renonick87 done, please review.

renonick87 · 2021-01-26T21:15:40Z

tests/node/rpc/messages/UnsubscribeVehicleDataResponseTests.js

        const JSON_WINDOWSTATUS = this.windowStatus.getParameters();

+        this.climateData = new VehicleDataResult()
+          .setDataType(VehicleDataType.VEHICLEDATA_CLIMATEDATA);


The indentation for line 43 is incorrect. It has an indentation of 10 spaces but it should be 12 spaces.

renonick87 · 2021-01-26T21:17:18Z

tests/node/rpc/messages/SubscribeVehicleDataResponseTests.js

        const JSON_WINDOWSTATUS = this.windowStatus.getParameters();

+        this.climateData = new VehicleDataResult()
+          .setDataType(VehicleDataType.VEHICLEDATA_CLIMATEDATA);


The indentation for line 43 is incorrect. It has an indentation of 10 spaces but it should be 12 spaces.

renonick87

@vladmu This looks good for the most part. See the comments regarding indentation. In the future, please use the command npm run lint to ensure all contributions pass lint validation.

vladmu · 2021-01-26T21:40:19Z

@renonick87 thank you for the review. We use lint if possible, but it is less useful for now because it shows 9 problems (5 errors, 4 warnings) in parts not related to current code changes. Therefore those 2 indentations were missed over the process of merging the conflict. Fixed, please review.

renonick87 · 2021-01-29T19:27:48Z

@vladmu We'll be looking to fix those linter problems that you're seeing in a future PR so hopefully that will no longer be an issue. Everything else looks good here. If you can pull in develop to resolve the merge conflicts then I can approve this PR.

…ew_vehicle_data_climateData

vladmu · 2021-02-01T08:24:57Z

@renonick87 we've updated the branch with the latest develop, please approve.

renonick87

Marking this PR as approved. Will hold off on merging it until its respective rpc_spec PR, smartdevicelink/rpc_spec#299, is merged.

renonick87 · 2021-02-10T15:49:35Z

@vladmu Please fix the merge conflicts on this PR and then I can merge it.

…ew_vehicle_data_climateData # Conflicts: # tests/Test.js

BogMW · 2021-02-10T16:18:30Z

@renonick87 all conflicts were fixed.

feature/sdl_0269_new_vehicle_data_climateData

a74af69

santhanamk reviewed Nov 20, 2020

View reviewed changes

vladmu added 2 commits November 25, 2020 13:38

Review fixes

d1e6617

Review fixes

3881d93

santhanamk approved these changes Nov 30, 2020

View reviewed changes

vladmu marked this pull request as ready for review December 1, 2020 15:15

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

9f8ba10

…ew_vehicle_data_climateData

vladmu added 2 commits January 26, 2021 18:53

regenerated RPCs

f3c57cb

renonick87 reviewed Jan 26, 2021

View reviewed changes

review fixes

f71de8c

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

29ece49

…ew_vehicle_data_climateData

renonick87 approved these changes Feb 2, 2021

View reviewed changes

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

e8316c2

…ew_vehicle_data_climateData # Conflicts: # tests/Test.js

renonick87 merged commit 76f054e into smartdevicelink:develop Feb 10, 2021

		.setValueParam(Test.GENERAL_NUMBER);

		const JSON_TEMPERATURE = Test.JSON_TEMPERATURE = GENERAL_TEMPERATURE.getParameters();

[SDL 0269] New vehicle data ClimateData #350

[SDL 0269] New vehicle data ClimateData #350

Uh oh!

Conversation

vladmu commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

Tasks Remaining:

CLA

Uh oh!

vladmu commented Nov 18, 2020

Uh oh!

santhanamk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladmu commented Nov 25, 2020

Uh oh!

santhanamk left a comment

Choose a reason for hiding this comment

Uh oh!

vladmu commented Dec 1, 2020

Uh oh!

jordynmackool commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladmu commented Dec 11, 2020

Uh oh!

jordynmackool commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vladmu commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

vladmu commented Nov 18, 2020 •

edited

Loading

jordynmackool commented Dec 10, 2020 •

edited

Loading

jordynmackool commented Dec 11, 2020 •

edited

Loading

vladmu commented Dec 11, 2020 •

edited

Loading

vladmu commented Jan 26, 2021 •

edited

Loading