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

Re-order rai logic for plugins #3526

Merged
merged 5 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,9 @@ class ApplicationManagerImpl
ApplicationSharedPtr RegisterApplication(
const std::shared_ptr<smart_objects::SmartObject>&
request_for_registration) OVERRIDE;

void FinalizeAppRegistration(ApplicationSharedPtr application,
const uint32_t connection_key) OVERRIDE;
/*
* @brief Closes application by id
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,16 @@ void RegisterAppInterfaceRequest::Run() {
SetupAppDeviceInfo(application);
auto status_notifier = AddApplicationDataToPolicy(application);

auto on_app_registered = [application](plugin_manager::RPCPlugin& plugin) {
plugin.OnApplicationEvent(plugin_manager::kApplicationRegistered,
application);
};
// To prevent timing issues, this event is called before an app is accessible
// by the applications accessor. This prevents incoming hmi rpcs from
// attempting to access an app before it has been fully initialized.
application_manager_.ApplyFunctorForEachPlugin(on_app_registered);
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case it is important to remember that when the plugin received kApplicationRegistered event, the application is not finally registered. I would add this as a comment in the kApplicationRegistered enum

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noting that this placement is the most important part of the change. This event needed to happen before an app was added to the applications accessor in the app manager.

application_manager_.FinalizeAppRegistration(application, connection_key());

std::string add_info;
const auto is_resumption_required = ApplicationDataShouldBeResumed(add_info);

Expand All @@ -762,12 +772,6 @@ void RegisterAppInterfaceRequest::Run() {
SendSubscribeCustomButtonNotification();
SendChangeRegistrationOnHMI(application);

auto on_app_registered = [application](plugin_manager::RPCPlugin& plugin) {
plugin.OnApplicationEvent(plugin_manager::kApplicationRegistered,
application);
};
application_manager_.ApplyFunctorForEachPlugin(on_app_registered);

if (is_resumption_required) {
const auto& msg_params = (*message_)[strings::msg_params];
const auto& hash_id = msg_params[strings::hash_id].asString();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,11 @@ ApplicationSharedPtr ApplicationManagerImpl::RegisterApplication(
// Timer will be started after hmi level resumption.
resume_controller().OnAppRegistrationStart(policy_app_id, device_mac);

return application;
}

void ApplicationManagerImpl::FinalizeAppRegistration(
ApplicationSharedPtr application, const uint32_t connection_key) {
AddAppToRegisteredAppList(application);

// Update cloud app information, in case any pending apps are unable to be
Expand All @@ -772,8 +777,6 @@ ApplicationSharedPtr ApplicationManagerImpl::RegisterApplication(
connection_handler::DeviceHandle secondary_device_handle = itr->second;
application->set_secondary_device(secondary_device_handle);
}

return application;
}

bool ApplicationManagerImpl::ActivateApplication(ApplicationSharedPtr app) {
Expand Down
15 changes: 15 additions & 0 deletions src/components/include/application_manager/application_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -554,9 +554,24 @@ class ApplicationManager {
*/
virtual void IviInfoUpdated(const std::string& vehicle_info, int value) = 0;

/**
* @brief Creates the application object for a newly registered application.
* This method performs initialiation of the app object by setting properties
* and starting the resumption process if applicable.
* @param request_for_registration Smart Object RegisterAppInterfaceRequest
* message received from mobile.
*/
virtual ApplicationSharedPtr RegisterApplication(
const std::shared_ptr<smart_objects::SmartObject>&
request_for_registration) = 0;
/**
* @brief Completes initialization process by adding the app to the
* applications accessor. App is now accessible by all other Core components.
* @param application ApplicationSharedPtr for newly registered application.
* @param connection_key Internal application id of registering application.
*/
virtual void FinalizeAppRegistration(ApplicationSharedPtr application,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have doxygen description

const uint32_t connection_key) = 0;

virtual void SendUpdateAppList() = 0;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ class MockApplicationManager : public application_manager::ApplicationManager {
application_manager::ApplicationSharedPtr(
const std::shared_ptr<smart_objects::SmartObject>&
request_for_registration));
MOCK_METHOD2(FinalizeAppRegistration,
void(application_manager::ApplicationSharedPtr,
const uint32_t connection_key));
MOCK_METHOD0(SendUpdateAppList, void());
MOCK_METHOD2(MarkAppsGreyOut,
void(const connection_handler::DeviceHandle handle,
Expand Down