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

[Withdrawn] SDL 0343 - Add Notifications for Required App Support #1174

Closed theresalech closed 2 years ago

theresalech commented 2 years ago

Hello SDL community,

The review of "SDL 0343 - Add Notifications for Required App Support" begins now and runs through August 24, 2021. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0343-Addition-of-a-way-to-notify-HMI-of-the-required-support-for-apps.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/1174

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

joeygrover commented 2 years ago
  1. This proposal affects all other app libraries as well. Since it is adding a new protocol message, all other platforms must at least implement the base ability to recognize such messages and therefore they will be affected. Please include iOS and JavaScript as impacted platforms.
  2. This proposal directly conflicts with an already accepted proposal from the Nexty team as well, SDL-0280. That proposal added a new param requiresAudioSupport and now this proposal is basically trying to accomplish the same goal but doing it in a new protocol message. There is no possible way we can accept both of these proposals and implement them. They interfere with each other and create a ton of redundant code and logic. The author needs to decide which they would like to support, until that time there is no reason to continue to review this proposal.
  3. This proposal is putting more logic into the router service that should be avoided. The router service is not intended to be a logic hub but instead a mere flow control of messages and physical transport management. It has bare bones logic when it comes to dealing with messages to and from Core that allow it to function but misunderstanding that and adding more logic is a dangerous precedent. The router service should not be bloated with more and more logic to have to deal with and watch for. The proposal creates a new type of message that is not addressed to a specific app but a general protocol message that has no specific app destination within the header as all other messages do. Instead, it is expected that this address-less message is open and read first, then attempted to be delivered. Nothing in the current protocol spec works like this, nor should it.
  4. The proposal uses appName as a way to identify which apps to communicate with, this is incorrect. Any app can spoof another app by using their name in these new messages. Asking the router service to identify app's based on the app name they provided through SDL will lead to issues. There is no guarantee that the app name sent by the app in their SDL registration matches that of their installed application name. App names are also not guaranteed to be unique.
  5. This proposal is bit redundant, not only from SDL-0208, but because app's have the ability to add this functionality themselves. They can register over lower bandwidth transports and inform the user themselves that USB needs to be connected. And in a similar manner, tell them bluetooth needs to be connected for audio if required.
  6. The proposal never clearly states if the app is supposed to stay up and running after they send the Request Additional Support message. Can the author clarify if they expect the app to stay open or if it is expected they close their app until a Ping Apps is received? If the app is supposed to stay open, then what happens if this Ping Apps is never received?
  7. There is no reference text as to what the HMI should state in the two "required additional support" cases, the author should provide some text even if it is only an example.
  8. Has the author considered the case where more than one app is going to request the same "additional support" and how that would be handled? Would the HMI keep a list of the received messages and make sure to create a ping messages for each one? How does the HMI pass the appName or app identifier after the user enables the required support?
  9. Has the author considered any other additional features that an app might require before registering?
  10. The author should have been able to come up with some alternatives for this proposal. For example, what about adding custom data to the app's manifest through meta data that would be easy to identify as required features instead of having that done by each app.

While I can understand where the author is attempting to go with this proposal, it simply seems like it has a long way to go to truly be flushed out. Since this is a conflicting proposal to another proposal the author or their company created it needs to be addressed which should be used if this one is ever accepted. The author needs to make sure that these ideas are well thought out and provide the best possible solution to a real problem.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for a lot of comments. However, since we should consider with internally, we take a time to reply to your comments. Please wait for a while (maybe by the end of this week). Thank you.

theresalech commented 2 years ago

The Steering Committee voted to keep this proposal in review, to allow additional time for the author to respond to the comments on the review issue.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your comments.

1. OK. We will add the iOS and JavaScript to the impacted platforms.

2. This proposal is update version of SDL-0280. In our system, since if the system does not have such a support, the the app cannot be registered, we should address before RPC RAI. Therefore, we propose to add new parameter to the Protocol Spec. We heard the predecessor that this solution is created according to your comment of SDL-0280. Is it right? If our understanding is wrong, Please teach us. Thank you.

3. We understood what you say. However, we heard the predecessor that this solution also is created according to your comment of SDL-0280. Is it right? If our recognition is wrong, could you please teach us. Thank you.

4. We assume that the appName which we use is equaled the appName used by the HMIApplication or ChangeRegistration. Aren't these unique?

5. Yes, that's right. However, the support for SDL Core and SDL HMI is required.

6. We assume that the app will close after sending the Request Additional Support message and when the app receive the Ping Apps message, it will reopen and send the Start Service message. If our thought matches yours, we will add text above.

7. How about the following sample texts.

The case to need the audioSupport. "In order to register xxxx app that require audio, a Bluetooth connection is required along with a USB connection. Please connect via Bluetooth from the setting screen of the Head Unit."

The case to need the highBandwidth. "In order to register xxxx app that require video, a USB connection is required along with a Bluetooth connection. Please connect with the Head Unit via USB."

8. We heard the predecessor to be not consideration. However, we assume that the HMI keep a list of the received messages and make sure to create a ping messages for each one. The list will include RPC name, appName attributed the RPC RequestAdditionalSupportand requiredAdditionalSupport. After the user enables the required support, the HMI pass the appName from the list.

9. We heard the predecessor that he considered any other additional features, however, he could not find out anything.

10. The predecessor assume that SDL-0280 and SDL-0280 Revise are alternative consideration. However, for SDL-0280, the BluetoothAddress cannot use and for SDL_0280 Revise, the deviceName is not unique parameter. Therefore, we described "The author was unable to consider any alternative approaches".

Your example looks like good for confirmed support such as audio and video. However, If it wants to add the app specific support, we think it is difficult to identify as required feature. Is our recognition wrong? If so, please teach us. Thank you.

joeygrover commented 2 years ago

2. I apologize on this item. I was under the impression 0280 was accepted however I see it was withdrawn and this proposal is in place of that one. Please disregard this comment.

I will need additional time to respond to the other items.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san,

2. We also think so (0280 was accepted), however, we could not find out something instead of the BluetoothAddress. Therefore, we decided to create a new proposal according to your comment. If our recognition is right, we will close this comment. Thank you.

We will wait for your comment to the other items. Thank you.

Jack-Byrne commented 2 years ago
  1. I am not sure it is a good idea to allow unregistered apps to trigger a message pop up on the HMI. This could lead to apps spamming the Request Additional Support message and pop ups constantly being displayed to the user.

  2. How would Request Additional Support and PingApps work in the case of multiple connected devices? The new notifications to/from the HMI should include some device info.

  3. How does the HMI know that the additional support issue was resolved correctly by the user (ie audio was connected)?

  4. I am not sure why PingApps notification is required. If the user connects their device over bluetooth to resolve the audio issue, why cant the app connect using the regular connection/registration process?

Akihiro-Miyazaki commented 2 years ago

@JackLivio -san, Thank you for your comments. Since we should consider with internally, we take a time to respond.

theresalech commented 2 years ago

The Steering Committee voted to keep this proposal in review, to allow for further discussion on the review issue.

joeygrover commented 2 years ago

1. Agreed to add iOS and JavaScript to the impacted platforms.

2. Disregard this original comment.

3. The comment you are linking is a vague idea where I was attempting to create some ideas. It should not be assumed to be a good solution without going through all aspects of how it would be implemented. So please respond to my point number 3 in this thread without using the linked comment.

4. appName is not guaranteed to be unique prior to an app registering where their message would also include their app ID. This point will need to be addressed.

5. Why is the support required? You have not stated a good argument that we shouldn't simply let app developers handle this logic instead of adding more changes into the protocol. Only stating that it is "required" is not a strong case to accept this proposal. Please explain why adding this is better than having app developers handle it.

6. If this is the case it needs to be added to the proposal to clearly define expected behavior.

7. These examples should be added to the proposal.

8. I'm sorry, I don't understand what you mean by "We heard the predecessor to be not consideration.", can you clarify? So does the HMI display a pop up with the messages described in point 7 for each app?

9. Ok so no other features to add to this method.

10. This does not address my concern. I actually stated an alternative solution for you in my previous comment. I would encourage the author to try and think of other alternatives to this proposal. They can help illustrate why the chosen method is better than other ways. Simply stating that there are no alternatives can signal that the author has not performed their due diligence to ensure that what they are proposing is the best approach.

Akihiro-Miyazaki commented 2 years ago

@JackLivio -san, 11. We want to make it one of the conditions for registering unregistered apps. However, as you say, spam problems are possible, so add a confirmation process on the HMI to see if the necessary functions have already been supported, and if it is supported, send PingApps without displaying comments. I think that the problem can be solved. 12. We had no consideration about this matter. Because we assume that SDL system can communicate only one phone even if the Head unit connected multi Phones. Is our recognition right? If wrong, please teach us. Thank you. 13. We assume that HMI can know on the RPC UpdateDeviceList sent from SDL Core. 14. Regarding to the registration of Media app via BT, since it has no problem to output the audio, this sequence does not excute. RequestAdditionalSupport and PingApps are not sent. On the other hand, if the registration of Media app via only USB, since it cannot output the audio, this sequence excutes.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your reply. 3. We can understand not to implement any functions to the trusted router service other than the physical transport management. We think to create a new process in the app library and perform this process before the trusted router service. 4. We understood that appName is not unique before app registering. How about the app ID? Is app ID unique before app registering? Why app ID is not unique before app registering? Also, how about file name? 5. ur recognition is below. Our previous purpose was to add BT as the communication path for audio output, if the communication path is only USB. In this time, to be expandable, we created this proposal. We targeted not only audio, but also video. We assume that your comment "redundant" means to add the video. We think these targets are same means to display the text and to prompt user operation. Our mentioned "the support required" meant that it is necessary to implement the new solution, if other additional support is required.

If our recognition is wrong, please let us know. Thank you. 6. OK. If this proposal became "Returned for Revisions", we will modify to add this description. 7. OK. If this proposal became "Returned for Revisions", we will modify to add this example. 8. We think that the displaying same comment for each app is not good for usability. It will be to display only one time. Also, we will remove the app name(xxx) in point 7. 10. How about below? SDL Core will determine wether to send the RPC to HMI by looking at the parameters of the start service request. If Start service request has s no parameter, SDL Core determine this app is Media app. If Start service request has videoProtocol, SDL Core determine this app is Navigation or Projection app. But, since the expandable of this method is low, thus if a new additional support is required, it is necessary to create a new determine process.

Jack-Byrne commented 2 years ago

11.

To mitigate spam my suggestion would be to add logic to SDL Core to only forward this message to the HMI one time per device per ignition cycle.

12.

Core has the ability to communicate with multiple apps on multiple devices at the same time.

There used to be a limitation where the same app on different devices could not register at the same time, but this limitation was lifted in SDL Core 6.0.0 with the implementation of this proposal. https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0204-same-app-from-multiple-devices.md

SDL Core does not know about the device's a2dp connection so this message would not be sent in the event a user resolves the audio issue. Core bluetooth transport only interacts with the bluetooth SPP.

14.

I misunderstood the flowchart. I thought ping apps was being sent over the bluetooth transport. This point is resolved.

theresalech commented 2 years ago

The Steering Committee voted to keep this proposal in review to allow for further discussion on the review issue.

Akihiro-Miyazaki commented 2 years ago

@JackLivio -san, 11. We understood that to add the logic to SDL Core is to reduce the sending the unnecessary HMI_API and the number of displaying the same message. We think your suggestion is effective. If our recognition is right, we will modify this proposal after returned. 12. Thank you so much for your advice. I did not know such a proposal… Anyway, we will add the parameter of device name according to your advice, after Returned. We assume that the additional parameters are deviceName and id. 13. Thank you for your information. We had remembered to be discussed this matter at SDL-0280 Proposal. At that time, we said the following. Since SDL Core does not have bluetooth feature, SDL Core will confirm the BT A2DP connection from HMI by adding the parameter of a2dpConnectionState of RPC OnDeviceStateChanged. In this time, we assume that RPC PingApps is instead of RPC OnDeviceStateChanged. But, we assume that it may add the parameter of deviceName and id.

joeygrover commented 2 years ago

Resolved

1. Agreed to add iOS and JavaScript to the impacted platforms.

2. Disregard this original comment.

6. Proposal needs to be updated to explain that after an app sends the Request Additional Support message that it is to close down and await being notified of that support being available.

7. Update proposal to include a sample message of what string would be displayed` on the HMI.

The case to need the audioSupport. "In order to register additional apps that require audio, a Bluetooth connection is required along with a USB connection. Please connect via Bluetooth from the setting screen of the Head Unit."

The case to need the highBandwidth. "In order to register additional apps that require video, a USB connection is required along with a Bluetooth connection. Please connect with the Head Unit via USB."

8. Update proposal to state that only a single prompt per support type request should be presented.

9. No other features to be added at this time.

Open

3. Can you explain further? This is exceptionally vague and does not provide any context of how you would accomplish the goal of this proposal with this moved logic.

4. App ID is potentially unique but how do you tie that together with what app to send the intent? That ID is not accessible to other apps and nor should it be. What do you mean the file name? Apps do not have access to file names on Android.

5. This is not correct. I have stated that the app developer currently has the option to display their app when not all features are met. Meaning when the user attempts to start the app the developers can provide a message giving an indication of what is required so that the user can take action and the app can then be fully useable. It can do this in both cases of needed audio and video. So my request is that you provide a good, well detailed explanation of why this solution is better than what developers can do today. We have to ensure changes we make to the project are worth doing; creating solutions and finding problems is not our goal.

10. In your example why is expandability low of that method? Including params in the StartService message is something we have done in the past and Core could seemingly reject the app if the feature is not present. I am pushing for alternative solutions to try and help strengthen the case for one solution over another since I believe this could be achieved in a number of different ways.

Jack-Byrne commented 2 years ago

Resolved:

  1. Ok I agree for this revision

  2. OK I agree to this revision

  3. Uses same revision specified in point 12.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your comments. Regarding to 3/4/10, since we should consider with internally, we take a time to respond. 5.

I have stated that the app developer currently has the option to display their app when not all features are met. Meaning when the user attempts to start the app the developers can provide a message giving an indication of what is required so that the user can take action and the app can then be fully useable.

Does the app have a feature to display their app when not all features are met? If you know that a proposal about this matter have already issued, could you please let us know the URL. If it is correct, we think it is not necessary this proposal.

But, we have a quetion. How do unregistered apps notify Core and HMI of missing features?

joeygrover commented 2 years ago

5.

Yes. There are currently options in the library that allow developers to require specific support (audio arbitration and high bandwidth transport). When the developers set these values to required, the library automatically does not register with the head unit.

If an application sets those values to not required, even though their main functionality does require it, they will be registered with the head unit. Upon an HMI_FULL status, the application can send an Alert or updated text fields that displays the need for additional support. They stay registered and connected to the IVI while they wait for the support they need.

For audio, the application can listen for bluetooth events already and receive an intent when the A2DP profile connects at which point they can begin their full functionality.

For high bandwidth, they can use callbacks in the library that allow them to listen for additional transport connections. At which point they can start their video service over that newly connected transport.

The main point here is that the app developers have to specifically add this logic into their app. That way they handle the situation exactly how they would like to. Going this route will also prevents multiple requests for the same support happening all at once.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your reply. 5. It is necessary for us to know more details. Please let us know where the details are written.

Also, we think that you did not answer our question.

How do unregistered apps notify Core and HMI of missing features?

Does that mean it depends on the implementation of app? Please let us know. Thank you.

theresalech commented 2 years ago

During the 2021-09-14 Steering Committee meeting, the Steering Committee voted to keep this proposal in review to allow for further discussion on the review issue.

joeygrover commented 2 years ago

5.

This isn't explicitly written down; these actions have always been available to developers. They use a combination of SDL and Android specific operations to achieve these. If the desire out of this review is to include more detailed instructions to developers I think that can be done and a page added to the developer portal explaining how to achieve each.

The app sets their respective flags to false

//For Audio
MultiplexTransportConfig.setRequiresAudioSupport(false);

//For Video
MultiplexTransportConfig.setRequiresHighBandwidth(false);

Then in the onStart method of the SdlManagerListener callback they would check if those options are available

For Audio

For Audio, the app developer would check the audio arbitration status and if not available register a broadcast receiver with the intent filter that included the action ACTION_CONNECTION_STATE_CHANGED. When that broadcast receiver was triggered, they would start up their normal integration from the SdlService.

MediaStreamingStatus mediaStreamingStatus = new MediaStreamingStatus(SdlService.this.getBaseContext(), new MediaStreamingStatus.Callback() {
    @Override
    public void onAudioNoLongerAvailable() {
        //Display alert to user requesting they connect audio if in HMI Full state
    }
});
if(mediaStreamingStatus.isAudioOutputAvailable()){{
    //Continue as normal 
} else {
    //Display alert to user requesting they connect audio if in HMI Full state
    //Register broadcast receiver to listen for potential Audio connections. This is standard Android behavior so it is not provided here.
}
For Video

For video, the developer would add a transport listener to the transport config instance. This would let them know that a potential transport is available for video streaming

MultiplexTransportConfig.setTransportListener(new MultiplexTransportConfig.TransportListener() {
    @Override
    public void onTransportEvent(List<TransportRecord> connectedTransports, boolean audioStreamTransportAvail, boolean videoStreamTransportAvail) {
        if(videoStreamTransportAvail) {
            //Continue as normal 
        } else {
            //Display alert to user requesting they connect USB or TCP if in HMI Full state
        }
    }
});
Additional Questions

Also, we think that you did not answer our question.

How do unregistered apps notify Core and HMI of missing features?

Per my previous comment:

[Apps] stay registered and connected to the IVI while they wait for the support they need.

This means they do not unregister so there is no need to inform the IVI directly. Again, the developer pushes the Alert when they are activated by the user.

Does that mean it depends on the implementation of app?

Yes. Please see the last paragraph in my previous comment.

The main point here is that the app developers have to specifically add this logic into their app. That way they handle the situation exactly how they would like to.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your information. 5. Our understanding about your suggestion(have already implemented on the app) is following.

Is it correct? If our understanding is wrong, please let us know more details.

Although we need to discuss internally, but if your suggestion is enough against of what we want to do, we may be withdrawn this proposal.

Thank you.

joeygrover commented 2 years ago

For audio I think that's right.

For video that is not correct. There is no message to inform the IVI of the need for a video path. The app, just like in the audio scenario, will register over the connected transport (in this case BT), stay registered, user selects the app, the app sees that USB is not available and sends an Alert to the IVI. When USB is connected that callback will be triggered and they can start their normal operation.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your detail description. We understood. Also, we discussed this matter internally.

We confirmed that the behavior of your suggestion is enough against our proposal. However, as a result, we cannot accepted to your suggestion. Your suggestion needs to implement to all apps. Please let us know how many corresponded apps does the SDL have, if you know? Also, the difference between apps corresponded this one or not is very big.

Do you have any idea?

theresalech commented 2 years ago

The Steering Committee voted to keep this proposal in review to allow for further discussion on the review issue.

joeygrover commented 2 years ago

5. It is true each app that wants this behavior would be required to implement the logic as previously stated. It is possible we add additional logic into the library to make this easier to use and therefore reduce the burden on developers; this would be pretty easy. Please note that not all apps need this behavior so it would not be required for all apps.

The argument of all apps would need to implement this is somewhat irrelevant, as the changes described in this proposal would also require developers to implement the latest version of the app libraries so it would not be automatic. Adding this feature as stated in the proposal also requires that the IVI system be updated to the version of Core and HMI that support such feature whereas what is available today will work with all IVI system's on the road. So if the argument against the solution that exists today is that app developers need to implement it, then the argument against the proposal could easily be that Core developers (IVIs) would be required to implement it. Therefore the argument itself is unconvincing when used for either position.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Thank you for your comment. 5. It is necessary to talk about this matter internally. Please wait for a while. Thank you.

theresalech commented 2 years ago

The Steering Committee voted to keep this proposal in review to allow for further discussion on the review issue. It was noted that the author needs to respond to the following open items: 3, 4, 5, and 10.

Akihiro-Miyazaki commented 2 years ago

@joeygrover -san, Sorry for late response.

We recognize the current status is following. Our proposal:

Joey's suggestion1:

Joey's suggestion2:

These plans are necessary to perform the test by app. Thus, whichever we will choose the one of them, we assume that it is necessary the support of app partners. Also, Joey's suggestion1 is already implemented all platforms except app.

As the result of discussion internally with the consideration above, since we want to depend all on the app partners, we decided to choose Joey's suggestion1 and to drop our proposal.

theresalech commented 2 years ago

This proposal has been withdrawn per the author's request.