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

Fix problem with subscription on way points during resumption #3450

Conversation

ychernysheva
Copy link
Contributor

@ychernysheva ychernysheva commented Jun 22, 2020

Fixes #2460

This PR is ready for review.

Risk

This PR makes no API changes.

Summary

This pull request is intended to fix problem with subscription on way points during resumption, when SDL does not send SubscribeWayPoints request to HMI in case of resumption. This problem is caused by absence of functionality, which may ensure the sending corresponding command to HMI during resumption.

MessageHelper class is responsible for creation messages to HMI in similar situations, and already has two in fact duplicate methods with identical functionality (SendUnsubscribedWayPoints and SendStopAudioPathThru). There is a possibility to add another duplicate method like SendSubscribedWayPoints, but better solution may be implementation one shared method for creation such type of messages (with any function ID, depending of situation).

That's why in this pull request CreateMessageWithFunctionID method is added to MessageHelper class, and methods SendUnsubscribedWayPoints and SendStopAudioPathThru are replaced by CreateMessageWithFunctionID method in all places, where they were used. Also call of CreateMessageWithFunctionID is added to method AddWayPointsSubscription of ResumeCtrlImpl class to solve problem, mentioned in corresponding issue.

CLA

There is a need in move this block of code, because deletion of data
about subscriptions before saving application data for resumption will
cause loss of data about subscription to way points. That's why logic for unsubsription
from way points needs to be located after code block, related to saving of application data for resumption.
@ychernysheva ychernysheva changed the title Fix/sdl does not send subscribe way points request during resumption Fix problem with subscription on way points during resumption Jun 22, 2020
@theresalech
Copy link
Contributor

Hi @ychernysheva, issue 2460 is not currently planned for the Core 6.2 release, but we will work to include if time allows.

* @return Shared pointer to smart object with received function ID and other
* parameters, which are needed for creation message to HMI
*/
static smart_objects::SmartObjectSPtr CreateMessageWithFunctionID(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static smart_objects::SmartObjectSPtr CreateMessageWithFunctionID(
static smart_objects::SmartObjectSPtr CreateRequestForHMI(

A small suggestion

Comment on lines 916 to +920
application_manager_.SubscribeAppForWayPoints(application);
auto message = MessageHelper::CreateMessageWithFunctionID(
application_manager_,
hmi_apis::FunctionID::Navigation_SubscribeWayPoints);
application_manager_.GetRPCService().ManageHMICommand(message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
application_manager_.SubscribeAppForWayPoints(application);
auto message = MessageHelper::CreateMessageWithFunctionID(
application_manager_,
hmi_apis::FunctionID::Navigation_SubscribeWayPoints);
application_manager_.GetRPCService().ManageHMICommand(message);
if (!application_manager_.IsAnyAppSubscribedForWayPoints())
auto message = MessageHelper::CreateMessageWithFunctionID(
application_manager_,
hmi_apis::FunctionID::Navigation_SubscribeWayPoints);
application_manager_.GetRPCService().ManageHMICommand(message);
}
application_manager_.SubscribeAppForWayPoints(application);

You should check if any apps are already subscribed before sending this message

* @brief Sends UnsubscribeWayPoints request
* @return true if UnsubscribedWayPoints is send otherwise false
*/
static bool SendUnsubscribedWayPoints(ApplicationManager& app_mngr);
DEPRECATED static bool SendUnsubscribedWayPoints(
ApplicationManager& app_mngr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These can be removed instead, due to the fact that the next release is a major release.

@ychernysheva
Copy link
Contributor Author

@jacobkeeler Unfortunately, now this PR is outdated, because fix of this defect is a part of new feature, which will be delivered as part of release 7.0. Realization of affected functionality, responsible for processing subscription on way points, will be different in planned feature, that's way there is no need to deal with this fix now. Also all related changes, which are still actual, will be moved to mentioned feature.

May I suppose to close this PR as outdated?

@jacobkeeler
Copy link
Contributor

@ychernysheva Which feature are you referring to?

@ychernysheva
Copy link
Contributor Author

@jacobkeeler This feature is implementation of proposal [SDL-0190] Handle response from HMI during resumption data (issue #2486)

@iCollin
Copy link
Collaborator

iCollin commented Sep 4, 2020

Implemented in #3475

@iCollin iCollin closed this Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants