Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Accepted] SDL 0326 - Handle Late Malformed HMI Responses #1099

Closed
jordynmackool opened this issue Nov 25, 2020 · 15 comments
Closed

[Accepted] SDL 0326 - Handle Late Malformed HMI Responses #1099

jordynmackool opened this issue Nov 25, 2020 · 15 comments

Comments

@jordynmackool
Copy link
Contributor

jordynmackool commented Nov 25, 2020

Hello SDL community,

The review of the revised proposal "SDL 0326 - "Handle Late Malformed HMI Responses" begins now and continues through January 12, 2021.

  • The original review took place from November 25, 2020, through December 08, 2020.

The proposal is available here:
https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0326-handle-late-malformed-hmi-responses.md

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

#1099

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:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

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

Thank you,
Jordyn Mackool

Program Manager - Livio
Jordyn@livio.io

@jordynmackool jordynmackool changed the title [In Review] SDL 0326 - "Handle Late Malformed HMI Responses [In Review] SDL 0326 - Handle Late Malformed HMI Responses Nov 25, 2020
@Sohei-Suzuki-Nexty
Copy link

Isn't it dangerous to just send the opposite command without knowing if the previous command has been processed by the HMI?
We think it is better to check the error status and handle it appropriately

We have a concern that if the HMI is times out with "too busy to process request" when sending an AddCommand, later deleteComand cannot be handled properly as well.
Could this be a problem?

Does it include a retry process, such as sending the command again, when AddCommand fails?
If not, wouldn't it be better to get the right information by retrying?

@Jack-Byrne
Copy link
Contributor

@Sohei-Suzuki-Nexty Thank you for the review.

  1. The HMI should be able to handle any delete/unsubscribe type command. If the requested data to be deleted/unsubscribed does not exist, then the HMI should respond to SDL Core with an error response.

I believe an HMI integration should be flexible enough to handle potentially invalid requests from SDL Core.

  1. Yes this issue was listed as a potential downside, but give a reasoning for implementation:

If the HMI is taking too long to process requests because the system is overloaded, SDL Core will be required to send more messages to the HMI to undo certain requests. Creating more requests will end up using more SDL Core -> HMI bandwidth.

However the proposed solution was selected because it aligns with existing and implemented behavior defined in proposals SDL 188 and SDL 190. These proposals add behavior where SDL Core will undo these same types of requests in case of a failed resumption scenario.

The current implementations of these proposals do nothing to revert requests that had late, missing, or malformed responses. I intended for this proposal to extend the functionality of reverting data implemented by those proposals to messages that are late, missing, or malformed.

  1. Retrying does seem like a reasonable request but this does come with a downside. The time for an app to register with resumed data would be extended by the time it takes to retry the requests. Worst case time to register would be * .

The default timeout is 10 seconds, each retry attempt would add an additional 10 seconds as long as the HMI responses continued to be late, lost, or malformed.

@Sohei-Suzuki-Nexty
Copy link

@JackLivio -san,
Thanks for your reply.

1
Also related to item 2,if the response such as Delete to the HMI is successful / unsuccessful / no response, what kind of HMI status can be considered?
If there are cases where the condition is unclear, we think it is better to think of a better method.

2
We understand the implementation, but we have a concern about item 1.

3
We understood.
We think it is better to add the retry process to the alternatives, including the reasons for not adopting it.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Dec 1, 2020

@Sohei-Suzuki-Nexty

  1. Resumed data results (success / unsuccess / no response) do not impact the resumed HMI status. An app can resume to HMI Level Full independently of the outcome of Cores attempts to resume data (subscriptions, submenus, menu items, etc).

If an app is resumed into HMI Level Full, but the AddCommands failed, an app will receive RESUME_FAILED result in the RegisterAppInterface response and the app will also receive OnHMIStatus HMILevel: FULL.

This is the current behavior of core, and the proposal wont change the outcome of the resumed HMI Level.

  1. I would be happy to had this information to the Alternate Solution Section of the proposal as a revision.

@Sohei-Suzuki-Nexty
Copy link

@JackLivio -san,

I'm sorry that the question is not clear.
What we would like to ask is not about HMILevel, but about the existence of indefinite state depending on the response results of AddCommand and DeleteComannd.

For example, if AddCommand was successful, if DeleteCommand was sent, or if DeleteComannd failed, was the command deleted or not?
We want you to clarify what the outcome would be for each possible situation.

@jordynmackool
Copy link
Contributor Author

The Steering Committee voted to keep this proposal in review on 2020-12-01 to allow for conversation to continue on open items 1 and 2.

Commenting members agreed on the revision for item number 3 to add the retry process to the “Alternatives considered” section, including the reasons for not adopting it.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Dec 7, 2020

@Sohei-Suzuki-Nexty

  1. If an HMI Delete Command is not successful, SDL Core does not consider the data to be deleted.

Existing code in the DeleteCommandRequest only removes menu items if the HMI DeleteCommands responses that were considered successful: https://github.com/smartdevicelink/sdl_core/blob/master/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/delete_command_request.cc#L206-L208

@Sohei-Suzuki-Nexty
Copy link

@JackLivio -san,
1.

If an HMI Delete Command is not successful, SDL Core does not consider the data to be deleted.

We understand how DeleteCommand works.

However, in that case, if there is no response to AddCommand, but DeleteCommand fails when the menu item is actually added, wouldn't it be unable to sync?
Also, if there is no response to AddCommand and the menu item is not actually added, we don't think it is necessary to send DeleteCommand.

Therefore, the proposed solution may send unnecessary commands, which we think is not the best method.

For example, we think it would be good to add an I/F that gets the status of the HMI menu command, check the status by that, and then let Core decide whether to delete or not.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Dec 8, 2020

However, in that case, if there is no response to AddCommand, but DeleteCommand fails when the menu item is actually added, wouldn't it be unable to sync?

They should be synced here. If the AddCommand failed, Core will remove the command from its own list. The DeleteCommand to the HMI is an attempt resync Core's list and the HMI's list in case the HMI for some reason is showing the failed AddComand.

Also, if there is no response to AddCommand and the menu item is not actually added, we don't think it is necessary to send DeleteCommand.

If the HMI has a valid error response for the AddCommand then the DeleteCommand will not be sent. It is known by Core and the HMI that the Command is not being displayed to the user.

In the other cases where a message is late, lost, or malformed, Core is unable to verify what the HMI did with that AddCommand. I believe in those cases the DeleteCommand should be sent.

For example, we think it would be good to add an I/F that gets the status of the HMI menu command, check the status by that, and then let Core decide whether to delete or not.

In my opinion adding a new message adds more technical debt than simply handling the existing RPC's. This HMI RPC would need to be added to every interface (buttons, ui, tts, rc, vd, bc, vr, nav, app services) for varying resumption data, therefore its really like adding 9 new HMI RPCs. This type of status message would need to be added for all resumed data + subscriptions.

I believe the problem case addressed in the proposal should only happen very rarely. I don't believe it warrants creating 9 new HMI rpc messages.

@Shohei-Kawano
Copy link
Contributor

@JackLivio -san

I reply on @Sohei-Suzuki-Nexty behalf.
Thank you for your detailed answer.

I understand that this proposal is not meant to ensure state synchronization, but to improve rare situations as much as possible.

If it's a solution for perfect synchronization, you need to answer/include our questions and requests, but if it's a solution for improvement as much as possible, we think that this method is better.

@jordynmackool
Copy link
Contributor Author

jordynmackool commented Dec 9, 2020

The Steering Committee voted to return this proposal for revision on 2020-12-08. Nexty, a commenting member on the issue noted during the meeting that further discussion on items 1 and 2 was not needed and their concerns were resolved.

The author is to update the proposal to include item 3’s revisions noted in this comment.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Dec 9, 2020
@jordynmackool jordynmackool changed the title [In Review] SDL 0326 - Handle Late Malformed HMI Responses [Returned for Revisions ] SDL 0326 - Handle Late Malformed HMI Responses Dec 9, 2020
@jordynmackool jordynmackool changed the title [Returned for Revisions ] SDL 0326 - Handle Late Malformed HMI Responses [Returned for Revisions] SDL 0326 - Handle Late Malformed HMI Responses Dec 9, 2020
@jordynmackool jordynmackool changed the title [Returned for Revisions] SDL 0326 - Handle Late Malformed HMI Responses [In Review] SDL 0326 - Handle Late Malformed HMI Responses Dec 16, 2020
@jordynmackool
Copy link
Contributor Author

jordynmackool commented Dec 16, 2020

The author has updated this proposal based on the Steering Committee's agreed-upon revisions. The review of the revisions starts now and continues until January 12, 2021.

The specific items that were updated since the last review can be found in this merged pull request: #1104

@smartdevicelink smartdevicelink unlocked this conversation Dec 21, 2020
@Sohei-Suzuki-Nexty
Copy link

@JackLivio -san

Thank you for the revision.
I have confirmed the revision of item 3.
There is no problem with the revision.

@jordynmackool
Copy link
Contributor Author

The Steering Committee fully voted to accept this proposal on 2021-01-12.

@smartdevicelink smartdevicelink locked as resolved and limited conversation to collaborators Jan 13, 2021
@jordynmackool jordynmackool changed the title [In Review] SDL 0326 - Handle Late Malformed HMI Responses [Accepted] SDL 0326 - Handle Late Malformed HMI Responses Jan 13, 2021
@jordynmackool
Copy link
Contributor Author

Implementation issues entered:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants