smartdevicelink / sdl_evolution

Tracking and proposing changes to SDL's public APIs.
https://smartdevicelink.github.io/sdl_evolution/
BSD 3-Clause "New" or "Revised" License
33 stars 122 forks source link

[Accepted with Revisions] SDL 0315 - Add RPC Conflict Management #1068

Closed theresalech closed 3 years ago

theresalech commented 4 years ago

Hello SDL community,

The review of the third revision(s) of the proposal "SDL 0315 - Add RPC Conflict Management" began on February 23, 2021, and continues through March 09, 2021.

The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0315-Add-RPC-Conflict-Management.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/1068

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

joeljfischer commented 3 years ago

Restarting numbering.

1. In the motivation, you state:

Management of RPC conflicts with OEMs may increase the difficulty of implementing middleware.

What kind of middleware are you envisioning here?

2. In the motivation, you state:

Since there is no guideline for RPC conflicts, implementations vary for each OEMs, thus, it is hard to standardize.

Is this a needed goal? Also, this proposal doesn't seem to solve this problem. After all, implementations will still vary by OEM and there won't be standardization since this is all customizable.

Jack-Byrne commented 3 years ago
  1. The number ranking for rpc_priority and hmi_status_priority starts at 1, while the number ranking for app_priority starts at 0 for emergency?

Is this because emergency is considered a higher priority than all other attributes? Maybe we should just remove the configuration of app_priority:EMERGENCY since we designate those apps as the highest possible priority.

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, Thank you for re-numbering. 1. We think the word "middleware" is wrong, correct is HMI. Therefore, we think we would like to modify the Motivation like below. (We will use the figure 1 as it is.)

When multiple apps operate, ON-screen (ONS) messages and text-to-speech (TTS) RPCs are requested together. However, the current SDL Core does not have the function to manage these conflicts. Thus, all requests are notified to HMI and managed by the OEMs themselves. For instance, ONS and TTS conflicts are managed by prioritizing the latter RPC. The figure below demonstrates the sequence process when PerformAudioPassThru occurs during ScrollableMessage.

The example above shows that the latter RPC PerformAudioPassThru is displayed and aborts ScrollableMessage. However, this kind of method has the following problem that management of RPC conflicts with OEMs may increase the difficulty of implementing HMI.

To solve this problem, we propose to add a new RPC conflict management function to SDL Core.

2. Since you are correct, we think we would like to remove this sentence like No.1 above.

@JackLivio -san, 3.

Is this because emergency is considered a higher priority than all other attributes?

Yes, That's right. We specify the EMERGENCY as the highest priority than all other attributes according to this comment No.12. As you say, maybe the EMERGENCY is not needed on the app_priority. However, if the EMERGENCY is removed from the app_priority list, anyone ask us why there is not the EMERGENCY in the app_priority. Therefore, we decided to remain the EMERGENCY as the highest priority in the list.

Jack-Byrne commented 3 years ago
  1. 👍
jordynmackool commented 3 years ago

The Steering Committee voted to keep this proposal in review on 2020-12-15 to allow for the conversation to continue on the open items.

joeljfischer commented 3 years ago

2. To be clear then, given that this goal is now removed, the only motivation for this proposal is to decrease the difficulty of implementing an HMI?

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, 2. As you say, by implementing and sharing this function onto SDL Core, to reduce the defects and the difficulty of HMI implementation is the one of purposes. In addition, by to store the priority table to policy table, to be easy to understand the behavior and to be able to set the priority order are also purpose.

joeljfischer commented 3 years ago

2. I don't know that "to be easy to understand the behavior and to be able to set the priority order are also purpose." is a good motivation. App developers can't understand the behavior from the policy table, so this isn't helpful. Being able to set the priority order can already happen in the HMI.

Given all of that, it's my opinion that the benefits here do not outweigh the downsides, which I see as very large.

joeljfischer commented 3 years ago

1. I'm sorry, I missed responding to this one.

For instance, ONS and TTS conflicts are managed by prioritizing the latter RPC.

This isn't always true right? This is dependent on the HMI implementation?

The example above shows that the latter RPC PerformAudioPassThru is displayed and aborts ScrollableMessage. However, this kind of method has the following problem that management of RPC conflicts with OEMs may increase the difficulty of implementing HMI.

While true, it's just shifting the burden to the HMI development on the policy server / Core setup side. It may be less burdensome, but it's certainly not no burden. HMI developers will still have work to do here.

jordynmackool commented 3 years ago

To provide clarity of the current status of this proposal review, here's a summary of the items in discussion on this review issue:

Open Items (author to respond):

1. [Livio] See comment here.

2. [Livio] I don't know that "to be easy to understand the behavior and to be able to set the priority order are also purpose." is a good motivation. App developers can't understand the behavior from the policy table, so this isn't helpful. Being able to set the priority order can already happen in the HMI. Given all of that, it's my opinion that the benefits here do not outweigh the downsides, which I see as very large.

No action required:

3

jordynmackool commented 3 years ago

The Steering Committee voted to keep this proposal in review on 2021-01-12 to allow for the author to respond to the open items, summarized in this comment.

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, 1.

This isn't always true right? This is dependent on the HMI implementation?

I think what you say is correct. It depend on the HMI implementation. However, we assume that almost of HMI implementation are managed by prioritizing the latter RPC.

To reduce the burden of HMI implementation is the one of aims of this proposal.

HMI developers will still have work to do here.

What's the work to do here? Please teach us.

2. Certainly, since app developers may not know the policy table itself, I don't think they can understand the behavior from the policy table. However, this proposal is for OEMs. For OEMs, to be able to share priority order method will reduce defects. We also assume that to be able to customize the priority order can be more close to OEM specification. Therefore, we assume this proposal has merits.

joeljfischer commented 3 years ago

1.

However, we assume that almost of HMI implementation are managed by prioritizing the latter RPC.

I don't know that that's true; in fact, I'm fairly sure it is not. I would recommend removing this from the proposal.

HMI developers will still have work to do here. What's the work to do here? Please teach us.

The HMI developers will still have to setup the policy table and determine how they want the conflict resolution to work. There's still a lot of configuration that needs to be done. They will still have work to do. It's less work, but it's still work and decisions to be made.

2.

For OEMs, to be able to share priority order method will reduce defects.

OEMs can already create a document that describes the priority order they have created. Sharing the policy table implementation probably won't be as helpful as a clearer description they can share. HMI developers will already be using version control to share the exact code with the priority order algorithms. I don't think that "being able to share priority order method" will reduce defects.

We also assume that to be able to customize the priority order can be more close to OEM specification.

This is simply false. Reducing the flexibility an OEM has to implement priority ordering does not help it be closer to their ideal specification.

jordynmackool commented 3 years ago

The Steering Committee voted to keep this proposal in review on 2021-01-19 to allow for conversation to continue on open items 1 & 2.

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, 1.

I would recommend removing this from the proposal.

OK. We will remove this sentence.

The HMI developers will still have to setup the policy table and determine how they want the conflict resolution to work. There's still a lot of configuration that needs to be done. They will still have work to do. It's less work, but it's still work and decisions to be made.

Please let us know the details if need to show on this proposal. We will maintain this proposal unless your advice.

2.

I don't think that "being able to share priority order method" will reduce defects.

Since the system has a stable method of priority order if there is no defect in this method, we assume the defects are overwhelmingly fewer than creating a new one.

Reducing the flexibility an OEM has to implement priority ordering does not help it be closer to their ideal specification.

At least, we assume it is useful for OEMs which wants to reduce the amount of code or wants to reduce the man power for bug handling this method.

joeljfischer commented 3 years ago

2.

Since the system has a stable method of priority order if there is no defect in this method, we assume the defects are overwhelmingly fewer than creating a new one.

I would object to the word overwhelmingly here. It should also be noted that because this feature is in Core, it becomes must more difficult and will take much more time to fix any bugs that do exist, compared to with the HMI implementation. I'm not arguing that implementing this proposal wouldn't reduce defects, it would, but I am arguing that it's less so than you're claiming, and the loss of flexibility to OEMs to implement this feature is significant.

You can continue with the proposal as is, I simply believe the proposal should be rejected because the feature that is offered isn't worth the downsides. Others may disagree.

Shohei-Kawano commented 3 years ago

@joeljfischer -san Thanks for your comments.

  1. We will wait your response.

  2. We will discuss how to proceed with this topic internally. If no conclusions have been reached by the next meeting, I think this proposal should be deferred. Also, our basic idea / opinion is that it is good to standardize the parts that can be shared and concentrate on the parts that the developer should originally decide or implement. By the way, this proposal is flexible because it has a common mechanism and can be customized in terms of behavior. I think it will be a point of what layer you want flexibility.

joeljfischer commented 3 years ago

1. Nothing to do here beyond what was already noted. I was just noting my objection to the proposal.

Shohei-Kawano commented 3 years ago

@joeljfischer -san, @jordynmackool -san

2. We have discussed how to proceed with this proposal internally. We believe that at this point we should vote on whether or not to proceed with this proposal. I think Joel-san's comments are unfair because they are implicitly perceived as withdrawing the proposal. I feel that it is unfair and a violation of etiquette to express opinions for or against the thread because it can be regarded as guidance. Also, this proposal has been reviewed since the end of August, but if there is any disagreement with the content of the proposal, it should be discussed at an early stage. We have made several revisions and are taking the time to consider them. At this stage, Joel-san's comments are trying to make our time meaningless. I'm wondering about the review process in the first place. If you don't first discuss and vote on what the proposal is and whether it's meaningful for your motivation, and then move on to technical discussions, the time you spend like us may be meaningless.

joeljfischer commented 3 years ago

Hello @Shohei-Kawano, I’m truly sorry that you feel that I am wasting your time. I want you to know that I take that seriously. However, I believe that there are some misunderstandings of the evolution review process that are leading to your frustration.

It is my opinion that while this proposal is helpful, the downsides of it are too great to outweigh the benefits. I think there is a misunderstanding of what evolution reviews are. They are fundamentally opinions, especially when referencing the motivation or how best to accomplish the task. Evolution reviews are intended to persuade the proposal author and SDLC that their opinion is correct. The reviewers, which can be anyone, the maintainer or otherwise, can express their opinions of support or opinions of what should change, but the proposal is still the author's. The author may choose to reject or ignore any review point that is offered to them and request that the proposal be put to a vote. The reviewers may then speak up against the proposal and it is up to the SDLC to decide what to do with it.

Deciding to take a point to SDLC vote can be done at any point you feel you don't want to make a suggested change or wish to move the proposal forward because there is a disagreement of opinion that you don't feel will be resolved, you can ask to take the point or proposal to an SDLC vote. The only changes that must be made are SDLC requested revisions.

If you’d like to discuss the motivation and topic of a potential proposal, you may bring it up for discussion in the SDL Slack. However, I disagree that the evolution process should be broken up into parts. Understanding the technical side of the discussion brings to light the limitations and drawbacks of any proposal. Almost every motivation is worthwhile to fix, but the fundamental issue to be discussed is the way it's fixed or the drawbacks of fixing it. Still, if that is a change you wish to see, you could propose a change to the SDLC process by making a PR in the sdl_evolution repo that changes the PROCESS.md document.

Please know that I'm sorry if it was not clear that the proposal could be moved to steering committee vote despite reviewers having a difference of opinion about the proposal. If you wish to do so, tag @jordynmackool that you wish to bring it to a vote. Please understand that I will be voicing my opinion in opposition of this proposal, though it is only my opinion as somebody with experience in this project. It will be up to the SDLC to decide on the result of the proposal.

Shohei-Kawano commented 3 years ago

@jordynmackool -san, We want to defer this proposal. Because we want to more discuss how to proceed internally.

Shohei-Kawano commented 3 years ago

@joeljfischer -san,

Do the shortcomings you're talking about fit in with "Off switch"? If "Off switch" features are added to this proposal, will your stance for or against it change?

jordynmackool commented 3 years ago

The Steering Committee voted to defer this proposal on 2021-01-26 at the author's request in this comment, asking for additional time to discuss internally how to proceed.

The proposal will remain deferred until the author has acknowledged (via email to sdlc@smartdevicelink.com) that they are ready to continue the conversation on the issue.

jordynmackool commented 3 years ago

The authoring company of this proposal has advised that they are now ready to respond to comments on this review issue. The Steering Committee can vote to bring this back into review when time allows.

jordynmackool commented 3 years ago

The Steering Committee has voted to bring this proposal back into review. The review will take place until our next meeting on February 09, 2021 (2021-02-09).

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, Thank you for your advice on slack. We will modify the proposal like below.

1. We will remove the following sentence from Motivation.

However, we assume that almost of HMI implementation are managed by prioritizing the latter RPC.

2. We will modify to add the off switch feature like below.

(1) Add below description of ON/OFF switch feature between HMI status priority table and RPC Conflict Management Module in Proposed solution.

Adding ON/OFF switch feature of RPC Conflict Management function

The following parameter described in SmartDeviceLink.ini are explained below. The parameter of UseCoreRpcConflictManagement shows whether to use the RPC conflict management method implemented on SDL Core. For example, if the UseCoreRpcConflictManagement is set to Disable, SDL Core sends RPCs to HMI as it is without using the RPC conflict management method implemented on SDL Core. The system needs to manage the RPC conflict by using OEMs own method. The default value is Disable. Thus, it also can handle the existing system. Also, if the UseCoreRpcConflictManagement is set to Enable, SDL Core sends only one RPC to HMI specified according to the priority tables above by using the RPC conflict management method implemented on SDL Core.

Below shows the example of UseCoreRpcConflictManagement parameter:

[RPCConflictManagement]
; Set whether to use the RPC conflict management method implemented on SDL Core.
; Disable means SDL Core throw out RPCs as it is. (SDL Core do not use the RPC conflict management method implemented on SDL Core)
; Enable means SDL Core throw out only one RPC according to priority tables in policy table by using the RPC conflict management method implemented on SDL Core.
UseCoreRpcConflictMamagement = Disable

(2) Removing 1 and modifying 2 like below in Potential downsides.

Implementing this feature will force only one modal RPC to ever be displayed for every OEM implementing SDL. As a result, the system can not support multi ON-screen(ONS) and text-to-speech(TTS) RPCs notification. However, by using the parameter of UseCoreRpcConflictManagement with Disable, since SDL Core can correspond to multi ONS and TTS RPCs, the system can support multi ONS and TTS RPCs notification.

joeljfischer commented 3 years ago

Hi @Akihiro-Miyazaki,

1. Looks good

2. A few notes on this:

a. I like the UseCoreRpcConflictManagement parameter. The only change I might ask for there is to use true / false instead of Enable / Disable. @JackLivio do you know what's most common in the ini file?

b. See my revised change to (2) downside:

Enabling this feature will force only one modal RPC to ever be displayed for every OEM implementing SDL. As a result, the system can not support multi ON-screen(ONS) and text-to-speech(TTS) RPCs notification.

However, by using the parameter of UseCoreRpcConflictManagement set to Disable, SDL Core can continue to pass through ONS and TTS RPCs to the HMI as happens now.

3. Upon further reflection, I think that SubtleAlert should be noted as exempt from this proposal and always sent the HMI. I don't see any case where SubtleAlert should not be presented alongside another ONS / TTS RPC. This gets a little tricky since the SubtleAlert has a TTS component, but that is something the HMI would have to manage if there's another TTS going on (e.g. from a PerformInteraction). I think that when this proposal feature is enabled, SubtleAlert becomes much less useful. It's not useful to display a notification if a choice set has to be dismissed to see it.

I'm okay with this proposal going through without this, but I do think it's a downside that really hurts SubtleAlert.

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, Thank you for your a lot of advice.

2a. We will follow the common. @JackLivio -san, Please let us know the rule of .ini file. Thank you.

2b. Thank you so much for your advice. We will modify the Potential downsides according to your advice. Also, if the common is True/False, then modify it.

3. To remove SubtleAlert from RPC priority table means to become lower priority than other RPCs written in RPC priority table. Because the following is shown in this proposal.

OEMs can delete any RPC priority. For example, if the priority of UI.Slider is deleted as shown below, its priority will be the same as a normal RPC and will be lower than any RPC in the RPC priority table.

Therefore, we will modify to add the sentence below.

Basically, it is possible to add or remove RPCs from RPC priority table. If RPC is removed, its priority is lower than other RPCs written in RPC priority table. However, regarding SubtleAlert, it becomes out of scope of this proposal due to the characteristics of this RPC and works as before.

joeljfischer commented 3 years ago

3. For clarification on this point, you note that if Slider is removed from the table, it will be lower priority than other RPCs and will therefore be dismissed or not appear if another ONS RPC from the table is presented or already present. I understand this part.

However, you also note that SubtleAlert will not function that way? Are you saying that it will still appear alongside other ONS items if it's not on the table and will not function like Slider does? Or will it function like you noted for Slider and be dismissed / not appear if another ONS RPC is already displayed. If possible, could we add to the proposal the following language?

Because SubtleAlert is designed to work alongside other ONS RPCs, it it exempted from this proposal and will be sent to the HMI alongside any other ONS RPC. It will be up to the HMI to determine if the SubtleAlert contains TTS data and whether or not to play that TTS data alongside showing the SubtleAlert UI based on whether other TTS data is currently playing from another RPC.

Is this acceptable? It does put slightly more work on the HMI developer to handle TTS conflicts with SubtleAlert but allows SubtleAlert to continue in its primary function.

Jack-Byrne commented 3 years ago

2a.

https://github.com/smartdevicelink/sdl_core/blob/master/src/appMain/smartDeviceLink.ini#L106

If it is singular parameter it can be added to the main section

; description
param = value

Suggesting:

EnableRPCConflictManager = true
Shohei-Kawano commented 3 years ago

@joeljfischer -san, @JackLivio -san Thank you for your advices. There is no objection to both 3. and 2a.

jordynmackool commented 3 years ago

The Steering Committee voted to return this proposal for revisions on 2021-02-09 to allow the author to update the proposal to include the agreed-upon revisions for items 1, 2, and 3.

Revision summary:

  1. Remove the below sentence from the "Motivation" section of the proposal:

    However, we assume that almost of HMI implementation are managed by prioritizing the latter RPC.

2a. Make EnableRPCConflictManager = true

2b. Modify the "Potential downsides" section in the proposal to include the below sentences:

Enabling this feature will force only one modal RPC to ever be displayed for every OEM implementing SDL. As a result, the system can not support multi ON-screen(ONS) and text-to-speech(TTS) RPCs notification. However, by using the parameter of UseCoreRpcConflictManagement set to Disable, SDL Core can continue to pass through ONS and TTS RPCs to the HMI as happens now.

  1. Remove references to SubtleAlert from tables and other comments about implementation and add the below sentence to the "Proposed solution" section of the proposal:

Because SubtleAlert is designed to work alongside other ONS RPCs, it is exempted from this proposal and will be sent to the HMI alongside any other ONS RPC. It will be up to the HMI to determine if the SubtleAlert contains TTS data and whether or not to play that TTS data alongside showing the SubtleAlert UI based on whether other TTS data is currently playing from another RPC.

jordynmackool commented 3 years ago

The author has updated this proposal based on the Steering Committee's agreed-upon revisions, and the revised proposal is now in review until February 23, 2021.

The specific items that were updated since the last review can be found in this merged pull request: https://github.com/smartdevicelink/sdl_evolution/pull/1123

joeljfischer commented 3 years ago

Hi, I'm very sorry that I have not been able to review this due to the upcoming release. Could we please keep this in review?

jordynmackool commented 3 years ago

The Steering Committee voted on 2021-02-24 to keep this proposal in review to allow for additional review time.

Shohei-Kawano commented 3 years ago

@joeljfischer -san Can you complete the review by this week's steering committee meeting?

joeljfischer commented 3 years ago

@Shohei-Kawano I apologize. I've been so busy with this upcoming release that it has been difficult to find time to review this proposal.

I'm going to restart numbering:

1. In the proposed solution you note:

OEMs can modify the App priority table and adjust the priority of application according to their own specifications. In fact, since EMERGENCY is set independently as the highest priority, the priority is determined by the items excluding EMERGENCY. For RPCs with the same priority, the HMI Status priority table, which is described later, will be used to determine the priority.

However, the table above this paragraph and the JSON example below it that contains the EMERGENCY option. Should this be removed or otherwise note that setting it doesn't do anything? Or am I misunderstanding this paragraph?

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san,

1. Please see JackLivio's comment and my comment.

As you say, maybe the EMERGENCY is not needed on the app_priority. Since we avoid that anyone ask us why there is not the EMERGENCY in the app_priority, we decided to remain the EMERGENCY as the highest priority in the list.

We hope this comment will be helpful for your understanding.

jordynmackool commented 3 years ago

The Steering Committee voted to keep this proposal in review on 2021-03-02 to allow for the conversation to continue on the open item.

joeljfischer commented 3 years ago

1. I apologize for missing that previous conversation. Perhaps we could clarify it in the following way?


Table 3. Default settings of App priority table

App priority (String) Priority (INT) Note (String)
EMERGENCY 0 Automatically always the top priority
NAVIGATION 1 Highest priority
VOICE_COMMUNICATION 2
COMMUNICATION 3
NORMAL 4
NONE 5 Lowest priority

OEMs can modify the App priority table and adjust the priority of application according to their own specifications. However, because EMERGENCY is set always the highest priority, the priority is determined by the items excluding EMERGENCY. Including EMERGENCY will change nothing. For RPCs with the same priority, the HMI Status priority table, which is described later, will be used to determine the priority.

Below shows the Json example for the App priority table:

"app_priority":{
    "EMERGENCY": 0, // Does not need to be included, if it is included it will be ignored no matter its assigned priority. See above.
    "NAVIGATION": 1,
    "VOICE_COMMUNICATION": 2,
    "COMMUNICATION": 3,
    "NORMAL": 4,
    "NONE": 5
}

If that is not acceptable, I'm okay with leaving it as is. I think the above changes would just help for clarification in implementation.

Akihiro-Miyazaki commented 3 years ago

@joeljfischer -san, 1. Thank you for your advice.

We think your suggestion is helpful for clarification in implementation. Therefore, we will modify proposal solution according to your suggestion.

jordynmackool commented 3 years ago

The Steering Committee voted to accept this proposal with revisions. The author is to update the proposal to include the agreed-upon revisions in this comment (which include modifying table 3, and the sentence following it).

jordynmackool commented 3 years ago

@Akihiro-Miyazaki please advise when a new PR has been entered to update the proposal to reflect the agreed-upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in impacted repositories for implementation. Thanks!

jordynmackool commented 3 years ago

The proposal has been updated and implementation issues have been entered:

Note: HMI issues have not been entered as it is believed that they will only need to be tested against for this proposal. If this is not the case when implementation for this proposal begins then issues can then be entered.