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 0329 - OnDataResumed Notification #1121

Closed
jordynmackool opened this issue Feb 3, 2021 · 18 comments
Closed

[Accepted] SDL 0329 - OnDataResumed Notification #1121

jordynmackool opened this issue Feb 3, 2021 · 18 comments

Comments

@jordynmackool
Copy link
Contributor

jordynmackool commented Feb 3, 2021

Hello SDL community,

The review of the revised proposal "SDL 0329 - OnDataResumed Notification" begins now and, and continues through April 06, 2021.

  • The original review of this proposal took place from February 09, 2021 - to February 23, 2021.

The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0329-on-data-resumed.md

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

#1121

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
[email protected]

@Sohei-Suzuki-Nexty
Copy link

I'm sorry that the question was not about this proposal but about the fundamental part.
If we implement this proposal, we assume that the app needs to be processed in the following order:

  1. Check the resumed data by notification and compare it with the current state of the app
  2. Send data from the app if it is different from the state of the app.

From the perspective of the app developer, the state management is performed by the app, and the state may change when disconnected,
so we think it is less burdensome for the app to simply send all the data than to check the received resume data and send data from the app that does not match the current state of the app.

We think this proposal is a Proposal to notify you of the data that will be resumed with the current Resume feature and to clarify the data that needs to be resent.
What are the benefits of using the Resume feature instead of the app sending all the data?

Also, is there an app that currently sends only the necessary data after checking the document when the app is restarted?
If all the apps are resending all the data on resumption, we don't think this suggestion is necessary.

@Jack-Byrne
Copy link
Contributor

Hello thank you for these questions.

Overall resumption is a feature to optimize the SDL ecosystems connection process.

Resumption is used to reduce the time it takes for an app to achieve its previous state after an ignition cycle or the app unexpectedly disconnects/reconnects.

An app may have previously had a lot of menu items or choice sets or subscriptions. The time that it would take for the app to reach its previous state is greatly reduced by the resumption feature, especially in the case of parsing long lists of voice recognition information from the app.

This proposal is a step towards improving the resumption feature and making it actually usable for developers. We can accomplish that by first creating a notification to let a developer know what information is resumed and does not need to be resent.

Without this notification developers have to rely on borderline insider knowledge for how to handle resumption on a specific head unit integration depending on the SDL Core version.

@Sohei-Suzuki-Nexty
Copy link

@JackLivio -san
Thanks for the reply.
With the contents you explained, I was able to understand the merits.
We haven't talked about the disadvantages, so we would like to clarify that.

We think it has the following demerits.
・ If the state of the application has changed by the time it resumes, the processing time may increase.

If the state of the app has changed by the time it resumes, the processing time can be increased because the app needs to check the OnDataResumed notification and send the necessary data compared to the current state of the app.
If the processing time increases, we think there may be a difference between the resumed HMI state and the app state until the app resends the data.
We are concerned that problems may occur if there is user interaction from the HMI in the meantime.
Please check the figure below for reference.
image

Also, the longer it takes to reconnect after disconnecting, the more likely it is that the state of the app has changed, so please let me know under what conditions the resume function would run.

@jordynmackool
Copy link
Contributor Author

jordynmackool commented Feb 10, 2021

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

@joeljfischer
Copy link
Contributor

2. I think that the added structs here will lead to a lot of confusion. Their names sound so similar to the related function names that searching for the correct RPC and simply being able to remember which is which between, for example, a CreateInteractionChoiceSet and an ApplicationChoiceSet, or an AddCommand and a Command will be difficult. If this feature remains the same (see 3), I would advocate for renaming each of these to more clearly represent their relation to the resume feature.

3. I don't believe that this feature is very useful and is unlikely to be used by mobile developers. At least not the complete feature set. Because so much of app library usage is built atop the manager system, and because this update would not work with the manager system, I don't believe this proposal should be accepted. The manager system is built atop closure callbacks, and those are difficult to resume. Providing this data isn't helpful because the manager system tracks what data has been accepted by the head unit already.

To refactor this proposal into something that may be usable by the manager system in the future would look something like this:

<function name="OnDataResumed" functionID="OnDataResumed" messagetype="notification" since="x.x">
    <description>
        Relays resumption information to an application
    </description>

    <param name="choiceSets" type="Bool" mandatory="false">
    </param>

    <param name="mainMenu" type="Bool" mandatory="false">
    </param>

    <param name="ephemeralFiles" type="Bool" mandatory="false">
    </param>

    <param name="globalProperties" type="Bool" mandatory="false">
    </param>

    <param name="vehicleDataSubscriptions" type="Bool" mandatory="false">
    </param>

    <param name="remoteControlSubscriptions" type="Bool" mandatory="false">
    </param>

    <param name="windowInfo" type="Bool" mandatory="false">
    </param>
</function>

This is more useful because the eventual app library resumption manager will be able to know for certain what was and was not resumed, but it already knows what the current state of those individual items would be.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Feb 15, 2021

  1. When an app is resuming, the app does not receive its RAI response or any other message from SDL Core until resumption has finished processing by the HMI. The app state wont change during resumption because it hasn't finished registering yet. This is how resumption is currently implemented in Core, the proposal is not changing this behavior. The contents of the OnDataResumedNotification would be computed before the app receives its RAI response when the resumption status is calculated by Core.

Since resumption is an implemented feature in SDL Core, any issues you find with the current implementation should be submitted as issues to the appropriate repositories. I think this discussion is out of scope for this proposal review since it is questioning the existence of the resumption feature.

  1. Agreed, open to the suggestions proposed in 3. I had the verbosity of the RPC spec listed as a potential downside so I saw this comment coming.

  2. Thank you for the suggested revision. This proposal doesn't make sense if it is not found useful for the mobile libraries. I would be happy to accomodate the revised notification. Only suggested change would be to include the suffix "Resumed" on each parameter.

<function name="OnDataResumed" functionID="OnDataResumed" messagetype="notification" since="x.x">
    <description>
        Relays resumption information to an application
    </description>

    <param name="choiceSetsResumed" type="Bool" mandatory="false">
    </param>

    <param name="mainMenuResumed" type="Bool" mandatory="false">
    </param>

    <param name="ephemeralFilesResumed" type="Bool" mandatory="false">
    </param>

    <param name="globalPropertiesResumed" type="Bool" mandatory="false">
    </param>

    <param name="vehicleDataSubscriptionsResumed" type="Bool" mandatory="false">
    </param>

    <param name="remoteControlSubscriptionsResumed" type="Bool" mandatory="false">
    </param>

    <param name="windowInfoResumed" type="Bool" mandatory="false">
    </param>
</function>

@Sohei-Suzuki-Nexty
Copy link

@JackLivio -san
Thank you for your reply.
With the contents you explained, we understood that there is no problem that user operation is involved.
However, the following concerns remain.
・Processing time may increase.
If the app's state has changed by the time it resumes, the app will check the OnDataResumed notification and compare it with the current app's state.
Processing time may increase because the process of sending the required data is needed.

・The burden on application developers may increase.
From the app developer's point of view, it may be less burdensome for the app developer because it is simpler and easier to send all the data than to add the process by notification of the proposal and use the resume function.

Due to the above disadvantages, we think there is little advantage in adding this proposal.

Also, are there any conditions such as disconnection time for the resume function to be performed?
If it runs regardless of the time you are disconnected, it is highly likely that the state of the app has changed if you have been disconnected for a long time, and the above disadvantages are likely to occur.

I think this discussion is out of scope for this proposal review since it is questioning the existence of the resumption feature.

These are certainly questions about reopening features, however we don't think that feature extension with less merit is necessary because it only complicates the implementation.
Therefore, we believe this discussion is within the scope of the proposal.

@joeygrover
Copy link
Member

1.

@Sohei-Suzuki-Nexty
The app resumption feature is already implemented, this proposal is a much needed improvement. Currently the RAI response will only tell the library and developer if resumption was successful but there is no way to know what was resumed. Your comments are still out of scope because you do not specify how those points are directly affected by this new proposal and notification over the original feature itself.

Processing time: This is the entire point of the resumption feature to begin with and relates directly to the original feature. The processing time for 100's if not 1000's of choice sets for a voice recognition engine to build out grammar for far exceeds the processing time of a few logical checks on a mobile device or sending of a small RPC notification.

Easier for developer: Again this goes back to the original feature and not this enhancement. However, easier for developer does not mean it's a better design. Take the use case of someone stepping out of their car to get gas then going back in their car. They expect the app to resume from its previous point, not to start all over again which might include sending 1000's of choice sets on connection; that would be very bad UX. It should also be noted it is completely up to the developer to use the original feature or not, this proposal doesn't change that. There is also already an accepted proposal to implement a convenience method at the manager layer to reduce burden on the developer, this proposal enhances that. Though, this issue has nothing to do specifically with this proposal and the issue is out of scope.

Disconnection time. Once again, this is part of the original feature and not this proposal. This proposal does not affect when app resumption takes place and when it does not.

All these issues relate to the original feature and are not scoped for this proposal. I would urge the commenter to understand the original feature and only comment on how this proposal affects that feature. I still consider these points out of scope for this proposal and until the commenter can define exactly how this proposal affects the original feature and not rely on the original feature's defects or misunderstandings to raise issues they should not be debated on this proposal. The commenter is welcome to submit any proposals on how they would like to see the original feature changed.

2. & 3.

I do agree the original solution was likely to add a lot of potential issues in maintenance. I do believe that sending the actual contents that were resumed would serve some benefit, but the overhead is likely not worth it at this time. The manager layers in the app library as well as developers should at least be aware of which pieces of app data were resumed and the potential revisions look to handle that well.

4. I would like to just clarify when this notification is sent. Is this sent after RAI response but before the first OnHMIStatus? Or after that OnHMIStatus? I think defining that is important in terms of how the app libraries will handle this notification and the HMI status.

@Shohei-Kawano
Copy link
Contributor

@joeygrover -san, @JackLivio -san

  1. We are just worried about the time it takes to transfer the information notified by OnDataResumed.
    If the time of this transfer process is negligible, there is no particular reason to object to this proposal.
    If the transfer process simply takes a long time and the data is likely to change during disconnection, we are concerned that the time-consuming transfer process will often go back and forth.

@jordynmackool
Copy link
Contributor Author

The Steering Committee voted on 2021-02-16 to keep this proposal in review to allow for conversation to continue on open items 1-4.

@joeljfischer
Copy link
Contributor

1. The transfer time will be negligible based on the updated and agreed to design. It's just transferring a few booleans. Additionally, the entire feature is opt-in for the app developer by sending the resumeHash in the RAI, so if they don't want to deal with the notification, they don't have to use the feature.

@Sohei-Suzuki-Nexty
Copy link

Sohei-Suzuki-Nexty commented Feb 22, 2021

@joeljfischer -san, @joeygrover -san, @JackLivio -san

1.Our concern has been resolved with the update of item 3.
Thank you for responding to the comments.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Feb 23, 2021

@joeygrover

  1. To clarify this notification will be sent after the RAI response and before the onHMIStatus is sent.

@jordynmackool
Copy link
Contributor Author

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

Agreed Upon Revisions:

2/3. Update the proposal to include changes for the app libraries; See this comment.

<function name="OnDataResumed" functionID="OnDataResumed" messagetype="notification" since="x.x">
    <description>
        Relays resumption information to an application
    </description>

    <param name="choiceSetsResumed" type="Bool" mandatory="false">
    </param>

    <param name="mainMenuResumed" type="Bool" mandatory="false">
    </param>

    <param name="ephemeralFilesResumed" type="Bool" mandatory="false">
    </param>

    <param name="globalPropertiesResumed" type="Bool" mandatory="false">
    </param>

    <param name="vehicleDataSubscriptionsResumed" type="Bool" mandatory="false">
    </param>

    <param name="remoteControlSubscriptionsResumed" type="Bool" mandatory="false">
    </param>

    <param name="windowInfoResumed" type="Bool" mandatory="false">
    </param>
</function>

4. Clarify that the OnDataResumed is sent after the RAI response and before the onHMIStatus notification.

No action required:
1

@jordynmackool
Copy link
Contributor Author

The Steering Committee voted on 2021-02-24 to return this proposal for revisions. The author is to update the proposal to include the agreed-upon revisions summarized in this comment.

@jordynmackool jordynmackool changed the title [In Review] SDL 0329 - OnDataResumed Notification [Returned for Revisions] SDL 0329 - OnDataResumed Notification Feb 24, 2021
@smartdevicelink smartdevicelink locked and limited conversation to collaborators Feb 24, 2021
@jordynmackool
Copy link
Contributor Author

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

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

@jordynmackool jordynmackool changed the title [Returned for Revisions] SDL 0329 - OnDataResumed Notification [In Review] SDL 0329 - OnDataResumed Notification Mar 31, 2021
@smartdevicelink smartdevicelink unlocked this conversation Apr 6, 2021
@jordynmackool
Copy link
Contributor Author

The Steering Committee fully voted to accept this proposal on 2021-04-06.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Apr 7, 2021
@jordynmackool jordynmackool changed the title [In Review] SDL 0329 - OnDataResumed Notification [Accepted] SDL 0329 - OnDataResumed Notification Apr 7, 2021
@jordynmackool
Copy link
Contributor Author

Implementation issues have been added:

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

6 participants