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

Start the new PTU after a failed retry sequence #3208

Merged
merged 12 commits into from
Feb 28, 2020
Merged

Start the new PTU after a failed retry sequence #3208

merged 12 commits into from
Feb 28, 2020

Conversation

ydementieiev
Copy link
Contributor

@ydementieiev ydementieiev commented Jan 13, 2020

Fixes #3136

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Summary

SDL should start the new PTU after a failed retry sequence in case some trigger occurs during the retry sequence.
Provided changes allow to perform the following actions:

  • Finish failed update with SDL.OnStatusUpdate(UPDATE_NEEDED);
  • Send SDL.OnStatusUpdate(UPDATE_NEEDED) and request PTU for applications which were added during the previous update;
  • Skip PTU for application which was registered during application list update timer;
  • Skip PTU for application with the same policy_id;

CLA

@AKalinich-Luxoft
Copy link
Contributor

@ydementieiev please sign CLA

@@ -1586,6 +1586,8 @@ class ApplicationManagerImpl

uint32_t apps_size_;

bool is_registered_in_timeout_;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev suggest to use atomic bool as timer might be in a data race with another thread

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#ifndef EXTERNAL_PROPRIETARY_MODE
void OnPTUTimeOut() OVERRIDE;
#endif
void OnPTUTimeOut();
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev OVERRIDE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AKalinich-Luxoft EXTERNAL_PROPRIETARY_MODE build failed with OVERRIDE

Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev then add pure virtual OnPTUTimeOut into interface class for EXTERNAL_PROPRIETARY_MODE

/**
* @brief Application queue ready for PTU
*/
std::vector<uint32_t> queue_applications_for_ptu_;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev i think queue will be better container for your purposes

Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev or even set as you want to keep only unique app id in your container

Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev i think this container might need access synchronization

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AKalinich-Luxoft used set and add AutoLock for work with this container in fe7b393

* with the same app_id
* @param new_app_id app id new application
*/
void AddNewApplicationIdToPTUQueue(const uint32_t new_app_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev can be renamed to PushAppIdToQueue and PopAppIdFromQueue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -2205,6 +2233,15 @@ void PolicyHandler::OnAppsSearchCompleted(const bool trigger_ptu) {
policy_manager_->OnAppsSearchCompleted(trigger_ptu);
}

void PolicyHandler::OnAddedNewApplicationToAppList(
const uint32_t new_app_id, const std::string policy_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev nice to have auto trace here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -524,6 +524,12 @@ class PolicyManager : public usage_statistics::StatisticsManager,
*/
virtual void OnAppsSearchCompleted(const bool trigger_ptu) = 0;

/**
* @brief Change applicatios count ready for PTU
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -608,6 +608,12 @@ class PolicyManagerImpl : public PolicyManager {
*/
void OnAppsSearchCompleted(const bool trigger_ptu) OVERRIDE;

/**
* @brief Change applicatios count ready for PTU
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1096,6 +1102,11 @@ class PolicyManagerImpl : public PolicyManager {
bool IsPTValid(std::shared_ptr<policy_table::Table> policy_table,
policy_table::PolicyTableType type) const;

/**
* @brief Check that application for PTU more than zero
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev

Suggested change
* @brief Check that application for PTU more than zero
* @brief Check that new applications for PTU were registered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* @brief Count application ready for PTU
*/
uint32_t count_application_;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev

Suggested change
uint32_t count_application_;
uint32_t applications_count_;

Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev @AKalinich-Luxoft i think name applications_pending_ptu_count_ would be more descriptive. Also, the doxygen description should be fixed accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -86,6 +86,11 @@ void UpdateStatusManager::OnUpdateSentOut(uint32_t update_timeout) {
ProcessEvent(kOnUpdateSentOut);
}

void UpdateStatusManager::OnUpdatePostponed() {
LOG4CXX_AUTO_TRACE(logger_);
ProcessEvent(kPostponedUpdate);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev consider using of SetPostponedStatus - maybe you can use it instead of kPostponedUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AKalinich-Luxoft I tried using SetPostponedStatus but always find a new regression after. Now I renamed OnUpdatePostponed to name which more describe what this method do, look please

* @param policy_id policy_id for this application
*/
void OnAddedNewApplicationToAppList(const uint32_t new_app_id,
const std::string policy_id) OVERRIDE;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -410,6 +410,14 @@ class PolicyHandlerInterface : public VehicleDataItemProvider {
*/
virtual void OnAppsSearchCompleted(const bool trigger_ptu) = 0;

/**
* @brief Notify that new applocation was added to application list
Copy link
Contributor

Choose a reason for hiding this comment

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

applocation typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* @brief Count application ready for PTU
*/
uint32_t count_application_;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev @AKalinich-Luxoft i think name applications_pending_ptu_count_ would be more descriptive. Also, the doxygen description should be fixed accordingly

@@ -550,6 +555,10 @@ void PolicyManagerImpl::OnPTUFinished(const PtProcessingResult ptu_result) {

update_status_manager_.OnValidUpdateReceived();

if (HasApplicationForPTU()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this condition independent from ptu_result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mked-luxoft No, no matter what the ptu_result will be from last update, we need set status to UPDATE_NEEDE for next ptu

/**
* @brief Check that application for PTU more than zero
*/
bool HasApplicationForPTU();
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this method can be const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

, trigger_ptu_(false) {}
, trigger_ptu_(false)
, ptu_requested_(false)
, last_registered_app_id_("") {}
Copy link
Contributor

Choose a reason for hiding this comment

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

is it policy_app_id? if so, then i think that the variable's name should be changed accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -116,6 +118,10 @@ void UpdateStatusManager::OnNewApplicationAdded(const DeviceConsent consent) {
return;
}
app_registered_from_non_consented_device_ = false;
if (last_update_was_failed_) {
last_update_was_failed_ = false;
ProcessEvent(kPostponedUpdate);
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure that we still need to call ProcessEvent(kOnNewAppRegistered); if we enter this condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mked-luxoft ProcessEvent(kPostponedUpdate) added for extra call UPDATE_NEEDED for new ptu if last was finished with the same status. I tried remove ProcessEvent(kOnNewAppRegistered) but it entailed a lot of regressions

/**
* @brief Application queue ready for PTU
*/
std::vector<uint32_t> queue_applications_for_ptu_;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev i think this container might need access synchronization

@@ -598,13 +598,13 @@ class PolicyHandler : public PolicyHandlerInterface,
* @param new_app_id app_id for this application
* @param policy_id policy_id for this application
*/
void OnAddedNewApplicationToAppList(const uint32_t new_app_id,
const std::string policy_id) OVERRIDE;
void OnAddedNewApplicationToAppList(const uint32_t& new_app_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev there is no need to pass int as a reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

queue_applications_for_ptu_.size());
}
sync_primitives::AutoLock lock(app_id_queue_lock_);
queue_applications_for_ptu_.insert(app_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev check for return result of insert operation. If the same app_id is inserted, count will not be changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -94,7 +94,7 @@ class UpdateStatusManager {
/**
* @brief Postponed update for PTU
*/
void OnUpdatePostponed();
void OnUpdateForNextInQueue();
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev please update description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#ifndef EXTERNAL_PROPRIETARY_MODE
void OnPTUTimeOut() OVERRIDE;
#endif
void OnPTUTimeOut();
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev then add pure virtual OnPTUTimeOut into interface class for EXTERNAL_PROPRIETARY_MODE

queue_applications_for_ptu_.insert(app_id);
policy_manager_->OnChangeApplicationCount(queue_applications_for_ptu_.size());
std::pair<std::set<uint32_t>::iterator, bool> result;
result = queue_applications_for_ptu_.insert(app_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev just

const auto result = queue_applications_for_ptu_.insert(app_id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -660,6 +660,8 @@ class PolicyHandlerInterface : public VehicleDataItemProvider {
const std::string& policy_app_id,
const std::string& hmi_level) = 0;

virtual void OnPTUTimeOut() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev please add a description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -183,9 +183,11 @@ class PolicyHandler : public PolicyHandlerInterface,
const std::string& policy_app_id,
const std::string& hmi_level) OVERRIDE;

#ifndef EXTERNAL_PROPRIETARY_MODE
/**
* @brief OnPTUTimeOut the callback which signals if PTU timeout occured
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev Suggested changes here:

  • the callback which signals if PTU timeout occured -> the callback which is performed when PTU timeout occurred
  • Move description to iface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AByzhynar update description c04c115ce

@@ -422,6 +424,18 @@ class PolicyHandler : public PolicyHandlerInterface,
*/
uint32_t GetAppIdForSending() const OVERRIDE;

/**
* @brief Add application to PTU queue if don't have application
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev don't have application ... -> no application with the same app id exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AByzhynar update description c04c115ce

void PushAppIdToQueue(const uint32_t new_app_id);

/**
* @brief Remove first from queue application
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev Remove the first application from applications queue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AByzhynar update description c04c115ce

* @param new_app_id app_id for this application
* @param policy_id policy_id for this application
*/
void OnAddedNewApplicationToAppList(const uint32_t new_app_id,
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev Move description to iface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AByzhynar leaved description only in iface c04c115ce

const std::string& policy_id) OVERRIDE;

/**
* @brief Application queue ready for PTU
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev What does it mean queue ready for PTU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AByzhynar update description c04c115ce

@@ -193,6 +193,7 @@ ApplicationManagerImpl::ApplicationManagerImpl(
this, &ApplicationManagerImpl::OnTimerSendTTSGlobalProperties))
, is_low_voltage_(false)
, apps_size_(0)
, is_registered_in_timeout_(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev rename it according to the meaning please.
registered_during_timer_execution or ...after_timer_expired or ..before_expiration_timer_start_ , depends on the meaning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

void PolicyHandler::OnAddedNewApplicationToAppList(
const uint32_t new_app_id, const std::string& policy_id) {
LOG4CXX_AUTO_TRACE(logger_);
if (last_registered_policy_app_id_ == policy_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev swap comparables please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -91,6 +91,12 @@ class UpdateStatusManager {
*/
void OnUpdateTimeoutOccurs();

/**
* @brief Update status for next in queue application
* after previous has finished
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev

  • previous status or previous update?
  • has finished -> has been finished or was finished

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AByzhynar update description c04c115ce

@@ -410,6 +410,14 @@ class PolicyHandlerInterface : public VehicleDataItemProvider {
*/
virtual void OnAppsSearchCompleted(const bool trigger_ptu) = 0;

/**
* @brief Notify that new applocation was added to application list
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev still typo...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* @brief Change applications count ready for PTU
* @param new_app_count new applications count for PTU
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev Remove description form derived classes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
* @brief Change applications count ready for PTU
* @param new_app_count new applications count for PTU
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

@ydementieiev Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ydementieiev
Copy link
Contributor Author

@theresalech this PR passed internal Luxoft review and ready for Livio team review.

Yevhenii Dementieiev (GitHub) added 2 commits January 31, 2020 12:57
… some trigger occurs during the retry sequence
@dboltovskyi
Copy link
Contributor

@ShobhitAd Please notice scripts in #2318 were updated.
However 2 of them are failed even if current PR would be merged to develop without conflicts. It seems GIT auto-merge works not so good.
Please see details in smartdevicelink/sdl_atf_test_scripts#2318 (comment)
Please do not merge this PR until issue is fixed.

@dboltovskyi
Copy link
Contributor

dboltovskyi commented Feb 11, 2020

@ShobhitAd Please notice all issues have been successfully fixed in ec86a40 and ready for review.

@dboltovskyi
Copy link
Contributor

dboltovskyi commented Feb 19, 2020

@theresalech Please notice all comments in current PR in core and corresponding one in scripts are addressed. Please advise when it's planned to finish PM review?

@jacobkeeler
Copy link
Contributor

jacobkeeler commented Feb 20, 2020

@ydementieiev After following the steps in #3136 (registering a second app during the retry sequence for the first app), I see an issue where the url parameter is missing from OnSystemRequest in the subsequent PTU retries after the first attempt fails. This means that the app cannot retrieve the policy table if the first attempt fails.

First request:

"notification": {
  "name": "OnSystemRequest",
  "correlationID": -1,
  "parameters": {
    "fileType": "JSON",
    "timeout": 500,
    "requestType": "PROPRIETARY",
    "offset": 1000,
    "length": 10000,
    "url": "http://192.168.1.166:3000/api/1/policies/proprietary"
  }
}

All subsequent requests:

"notification": {
  "name": "OnSystemRequest",
  "correlationID": -1,
  "parameters": {
    "fileType": "JSON",
    "timeout": 500,
    "requestType": "PROPRIETARY",
    "offset": 1000,
    "length": 10000
  }
}

Note that I am testing with EXTERNAL_PROPRIETARY

@theresalech
Copy link
Contributor

@ydementieiev @dboltovskyi do you have an update here? We would like to be able to review and merge this PR by the end of this week, so testing on Core 6.1 can begin.

@dboltovskyi
Copy link
Contributor

dboltovskyi commented Feb 25, 2020

@theresalech Please notice we have identified one additional issue since last PM review. All updates are planned to be finished at 02/26 EOD EST

@dboltovskyi
Copy link
Contributor

dboltovskyi commented Feb 27, 2020

@jacobkeeler Please find an explanation for the comment: #3208 (comment):
We suppose you did manual testing to get this result.
As for EXTERNAL_PROPRIETARY HMI provides data in BC.OnSystemRequest including url
SDL just transfers this data to mobile app in OnSystemRequest.

We did some check using sdl_hmi and found out that within merge of RGNSD feature logic related to url was broken.
GetURLs RPC was replaced by GetPolicyConfigurationData but usage of a new data structure was not aligned.

We have created an issue against sdl_hmi: smartdevicelink/sdl_hmi#278

@dboltovskyi
Copy link
Contributor

@theresalech Please notice we made all planned updates in sdl_core and sdl_atf_test_scripts so they are ready for PM review.
However within merge of WebEngine feature there are a few conflicts which needs to be resolved (in core and scripts).
We have a plan to finish this activity tomorrow.

@dboltovskyi
Copy link
Contributor

@jacobkeeler Please notice we resolved conflicts in core and scripts, so both PRs are ready for review.

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.

Redundant SDL.OnStatusUpdate(UPDATE_NEEDED) notification is sent once device is consented
9 participants