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 with Revisions] SDL 0250 - Next RPC Indication for the HMI #827

Closed
theresalech opened this issue Oct 2, 2019 · 48 comments
Closed

Comments

@theresalech
Copy link
Contributor

theresalech commented Oct 2, 2019

Hello SDL community,

The review of the revised proposal "SDL 0250 - Next RPC Indication for the HMI" begins now and runs through January 14, 2020. Previous reviews of the proposal took place October 2 - 15, 2019 and October 30 - November 19, 2019. The original review of this proposal took place October 2 - 15, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0250-NextRpcIndication.md

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

#827

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,
Theresa Lech

Program Manager - Livio
theresa@livio.io

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Oct 7, 2019

@MichaelCrimando Are you familiar with this accepted but not yet implemented proposal: Template Improvements: Additional SubMenus?

The additional submenus proposal allows for deeper submenus so the developer doesn't have to simulate menu depth by using multiple perform interactions.

Also there is a proposal that is implemented in SDLCore 6.0.0 that allows for in app menu tiles views.

I believe the pictured flow in the proposal could be accomplished using additional submenu depth and menu tiles without the need for the HMI anticipating future RPCs. Also the performance of using submenus over perform interactions would be much better and not require any loading screen.

@MichaelCrimando
Copy link
Contributor

Hey @JackLivio

That is a great proposal and very similar but executed in a different way. That is great for apps that want to PRE load all their choice sets. The proposal that I've created is great for apps that want to POST load all their choice sets. Plus the proposal here also applies to things like softbuttons given from a Show RPC, ScrollableMessage or PerformInteraction

@joeygrover
Copy link
Member

So reading the proposal the only use case that is really given is for a "Loading" screen to be displayed while waiting to receive an RPC of a certain function ID.

First to Jack's point, submenus do not have to be PRE loaded. They can be added at any time and I would expect that some of them are added POST load because of the new proposal. PRE loading all those choices will take forever if VR synonyms are attached, so it is likely that the sub menus are added as a skeleton and apps add to the menus as they are selected which mimics the chaining of choice set functionality. The choice set chaning that happens was an apps way to get around a lot of initial driver distraction requirements and the flow itself is very clunky. Obviously this doesn't address the soft buttons or ScrollableMessage use case though.

Regarding the solution itself, I think it still needs some work. The next function ID check works in best case scenarios, but what if multiple Shows are sent. For example, the manager layer will check if an image will need to be uploaded to Core before a Show is sent with that image name. If it does, it sends a Show without the image field to start, then once the file is uploaded, sends another show. Is it acceptable for the loading screen or wait dialog to be dismissed on the first show? Has the author through about other information that could be included for a more precise check? For example, message ID, correlation ID, etc?

For alternatives, what about being more accurate with the use case given of showing a loading screen? Possibly a new RPC that triggers a loading screen, and then uses the dismiss ability recently introduced. I really want to make sure we understand the use case to craft the right solution. Because another alternative could be to include the full RPC to process in sort of a SelectionListener if we want to speed things up, but again if it's only to show a loading screen that's overkill.

Furthermore, has the author looked into the implications this will have on the manager layer for the mobile libraries? Changes should be included in this proposal so it can be agreed upon if the proposal is accepted.

@BrettyWhite
Copy link
Contributor

In addition to what joey said, there are multiple places in the managers (file, soft button, text and graphic, menu, and especially choice set) that have asynchronous calls and that will pre-load menus for example without images, then automatically resend when the images are uploaded. Just something to keep in mind that RPCs may be sent asynchronously and automatically without the developer ever knowing.

@MichaelCrimando
Copy link
Contributor

Hey @joeygrover,

Those are some great points.

Going along with that, if you post load everything, you dont get the indicators (usually arrows) that something is next. Now if I understand what @BrettyWhite is saying, that could maybe be handled by Core where it knows something is coming next show it shows the indicator but doesn't bother to load anything yet.
nextrpc1
nextrpc2

Plus this functionality enables specfic loading screens that can be popupluated as the items load in. If Core wanted to get fancy, it could potentially load in only the first 10 items really fast, and work on the next items in the background. This is similar to how iPhone apps load long lists while keeping performance great.
nextrpc3
nextrpc4

Further, we have a need for the HMI to start pre-exposing things in the HMI before the user actually interacts with anything. If the HMI knows what comes next, and it's the type of RPC it's looking for, it can know what to expect and simulate the user pressing a certain button on the screen in order to get the required info from the app.
Here's one example from Tidal. The first item in the in-app menu for Tidal is "My Mixes"
nextrpc5

Now a benefit that the additional Submenus proposal can provide is that it can improve performance in going back and forth in menu structures. However, apps would have to take the time to reorganize their menu structure from PerformInteraction style to Menu style. In addition, addCommand items can only have 1 text field and 1 image. Choice items can have 3 text fields and 2 images, so apps might not want to move from PerformInteraction styles. Also I don't know if this is handled on the Core side, but submenus are completely locked out while driving on SYNC3 for MY16-19 vehicles. Apps would then have to maintain 2 different styles of menu structure depending on what version of SYNC it's on.

@kshala-ford
Copy link
Contributor

The motivation of this proposal is trying to address three issues that exist today:

  1. Better HMI possibilities: can show forward and backward arrows ... which is a feature of sub menus by the nature of sub menus. Choices could benefit from the feature but actually SDL-0148 made sub menus compete with choice sets... Seems like the proposal prioritizes sub menus as it says that on older head units it can automatically translate those sub-submenus into Choice Sets
  2. Smart Driver Distraction: if a softbutton or choice would lead to a screen that's normally locked ... this feature doesn't exist yet for choices and the proposal would enable us to do so. It enables the system to detect use cases with many steps. For sub menus it also exist today assuming pre-loading by the app.
  3. The HMI can have a much smoother experience with native HMI loading screens which is true for choice sets when initializing the app. Also it helps the "loading"-screen practice

I think the motivation of this proposal is that head units are slow especially with voice and that user interaction can only start once all the content is loaded. Knowing that the use case continues with a choice/menu conmand is very helpful to improve the experience. The proposal says that the HMI could show some loading indication. It's very helpful for the proposal understanding but I want to note that it'll be the SDL integrator responsibility on how the HMI should do this and that it should be out of scope. For instance when I tap on a choice which has a next RPC indication I could think of the HMI to make the choice change to show a "loading circle" as the choice image... again this is the decision of the HMI.

@MichaelCrimando already mentioned background loading which I believe is where we should have some focus. As of right now apps don't know what menu level or what menu entries are visible (except the root level with OnHMIStatus.systemContext = MENU). I think the method of table views in iOS would be a really big benefit to SDL app menus. AddSubMenu should have a parameter to tell how many entries this sub menu will contain but they entries don't need to be created. The HMI will do it's UI and tell the app if the menu is visible and what menu entries the app needs to create (e.g. extending OnHMIStatus with menu id).

The same could be done for PerformInteraction which would already start and show only choices and choice images that are visible. I believe this would require a larger API change.

Regarding the parameter nextFunctionInfo I am wondering about what should happen if the app fails to perform the next function. I'm thinking of specifying a timeout as an HMI requirement. Also I think it might be sufficient if choices would just provide an indication as a flag/bool. Then the HMI would wait for any kind of overlay RPC from the app (another PerformInteraction or Alert, ScrollableMessage, Slider etc.).

@joeljfischer
Copy link
Contributor

PRE loading all those choices will take forever if VR synonyms are attached, so it is likely that the sub menus are added as a skeleton and apps add to the menus as they are selected which mimics the chaining of choice set functionality.

Currently this is not possible. There is no callback when a submenu is pressed; therefore, the entire menu structure has to be preloaded IIRC. This is a major deficiency of the current menu setup.

Has the author through about other information that could be included for a more precise check? For example, message ID, correlation ID, etc?

I don't think that either messageID nor correlationID would work very well in this scenario. The manager layer handles these ids, so a complicated system of "reserving" an ID for a particular message and then the developer indicating that some screen manager call is meant for a "reserved" ID would have to take place, and this would be extremely complicated.

For alternatives, what about being more accurate with the use case given of showing a loading screen? Possibly a new RPC that triggers a loading screen, and then uses the dismiss ability recently introduced.

This already exists within Alert (the progressIndicator boolean parameter), and is probably a better solution, but doesn't handle showing the "arrow" functionality.

Going along with that, if you post load everything, you dont get the indicators (usually arrows) that something is next.

This could be added to Choice and SoftButton through a boolean parameter, but Choices should probably move over to menus for cases where they present another PerformInteraction if possible.

Plus this functionality enables specfic loading screens that can be popupluated as the items load in. If Core wanted to get fancy, it could potentially load in only the first 10 items really fast, and work on the next items in the background. This is similar to how iPhone apps load long lists while keeping performance great.

This is not currently possible given the scheme of PerformInteraction. Everything must be pre-loaded, ironically. This would be more possible if we created additional RPCs to enable it, but it's currently not possible.

Further, we have a need for the HMI to start pre-exposing things in the HMI before the user actually interacts with anything. If the HMI knows what comes next, and it's the type of RPC it's looking for, it can know what to expect and simulate the user pressing a certain button on the screen in order to get the required info from the app.

This is unclear to me, could you elaborate? The diagram is unclear.

In addition, addCommand items can only have 1 text field and 1 image. Choice items can have 3 text fields and 2 images, so apps might not want to move from PerformInteraction styles.

This is true. We should expand AddCommand to enable further functionality if possible.

Also I don't know if this is handled on the Core side, but submenus are completely locked out while driving on SYNC3 for MY16-19 vehicles. Apps would then have to maintain 2 different styles of menu structure depending on what version of SYNC it's on.

I think you're missing the point here. The fact that submenus are locked out while driving doesn't mean that developers should circumvent that lock out by using PerformInteraction. In fact, PerformInteraction depth should be similarly locked out. However, your point is good in terms of the app would have to maintain both if they were using beyond two menu depth (unless we worked around it in the manager layer). But, that doesn't help this proposal at all either.

As of right now apps don't know what menu level or what menu entries are visible (except the root level with OnHMIStatus.systemContext = MENU). I think the method of table views in iOS would be a really big benefit to SDL app menus.

Agreed, I've had a task sitting in my backlog for a few months now to think about this. Your ideas here are pretty good. I think that this task (of making menus load more dynamically) is more worthwhile than this proposal in general.


I think that the only functionality this proposal offers is two-fold:

First, to "disable" some soft buttons when driver distraction says that those buttons' effects will be locked out. This is a decent improvement, but I'm not sure that it's worth the effort.

Second, to show "better" loading screens, i.e. instead of an Alert with progressIndicator=true, they could show the PerformInteraction or ScrollableMessage without the data and a loading indicator, then load it in. I think that this could be better accomplished by the dynamic loading of PerformInteraction and AddCommand / AddSubmenu.

@MichaelCrimando
Copy link
Contributor

@joeljfischer just to elaborate on a couple of points

Currently with Tidal, the in-app menu lists "My Mix, My Albums, My Playlists, My Tracks, and Explore"
So on 8" SYNC it goes something like this - Home screen -> Menu -> Perform Interaction
image
image
image

So what the HMI on a future headunit with a larger screen size could do with this proposal is look at the in-app menu and see that the first menu item will lead to a performInteraction (or submenu... but that part isn't new). It can send an onCommand to the app, and have the upcoming performInteraction hang out in the side bar like so: (ignoring performInteraction timeout)
image
Further, the HMI could do something effectively like "Hey the first item in the menu will launch a scrollableMessage, so skip that one. However the second item in the menu will launch a performInteraction so send OnCommand for that"

Just a side note I have: you could use a bool flag for this functionality, however the proposal give you that same information (is a RPC next?) and more (WHICH RPC is next?).

Submenu is locked out completely on MY16-19 on SYNC vehicles which makes it pretty useless in the car vs PerformInteraction is page limited. I actually did the proper work to make sure that submenu is page limited like PerformInteraction is so apps can actually use it.

@theresalech
Copy link
Contributor Author

A major point of discussion during the Steering Committee meeting with regards to this proposal was that it suggests using PerformInteraction in a way that was not intended. It was clarified that the original intention of PerformInteraction was for very dynamic pieces of data that need to be loaded at run time, like POI searches. The author noted that application developers are currently using PerformInteraction to simulate menu depth, and therefore this proposal improves that experience, but it was then questioned by SDLC Members and the Project Maintainer if we should instead focus on improving menus so developers can use those as intended. While there was discussion about this proposal as a short term solution, it was noted that this would likely instead result in two different solutions that both need to be maintained, and could be confusing to developers.

Ultimately, the Steering Committee voted to defer this proposal, and keep it in review until our next meeting. SDLC Members and the Project Maintainer can continue discussion on the review issue, focusing on the motivations for the proposal outside of menu replacement items.

@MichaelCrimando
Copy link
Contributor

I believe I addressed all the main comments with PR #843

@theresalech
Copy link
Contributor Author

@MichaelCrimando thanks! As this proposal is currently [In Review], we'll need to let the Steering Committee continue to review until our meeting next week. If during the meeting, they agree to return for revisions, we will document what those revisions need to be, and can then bring your revised proposal (PR 843) in for review the following week once we've ensured those revisions are incorporated.

@MichaelCrimando
Copy link
Contributor

@theresalech ah ok!
If it helps at all, I didn't update any functional pieces of it, just the motivation around it for future documentation

@theresalech theresalech changed the title [In Review] SDL 0250 - Next RPC Indication for the HMI [Returned for Revisions] SDL 0250 - Next RPC Indication for the HMI Oct 16, 2019
@theresalech
Copy link
Contributor Author

The Steering Committee voted to return this proposal for revisions. The author will update the proposal to update the documentation and motivation specifying that add command/sub menu structure is the main structure for apps.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Oct 16, 2019
@theresalech theresalech changed the title [Returned for Revisions] SDL 0250 - Next RPC Indication for the HMI [In Review] SDL 0250 - Next RPC Indication for the HMI Oct 30, 2019
@theresalech
Copy link
Contributor Author

The author has revised this proposal based on the Steering Committee's feedback, and the revised proposal is now in review until 2019-11-05.

@smartdevicelink smartdevicelink unlocked this conversation Oct 30, 2019
@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Nov 4, 2019

@MichaelCrimando I think the revision puts the use cases for the feature in a much better context.

I have a comment regarding the HMI API implementation. The HMI does not have or use a record of mobile function IDs. When an HMI receives an RPC it receives it as "InterfaceName.RPCType" ie (BasicCommunication.UpdateAppList)

I think nextFunctionID should be renamed to nextRPC and changed to type string in the HMI API.

HMI_API.xml:

<struct name="NextFunctionInfo" since="x.x">
	<description>
		Outlines information about the next RPC that will be triggered.		
	</description>
	<param name="nextRPC" type="String" mandatory="true"/>
		<description>The next function (RPC) that will be triggered by selecting the current option/command/choice etc.</description>
	</param>
...

Core will be able to take an integer function ID and convert it into the correct HMI RPC String.

@MichaelCrimando
Copy link
Contributor

Thanks for the info @JackLivio
Now should we add a bit of that info into the description of the param?
Like

<description>The next function (RPC) that was translated from InterfaceName.RPCType that will be triggered by selecting the current option/command/choice etc.</description>
	</param>

@theresalech theresalech changed the title [In Review] SDL 0250 - Next RPC Indication for the HMI [Returned for Revisions] SDL 0250 - Next RPC Indication for the HMI Nov 20, 2019
@theresalech
Copy link
Contributor Author

The Steering Committee voted to return this proposal for revisions. The author will revise the proposal to include the manager-level implementation as discussed in the comments on this review issue with the Project Maintainer.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Nov 20, 2019
@theresalech theresalech changed the title [Returned for Revisions] SDL 0250 - Next RPC Indication for the HMI [In Review] SDL 0250 - Next RPC Indication for the HMI Dec 18, 2019
@theresalech
Copy link
Contributor Author

The author has revised this proposal to incorporate the requested revisions from the Steering Committee. The revised proposal is now in review and will be voted upon during the 2020-01-14 meeting.

@smartdevicelink smartdevicelink unlocked this conversation Dec 18, 2019
@Jack-Byrne
Copy link
Contributor

Apologies if i missed this in the conversation but the enum type "FunctionID" does not exist in the HMI API. We could add FunctionID to the HMI to fix this issue but that needs to be mentioned in the proposal.

Alternatively if we do not add that enum to the hmi api, NextFunctionInfo would require a slightly different param definition for nextFunctionID. In the HMI API nextFunctionID could be a string with values similar to "BasicCommunication.PerformInteraction".

@Shohei-Kawano
Copy link
Contributor

If it is not those Functions that are assigned to the menu but screen transitions, how should NextFunction be set?
Also, how do you determine whether the screen transition destination cannot be operated by driver distraction?

@joeljfischer
Copy link
Contributor

@Shohei-Kawano I'm going to number your questions.

1. NextFunction is set based on the developer's knowledge of what the button press will do when pressed.
2. The HMI determines that and then uses the NextFunction info to show its own UI based on that.

@MichaelCrimando
Copy link
Contributor

Hey @JackLivio
Good point. Logic-wise adding an enum to the HMI api is more straight forward to me. It just avoids some developer errors that can come up when you use a string instead. When this gets accepted, I can just add in that the enum will be added to the HMI API

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Jan 14, 2020

@MichaelCrimando Since on the HMI side since there isnt really a concept of function IDs, I believe the alternate I suggested to be a better option (using strings). All rpcs communicated between core and HMI already use strings for the rpc message type.

Here is an example rpc GetVehicleData Request, see method:
{
id: 19,
jsonrpc: "2.0",
method: "VehicleInfo.GetVehicleData",
params: {
vin: true
}
}

@MichaelCrimando
Copy link
Contributor

@JackLivio
Gotcha, so what would we add to the proposal?
Just something like

The HMI API doesn't use function IDs so SDL Core would communicate the next function through strings:
  NextFunction.Default
  NextFunction.PerformChoiceSet
  NextFunction.Alert
  NextFunction.ScreenUpdate
  NextFunction.Speak
  NextFunction.AccessMicrophone
  NextFunction.ScrollableMessage
  NextFunction.Slider
  NextFunction.SendLocation
  NextFunction.DialNumber
  NextFunction.OpenMenu

@MichaelCrimando
Copy link
Contributor

From today's call @JackLivio how about I add

The HMI API doesn't use function IDs so SDL Core would communicate the next function ID through the existing RPC interface strings in the format of `interfaceName.RPCMessage".

@Shohei-Kawano
Copy link
Contributor

I’m sorry that I couldn't respond at the Steering Committee due to my lack of English.

@joeljfischer -san

  1. For screen transition, should the developer set ScreenUpdate to NextFunction?
  2. I understand.
    I wrote it in SDL-259, but I think there were cases where buttons were disabled by DD depending on the OEM.
    If this happens, there may be cases where operations cannot be performed at the transition screen.
    If the operation cannot be performed on the transition screen, does the HMI need to disable the button?

@MichaelCrimando
Copy link
Contributor

@Shohei-Kawano -san
No problem, I got certified in the N5 Japanese Language Proficiency Test (JLPT) which took years to get that super basic-level of understanding. I can't imagine learning English is much easier

I can answer for Joel:

  1. Correct
  2. That's why we ended up saying in SDL-259 (Theresa didn't add the comment yet) that the HMI DD would take priority for the documentation.

@theresalech
Copy link
Contributor Author

The Steering Committee voted to accept this proposal with revisions. The author will revise the proposal to specify that the HMI API doesn't use function IDs so SDL Core would communicate the next function ID through the existing RPC interface strings in the format of interfaceName.RPCMessage.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Jan 15, 2020
@theresalech theresalech changed the title [In Review] SDL 0250 - Next RPC Indication for the HMI [Accepted with Revisions] SDL 0250 - Next RPC Indication for the HMI Jan 15, 2020
@theresalech
Copy link
Contributor Author

@MichaelCrimando 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 the respective repositories for implementation. Thanks!

@theresalech
Copy link
Contributor Author

theresalech commented Jan 28, 2020

The author has updated this proposal to reflect the agreed upon revisions, and implementation issues have been 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

9 participants