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

Add CloseApplication RPC #2948

Merged
merged 4 commits into from
Jul 16, 2019
Merged

Add CloseApplication RPC #2948

merged 4 commits into from
Jul 16, 2019

Conversation

jacobkeeler
Copy link
Contributor

Implements #1931

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Create base ATF tests for new RPC

Summary

Add new CloseApplication RPC to Mobile and HMI interfaces.

Changelog

Enhancements
  • Add new CloseApplication RPC to Mobile and HMI interfaces.

CLA

@jacobkeeler jacobkeeler force-pushed the feature/close_application_rpc branch from 96c201d to d8d34c8 Compare June 17, 2019 15:23
@jacobkeeler jacobkeeler force-pushed the feature/close_application_rpc branch from d8d34c8 to bd5b5d7 Compare June 17, 2019 19:41
@LuxoftAKutsan
Copy link
Contributor

@jacobkeeler This PR contains additional Request\Resoponse to HMI.
There was clarification with proposal author, that for Mobile Close RPC can be used existing ActivateApp to None .
smartdevicelink/sdl_requirements#111
@kshala-ford could you please provide any information about it?

@kshala-ford
Copy link

@LuxoftAKutsan does this PR follow option 2 of smartdevicelink/sdl_requirements#111 ?

@jacobkeeler
Copy link
Contributor Author

jacobkeeler commented Jun 19, 2019

@kshala-ford It would appear so.

@LuxoftAKutsan My only reservation with using BC.ActivateApp for this purpose is that it seems like an improper use of the RPC. The app is not being activated, it is being deactivated. I would think that the level parameter of BC.ActivateApp is really only meant to be filled with LIMITED or BACKGROUND in specific cases, NONE doesn't make much sense in the context of this message since the description states that the app should be brought to the foreground. It would technically work as it is implemented in the sdl_hmi currently, but the generic_hmi doesn't have this sort of logic because it isn't really described in the HMI integration guidelines.

@LuxoftAKutsan
Copy link
Contributor

@kshala-ford this PR follow option 2 from smartdevicelink/sdl_requirements#111.

@jacobkeeler From my perspective Adding CloseApplication RPC to HMI API will add confusion.
We will have :

  • BC.ActiveteApp (with possible HMI level NONE and background, and as I remember SDL use int in some cases)
  • CloseApplicaiton

Now we have 2 options :

  • Add explicit description that NONE, BACKGROUND should not be used in ActivateApp and add new CloseApplication RPC. Look throw existing sequenses and replace sending ActivateApp[NONE] to CloseApplication (I suppose that one of the use cases is revoke applicaiton).

  • Avoid adding additional RPC CloseApplication, and use ActivateApp[NONE] instead of it. Add apropriate implementation to generic_hmi.

Also it is not clear for me difference between sdl_hmi and generic_hmi. Why do we need both in opensource?

@jacobkeeler
Copy link
Contributor Author

@LuxoftAKutsan I believe that the documentation would need to be updated in either case, because the HMI documentation doesn't describe the HMI's behavior for each HMI level. As stated, it doesn't seem like ActivateApp(NONE) is a valid case as it is currently described:

<description>Request from SDL to HMI to bring specified application to front on UI e.g make it HMI status 'FULL'.</description>

In addition, it seems to me that HMILevel is generally used as a Mobile<->Core concept rather than a Core<->HMI concept. BC.ActivateApp is the only message which includes this enum in the entire HMI API, so it appears to me that it might have been added after the RPC was initially implemented to fix a specific issue.

The generic_hmi is generally preferred as a reference implementation for SDL, but it is missing several components at the moment (such as Navigation). Being a reference implementation, it is not meant to imitate a full infotainment system, so components such as VehicleInfo will likely never be a part of the generic_hmi. The sdl_hmi is still available specifically as an emulator for a full infotainment system, including the components which cannot be included in the scope of the generic_hmi.

Copy link
Contributor

@LuxoftAKutsan LuxoftAKutsan left a comment

Choose a reason for hiding this comment

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

Not sure, but I suppose that merging this PR will affect existing test scripts ( Smoke and delivered features).
Existing scripts probably will expect ActivateApp(None) in case if the application will be deactivated.

MessageHelper::HMIToMobileResult(hmi_result);
bool success = PrepareResultForMobileResponse(
hmi_result, HmiInterfaces::HMI_INTERFACE_AppService);
if (success) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should part of the success be to check if the HMI also sent an OnAppDeactivated? Before setting the app state to HMI_LEVEL none, should the state be checked to see if it is in limited or background?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think OnExitApplication would be more appropriate, but I don't think it's necessary for the HMI to send this message, the response should be enough.

(*bc_activate_app_request)[strings::params][strings::correlation_id]
.asInt();
return corr_id;
const uint32_t corr_id =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the change to uint32 and then recast to int64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems this was originally chosen for the return value of SendBCActivateApp to allow for an error response of -1, correlation IDs use uint32_t. I could remove these casts, but it was being done implicitly before anyway.

@jacobkeeler jacobkeeler force-pushed the feature/close_application_rpc branch from e731952 to ca57f92 Compare July 11, 2019 18:55
@jacobkeeler jacobkeeler merged commit 5e6ea2c into develop Jul 16, 2019
@jacobkeeler jacobkeeler deleted the feature/close_application_rpc branch February 19, 2020 20:57
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