[Deferred] SDL 0291 - Allow navigation apps to access information about WiFi networks

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0291 - Allow navigation apps to access information about WiFi networks" begins now and runs through September 1, 2020. Previous reviews of this proposal took place March 11- 17, 2020 and June 10 - 23, 2020. The original review took place March 11 - 17, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0291-allows-navigation-apps-to-access-information-about-Wi-Fi-networks.md

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

#965

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

[joeygrover]

To fix these issues, navigation apps need to access information about WiFi networks.

The proposal is merely stating we need to require a permission without stating how it will be used. This proposal should be return for revisions and reworked to frame the problem they are trying to solve with the solution that would require the new permission. This would then allow the SDLC to better understand the solution and weigh the merits of a new permission requirement.

[theresalech]

The Steering Committee voted to return this proposal for revisions. The author will update the proposal to describe the problem they're trying to solve with the solution that would require the new permission android.permission.ACCESS_WIFI_STATE.

[theresalech]

The author has revised this proposal to reflect the agreed upon revisions, and the revised proposal is now in review until 2020-06-16.

[joeygrover]

I apologize for my last minute review of the updated proposal, hopefully we can discuss on the call or allow more time for review as well.

1. The general idea appears to be that in order to connect over the WiFi secondary transport it is sometimes possible to miss the timing over when the mobile device connects to the IVI system. Because this is more general than only the video streaming service, doesn't it make sense to move this logic into something closer to the protocol layer? Or possibly lifecycle?

2. Creating two different public API's for start() is very ambiguous and unintuitive to a developer. Pending the previous point, it would make more sense to include the context as a new constructor, deprecate the current one, and then in the SDL Manager have it supply that manager with it. Again, I think this will be pending if we truly want to keep the broadcast receiver in that class or not.

3. In the supplied flow chart it shows that onTransportEventUpdate doesn't happen until the mobile phone connects over wifi. If this is the case, then can't we use that as the trigger event to retry starting the video service? Essentially, upon receiving this packet, the sub-managers could be notified that the potential transport is now available and then go through the process of starting the service which would automatically connect the transport, then start the service.

[theresalech]

The Steering Committee voted to keep this proposal in review to allow the author time to respond to the comment provided, and fur further discussion on the review issue.

[zhouxin627]

Hi @joeygrover, thank you for your reply.

1. The general idea appears to be that in order to connect over the WiFi secondary transport it is sometimes possible to miss the timing over when the mobile device connects to the IVI system. Because this is more general than only the video streaming service, doesn't it make sense to move this logic into something closer to the protocol layer? Or possibly lifecycle?

Sounds great! we also suppose that it make sense to move this logic into transport layer which is more reasonable from the perspective of design.

2. Creating two different public API's for start() is very ambiguous and unintuitive to a developer. Pending the previous point, it would make more sense to include the context as a new constructor, deprecate the current one, and then in the SDL Manager have it supply that manager with it. Again, I think this will be pending if we truly want to keep the broadcast receiver in that class or not.

As question 1, if we move the logic of receiving broadcast to the transport layer, start() doesn't need to be changed.

3. In the supplied flow chart it shows that onTransportEventUpdate doesn't happen until the mobile phone connects over wifi. If this is the case, then can't we use that as the trigger event to retry starting the video service? Essentially, upon receiving this packet, the sub-managers could be notified that the potential transport is now available and then go through the process of starting the service which would automatically connect the transport, then start the service.

I see what you mean, but except for that trigger the onTransportEventUpdate, initiate a request for a TCP connection, there are also usecases like this:

If the HU and mobile app will communicate through the router. The HU is successfully connected to the router, and then sends the onTransportEventUpdate to the mobile app.
But the mobile phone is not connected to the router yet. After the mobile phone is connected to the router, it will receive a successful WiFi connection, a TCP connection request also needs to be initiated.

[joeygrover]

1. & 2. 👍

3. I'm a little confused on the response here. Are you saying if the IVI system connects to the router service through bluetooth or USB for example, the onTransportEventUpdate packet is sent right away? I believe the app must bind to the router service and successfully start the RPC service at least. I was simply going off the flow chart provided that stated only have the WiFi network had been joined would the onTransportEventUpdate be sent. If this is not the case though, then I would say this comment is void and the revisions should reflect the agreement to point 1.

[zhouxin627]

3. Yes, in this use case, the onTransportEventUpdate packet is sent before the mobile phone connects to the router.
   I will reflect the agreement to point 1 in the revisions.

[theresalech]

The Steering Committee voted to return this proposal for revisions. The author will update the proposal to reflect item 1 from the comments: moving the logic to the transport layer. It was noted that no further action is required for items 2 (void based on item 1) and 3 (a misunderstanding of the proposal).

[theresalech]

Closing as inactive. The issue will remain in a returned for revisions state and unlocked so the author (@zhouxin627) or any other interested party can notify the Project Maintainer when it is ready to be revisited.

[theresalech]

The author has updated this proposal to reflect the agreed upon revisions. The revised proposal will now be under review until September 1, 2020.

[theresalech]

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

[zhouxin627]

Hi @joeygrover, thank you for your comment.

1. I think this is only useful if the app type is navigation or projection at the moment. I don't think any other apps need to connect over WiFi so listing for these events should be a toggle.

Sounds great! We will add a boolean value in MultiplexTranportConfig, and if the app type is navigation or projection, set it to true.

2. I don't want to perform a full code review in this process and think we should leave a note saying that code provided is for reference but may or may not reflect accepted solution.

Ok. We will add a note.
The codes provided are all for fixing the problem if we need to consider that HS and IVI are connected to a third-party router. And it exits limit which is the IVI can not detect the WiFi of HS is disconnected.

For example, I see that in the SdlProtocolBase code changes you check if wifi is enabled or not before stating if a secondary transport is available. This is incorrect. It only needs to be checked in the event the only secondary transport available is the WiFi TCP connection. It is possible that USB is a secondary transport.

The origin code is else if (secondaryTransportParams != null && secondaryTransportParams.containsKey(supportedSecondary))
and the secondaryTransportParams is only put when IVI update IP and port. So we think only when USB is connected, the judgment is true and USB can set as secondary transport.

And I'm not sure why you are clearing listener lists, but again I think that is better suited for the actual code review of the implementation.

I'm very sorry the code belongs to the old scheme and we will delete it.

3. You are making the assumption that if a TCP disconnect happens that the IP and Port are no longer valid. However, I do not see any requirements that state this is the case. I believe the library should assume the IP and Port are valid until Core sends out another transport event update that includes new IP and port information.

We think if TCP disconnected, isSecondaryTransportAvailable should be false in this case. And if we do not remove secondaryTransportParams, isSecondaryTransportAvailable is always true when disconnect WiFi of HU
from a third-party router or mobile devices hotspot and reconnect it. Then videoStreamTransportAvail in VideoStreamManager.onTransportUpdate is always true. We can not distinguish them.

4. It seems that you have added the case when the mobile device creates an access point as a trigger. I'm a little worried that this is not going to solve the problem as it should. If the mobile device creates the access point, the IVI system will need time to connect to that mobile devices hotspot. Otherwise, the mobile library will try to connect to the supplied IP and port which will result in failure because the IVI system has not joined the network yet. Offhand I'm not sure if Android has any intents sent out when a new device joins the mobile device's network but that would be ideal.

The AP broadcast is not a trigger. We add the WIFI_AP_STATE_CHANGED_ACTION to judge isSecondaryTransportAvailable. If we do not add it, isSecondaryTransportAvailable will always false when using mobile devices hotspot because we add transportManager.isWifiConnected() in this method. And there is no need to use it as a trigger because IVI will send
TransportEventUpdate every time connected to mobile devices hotspot.

[joeygrover]

3. So what happens if the IVI never sends another TransportEventUpdate? If the IVI system assumes the same IP and port are available for the app to connect to then why would it send another TransportEventUpdate? My concern here is that this is potentially adding a requirement that on TCP disconnect, the IVI MUST send another TransportEventUpdate if the app is to ever connect over TCP again. I could be mistaken, but I do not believe this is how Core works today. Maybe @jack can assist here.

The library should still try to connect to the older IP and port, then even if it does fail, the library would get a new TransportEventUpdate that should cause the library to try again.

4. Then the code is still making the assumption that the IVI has connected to the mobile AP at the time of attempting to established the TCP connection. At which point, if the IVI isn't connected the library will still fail. I'm nervous we are simply kicking the can instead of coming up with a solid solution.

5. Is there any similar behavior for iOS? This might be causing some diverting behavior between the two platforms which is not ideal.

[zhouxin627]

We will respond to open items after internal discussion. Request you to please keep this proposal in review until next week. Thank you.

[theresalech]

The Steering Committee voted to keep this proposal in review, per the author’s request, to allow time for the author to respond to the comments posted on the review issue.

[zhouxin627]

Hi @joeygrover, thank you for your comment.

3. In our IVI system, if TCP disconnect, it will notify IP and port are "" and send TransportEventUpdate when connected again. So we didn't think about this kind of case. Maybe we can add param in onTransportUpdate to distinguish Connect/Disconnect or modify core logic?

4. I'm sorry, but I can not understand this case. I think only when IVI has connected to the mobile AP, IP and Port will be sent by TransportEventUpdate. At other times, isSecondaryTransportAvailable is false and can not start a connection.

5. Yes, we test IOS before, and the problem Turn off the Wi-Fi on the phone, then turn on the Wi-Fi on the phone, video streaming can not continue sdl_ios#1479 and If the application is switched to the background and then to the foreground, video streaming timeout occurs. sdl_ios#1471 have been fixed.

[joeygrover]

3. As of right now we can't clear out the IP and port as suggested in the proposal. Basically this is how it currently works with the open source Core and HMI and we have to adhere to this behavior:

Prerequisites:

• Mobile and Core are connected to WiFi network.
• Mobile connects to core with primary transport (BT or USB).
• Core sends IP/Port in TransportEventUpdate for first time.
• Assume mobile phone then connects TCP as secondary transport.

Scenarios:

1. TCP socket connection is terminated from Mobile but still connected to WiFi network, IP and port are still valid.
2. TCP socket connection is terminated from Core but still connected to WiFi network, IP and port are still valid.
3. Mobile phone disconnects from WiFi network, also ending TCP socket connection, IP and port are still valid.
4. Core disconnects from WiFi network, also ending TCP socket connection, IP and port are NO longer valid and core sends TransportEventUpdate packet with IP="" port=0.

4. Ok, Talking with Core team it does seem if the IP does change on the Core side it will send a TransportEventUpdate, so I think this case is actually ok.

5. I'm not exactly sure how these relate so I will need some time to review.

[zhouxin627]

3. We need some time for internal discussion. Request you to please keep this proposal in review until next week. Thank you.

4 & 5. 👍

[theresalech]

The Steering Committee voted to keep this proposal in review, per the author’s request, to allow time for the author to respond to the comments posted on the review issue.

[zhouxin627]

3. OK, We will do not remove TCP when TransportDisconnected. And add judgement in onTransportUpdate(VideoStreamManager.java) to distinguish connect/disconnect.

[joeljfischer]

5. Hello @zhouxin627, I have reviewed the sdl_ios issues you mentioned and the PR that fixed them, however, I don't see how the fix there connects to the fix applied here, even though the issues mentioned are the same. Is there a particular reason why the changes applied there were deemed unsuitable to fix the same issues in the java_suite project?

The changes applied there resolved a few race conditions and other bugs that were causing the issues to appear on the iOS project. I have not done a deep-dive on the java_suite project to see if those fixes may be applied there, but I assume that you have and deemed them unsuitable. I just want to make sure that before we make these changes that would diverge the two projects, we have a full understanding of the differences between the platforms and why different fixes were required. Thanks!

[zhouxin627]

We will respond to open items after internal discussion. Request you to please keep this proposal in review until next week. Thank you.

[theresalech]

Per the author's request, the Steering Committee voted to keep this proposal in review to allow for further discussion on the review issue.

[zhouxin627]

5. I am sorry that there was a misunderstanding in the previous response. Strictly speaking, IOS doesn't have similar behavior. The sdl_ios issues I mentioned earlier are specific to IOS, and the root causes are different from those in this proposal.

[joeljfischer]

@zhouxin627 Could you please explain how the issues are different, and, as I requested, why this behavior change is necessary when it wasn't on iOS in more technical detail? Thank you.

[zhouxin627]

@joeljfischer OK. I will add some explanation in more technical detail as you request. Request you to please keep this proposal in review until next week. Thank you.

[theresalech]

Per the author's request, the Steering Committee voted to keep this proposal in review, to allow time for the author to respond to the remaining open item in the comments.

[theresalech]

During the 2020-10-06 Steering Committee Meeting, the proposal's author committed to responding to the comments on this review issue before our next meeting on 2020-10-13. The Steering Committee then voted to keep this proposal in review for another week, instead of deferring due to inactivity, as is allowed by this section of the SDL Evolution Process document.

[Shohei-Kawano]

@joeljfischer -san
5. Because the app must be in the background when iOS connect/disconnect to Wi-Fi, the essence of the IOS problem is that proxy failed to startVideoSession again when app from the background to the foreground.
The logic of Android and IOS are different, and Android does not care whether the app is in the background.

[joeljfischer]

5. It is not true that the iOS app must be in the background to connect or disconnect from Wi-Fi. The user could walk away from the Wi-Fi access point and leave the range and then come back into range. I also don't know what you mean by "Android doesn't care whether the app is in the background." Again, I must ask for the technical details of why this solution is necessary on Android when it was not on iOS.

[theresalech]

Per the author's request, the Steering Committee voted to keep this proposal in review. The author will discuss the latest comment from the PM internally and respond on the review issue before the next meeting.

[Shohei-Kawano]

@theresalech -san,
I'm sorry, we're taking a long time to confirm the internally.
Let me decide whether to make it "In Review" or "Defer" based on the situation before the meeting.

[Shohei-Kawano]

@theresalech -san,
We need to more time for confirming internally.
I think it is not good to hold a slot, so please defer this proposal.
When our confirmation is complete, I will request a review resume.

[theresalech]

Per the author's request, the Steering Committee voted to defer this proposal until the author can meet internally and respond to the open item (Item 5) on the review issue. The author should email SDLC@smartdevicelink.com when they are able to respond to the comment, so the Steering Committee can vote on bringing the proposal back in review.

[theresalech]

Closing as inactive. The issue will remain in a deferred state. The author can email SDLC@smartdevicelink.com when they are able to respond to the open item in the comments, and allow the Steering Committee to vote on bringing the proposal back in review.