Skip to content

Update remote control radio parameter #93

Merged
jacobkeeler merged 8 commits intodevelopfrom
feature/rc_radio_parameter_update
Aug 28, 2018
Merged

Update remote control radio parameter #93
jacobkeeler merged 8 commits intodevelopfrom
feature/rc_radio_parameter_update

Conversation

@ghost
Copy link

@ghost ghost commented Jun 15, 2018

Technical task: smartdevicelink/sdl_core#2162
Add new parameter to RadioControlData:

  • hdRadioEnable
    Update max value from 3 to 7 for the next parameters:
  • availableHDs
  • hdChannel
    Add new radio control capabilities:
  • hdRadioenableAvailable
  • siriusxmRadioAvailable

false
],

hdChannelAvailableCurrent:[

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it is an array?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yang1070 This is array, because we can access the array to index which that the matches with available channel. And that's why we can to take off channel on the radio view. If it was a structure, we doesn't can to do that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ValeriiMalkov thank you

Copy link

@yang1070 yang1070 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There need a change in the case that when hdRadioEnabled changes from false to true, the OnInteriorVehicleData notifications shall include all parameters related to HD radio, not just the hdRadioEnabled parameter only. Similarly, for the response of SetInteriorVehicleData that turns on the hdRadio, all HD related parameters shall be included.

@yang1070
Copy link

Another requirement is that the XM radio channel number shall be the same as in the frequency integer. When click a XM radio station name in HMI, the frequency integer shall show the XM channel number.
When set a XM with channel number, HMI shall select the corresponding station number.

@ghost
Copy link
Author

ghost commented Jun 15, 2018

@yang1070 You can see fix hdRadioEnabled with related parameters at the commit 17dc458

@yang1070
Copy link

@ValeriiMalkov I haven't see any update for the XM radio channel number update so far.

@LitvinenkoIra
Copy link
Contributor

@yang1070 Sorry, updates for XM radio channel number are still in progress

@BSolonenko
Copy link

@yang1070 In commit a199fb7, I made the necessary changes to the XM radio.
Also, in b4ad697 I added the file with RC capabilities.

@BSolonenko BSolonenko force-pushed the feature/rc_radio_parameter_update branch 7 times, most recently from 490daba to cb828c0 Compare June 21, 2018 11:01
}
break;
default:
properties.push('signalStrength');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these three properties shall be in AM and FM too

@BSolonenko BSolonenko force-pushed the feature/rc_radio_parameter_update branch 2 times, most recently from 12fa09d to 2d71ea6 Compare June 22, 2018 16:13
Copy link

@yang1070 yang1070 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me.

@yang1070
Copy link

@theresalech Ford reviewed, tested and approves this change.

@BSolonenko BSolonenko force-pushed the feature/rc_radio_parameter_update branch 2 times, most recently from e0d4cd1 to c80210a Compare June 23, 2018 14:58
@jacobkeeler
Copy link
Contributor

@ValeriiMalkov please fix merge conflicts

@AKalinich-Luxoft AKalinich-Luxoft force-pushed the feature/rc_radio_parameter_update branch 2 times, most recently from e2f2180 to 6c6514a Compare August 20, 2018 17:15
@AKalinich-Luxoft AKalinich-Luxoft force-pushed the feature/rc_radio_parameter_update branch 3 times, most recently from cdd76ac to 33ca053 Compare August 20, 2018 18:24
@AKalinich-Luxoft
Copy link
Contributor

@jacobkeeler all merge conflicts have been resolved

@ghost ghost force-pushed the feature/rc_radio_parameter_update branch from ac8cd93 to 984c991 Compare August 27, 2018 13:54
@ghost
Copy link
Author

ghost commented Aug 27, 2018

@jacobkeeler, PR was rebased to develop branch

longPressAvailable: false,
upDownAvailable: false
}],
audioControlCapabilities: [{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

audioControlCapabilities already exists in this file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobkeeler This issue was resolved after rebase

frontVerticalPositionAvailable: true,
headSupportHorizontalPositionAvailable: true
}],
buttonCapabilities: [{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buttonCapabilities already exists in this file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobkeeler This issue was resolved after rebase

index.html Outdated
src="app/view/navigationApp/baseNavigationView.js"></script>
<script type="text/javascript" src="app/view/navigationAppView.js"></script>

<!-- SDL Capabilities -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobkeeler This issue was resolved after rebase

upDownAvailable: false
}],
climateControlCapabilities: [{
moduleName: 'Climate Control Module',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moduleName already exists for this capability object

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobkeeler This issue was resolved after rebase

Copy link
Contributor

@jacobkeeler jacobkeeler Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the issues were fixed, but this wasn't:

moduleName: 'primary_climate',

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacobkeeler we leave rc_capabilities.js version from develop branch and add two new parameters. All other changes were reverted. See 9de70a2

@GetmanetsIrina
Copy link

@jacobkeeler, HMI is tested after rebase, works according to proposal.

@ghost ghost force-pushed the feature/rc_radio_parameter_update branch 3 times, most recently from f076885 to b90f1a3 Compare August 27, 2018 16:10
Valerii and others added 3 commits August 27, 2018 19:24
Add new parameter to RadioControlData:
- `hdRadioEnable`
Update max value from 3 to 7 for the next parameters:
- `availableHDs`
- `hdChannel`
Add new radio control capabilities:
- `hdRadioenableAvailable`
- `siriusxmRadioAvailable`
Add parameters to notification in case
when switching hdRadio enable
@ghost ghost force-pushed the feature/rc_radio_parameter_update branch from b90f1a3 to e0f4d08 Compare August 27, 2018 16:25
@AStasiuk
Copy link
Contributor

@jacobkeeler, all comments are processed, please have a look.

ventilationModeAvailable: true
}],
radioControlCapabilities: [{
moduleName: 'Radio Control Module',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing hdRadioEnableAvailable from radioControlCapabilities

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JackLivio new parameters hdRadioEnableAvailable and siriusxmRadioAvailable was added. See 9de70a2

upDownAvailable: false
}],
climateControlCapabilities: [{
moduleName: 'Climate Control Module',
Copy link
Contributor

@jacobkeeler jacobkeeler Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the issues were fixed, but this wasn't:

moduleName: 'primary_climate',

ventilationModeAvailable: true
}],
radioControlCapabilities: [{
moduleName: 'Radio Control Module',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this one is a duplicate key (see line 118)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AKalinich-Luxoft AKalinich-Luxoft force-pushed the feature/rc_radio_parameter_update branch from e0f4d08 to 0f69786 Compare August 28, 2018 07:44
@GetmanetsIrina
Copy link

@jacobkeeler , regarding the issue with the registration of several apps from different devices - bug was created #112 .
Also there is fix for this issue in #111 .

@jacobkeeler jacobkeeler merged commit 3e81a3f into develop Aug 28, 2018
@Jack-Byrne Jack-Byrne mentioned this pull request Oct 19, 2018
@jacobkeeler jacobkeeler deleted the feature/rc_radio_parameter_update branch March 18, 2020 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants