[SDL-0262] New vehicle data SeatOccupancy #351

ymalovanyi · 2020-11-19T12:19:15Z

Fixes #336

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 0262] New vehicle data SeatOccupancy changes

Core Tests

VehicleDataType enum contains additional value "VEHICLEDATA_SEATOCCUPANCY"
each of the properties in the struct SeatStatus can be received from HMI
each of the properties in the struct SeatOccupancy can be received from HMI
SubscribeVehicleData, UnsubscribeVehicleData, GetVehicleData request messages can be sent with seatOccupancy parameter defined as boolean value
SubscribeVehicleData, UnsubscribeVehicleData response messages contain seatOccupancy parameter defined as VehicleDataResult struct
GetVehicleData , OnVehicleData response messages contain seatOccupancy parameter defined as SeatOccupancy struct

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

Summary

Applied [SDL 0262] New vehicle data SeatOccupancy changes

Changelog

Breaking Changes

N/A

Enhancements

New structs SeatStatus, SeatOccupancy
Added required getters/setters in related RPC classes

Bug Fixes

N/A

Tasks Remaining:

N/A

CLA

I have signed the CLA

ymalovanyi · 2020-11-19T13:43:41Z

@santhanamk the PR is ready for Ford review.

santhanamk

Hi @ymalovanyi I reviewed the PR. Code looks good.

I have a few comments. Please look through it, and see if any changes need to be made.

santhanamk · 2020-11-20T14:49:46Z

tests/Test.js

+};
+
+const GENERAL_SEAT_OCCUPANCY = Test.GENERAL_SEAT_OCCUPANCY = new SeatOccupancy()
+    .setSeatsOccupied([Test.GENERAL_SEAT_STATUS])


Given that seatsOccupied and seatsBelted are arrays in the 'SeatsOccupancy.js' struct, does the GENERAL_SEAT_STATUS on line 431 need to be instantiated differently?

For example on line 128 imageTypeSupported is an array and is set up with the GENERAL_FILETYPE_LIST using the line GENERAL_IMAGEFIELD.setImageTypeSupported(GENERAL_FILETYPE_LIST);.

Hi @santhanamk
I've created GENERAL_SEAT_STATUS_LIST and JSON_SEATSTATUS_LIST arrays to make code look cleaner.

santhanamk · 2020-11-20T14:51:24Z

tests/node/rpc/messages/GetVehicleDataResponseTests.js


        // Valid Tests
        Validator.assertEquals(this.stabilityControlsStatus, testStabilityControlsStatus);
        Validator.assertEquals(Test.GENERAL_BOOLEAN, testHandsOffSteering);
        Validator.assertEquals([Test.GENERAL_WINDOW_STATUS], testWindowStatus);
        Validator.assertEquals(this.gearStatus, testGearStatus);
+        Validator.assertEquals(Test.GENERAL_SEAT_OCCUPANCY, testSeatOccupancy);


On line 80 are brackets ( [] ) needed around Test.GENERAL_SEAT_OCCUPANCY similar to line 76 for Test.GENERAL_WINDOW_STATUS?

No, because seatOccupancy is not an array (SeatOccupancy), compared to windowStatus which is indeed an array (WindowStatus[])

santhanamk · 2020-11-20T14:52:25Z

tests/node/rpc/messages/OnVehicleDataTests.js


        // Valid Tests
        Validator.assertEquals(this.stabilityControlsStatus, testStabilityControlsStatus);
        Validator.assertEquals([Test.GENERAL_WINDOW_STATUS], testWindowStatus);
        Validator.assertEquals(this.gearStatus, testGearStatus);
+        Validator.assertEquals(Test.GENERAL_SEAT_OCCUPANCY, testSeatOccupancy);


On line 78 are brackets ( [] ) needed around Test.GENERAL_SEAT_OCCUPANCY similar to line 76 for Test.GENERAL_WINDOW_STATUS?

Also not needed, same as in GetVehicleDataResponseTests.js

santhanamk

Hi @ymalovanyi . The changes you have made from the code review good. I will approve this PR. Thanks.

ymalovanyi · 2020-12-03T10:07:42Z

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

jordynmackool · 2020-12-10T20:28:21Z

Hi @ymalovanyi 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-18T17:31:54Z

@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

Successfully tested against core and sdl_hmi. Everything here looks good. Approved.

vladmu · 2021-01-14T13:21:30Z

Hi @renonick87, after merging, we have some conflicted vehicle data PRs. Could you define which will be reviewed next, then we could prepare it and fix conflicts before your review?

crokita · 2021-01-14T14:37:59Z

Hello @vladmu. I will be reviewing this PR: #302
That will be the next one tested.

I also noticed that my unit tests started failing because the associated rpc_spec PR for this feature wasn't merged in develop yet. Please check that the associated rpc_spec PR is merged in before merging the JS one!

vladmu · 2021-01-14T14:45:45Z

@crokita thank you for your reply, I'll update #302 shortly. Regarding merging, it is a good point, definitely, the related rpc_spec should be merged first.

[SDL-0262] New vehicle data SeatOccupancy

8be4eaf

santhanamk reviewed Nov 20, 2020

View reviewed changes

Add GENERAL_SEAT_STATUS_LIST and JSON_SEATSTATUS_LIST arrays in tests

edf43e8

santhanamk approved these changes Dec 1, 2020

View reviewed changes

ymalovanyi marked this pull request as ready for review December 3, 2020 10:07

renonick87 approved these changes Jan 13, 2021

View reviewed changes

renonick87 merged commit 51b40ab into smartdevicelink:develop Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDL-0262] New vehicle data SeatOccupancy #351

[SDL-0262] New vehicle data SeatOccupancy #351

ymalovanyi commented Nov 19, 2020 •

edited

Loading

ymalovanyi commented Nov 19, 2020

santhanamk left a comment

santhanamk Nov 20, 2020

ymalovanyi Nov 23, 2020

santhanamk Nov 20, 2020

ymalovanyi Nov 23, 2020 •

edited

Loading

santhanamk Nov 20, 2020

ymalovanyi Nov 23, 2020

santhanamk left a comment

ymalovanyi commented Dec 3, 2020

jordynmackool commented Dec 10, 2020 •

edited

Loading

vladmu commented Dec 18, 2020

renonick87 left a comment

vladmu commented Jan 14, 2021

crokita commented Jan 14, 2021

vladmu commented Jan 14, 2021

[SDL-0262] New vehicle data SeatOccupancy #351

[SDL-0262] New vehicle data SeatOccupancy #351

Conversation

ymalovanyi commented Nov 19, 2020 • edited Loading

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Breaking Changes

Enhancements

Bug Fixes

Tasks Remaining:

CLA

ymalovanyi commented Nov 19, 2020

santhanamk left a comment

Choose a reason for hiding this comment

santhanamk Nov 20, 2020

Choose a reason for hiding this comment

ymalovanyi Nov 23, 2020

Choose a reason for hiding this comment

santhanamk Nov 20, 2020

Choose a reason for hiding this comment

ymalovanyi Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

santhanamk Nov 20, 2020

Choose a reason for hiding this comment

ymalovanyi Nov 23, 2020

Choose a reason for hiding this comment

santhanamk left a comment

Choose a reason for hiding this comment

ymalovanyi commented Dec 3, 2020

jordynmackool commented Dec 10, 2020 • edited Loading

vladmu commented Dec 18, 2020

renonick87 left a comment

Choose a reason for hiding this comment

vladmu commented Jan 14, 2021

crokita commented Jan 14, 2021

vladmu commented Jan 14, 2021

ymalovanyi commented Nov 19, 2020 •

edited

Loading

ymalovanyi Nov 23, 2020 •

edited

Loading

jordynmackool commented Dec 10, 2020 •

edited

Loading