Feature/resumption during low voltage #2412

AByzhynar · 2018-07-24T09:42:49Z

Fixes #2233

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Testing Plan

Unit tests will be provided
ATF tests will be provided

Summary

Added resumption logic during Low Voltage event

CLA

I have signed the CLA

LuxoftAKutsan · 2018-07-24T09:48:29Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl_impl.h

+   * @return Low Voltage event timestamp if event LOV VOLTAGE event occures
+   * otherwise 0
+   */
+  time_t LowVoltageTime() const OVERRIDE;


@AByzhynar please avoid duplication of comments in implementation and interface

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T09:50:34Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl.h

+  /**
+   * @brief Resets Low Voltage signal timestamp to zero
+   */
+  virtual void ResetLowVoltageTime() = 0;


@AByzhynar check if this functions should be public in interface

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T09:51:40Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl_impl.h

+   * @brief Checks if saved HMI level is allowed for resumption
+   * by Ignition Cycle restrictions
+   * @param saved_app application specific section from backup file
+   * @return True is allowed , othrwise - False


@AByzhynar typo
true if allowed

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T10:05:03Z

src/components/application_manager/src/application_manager_impl.cc

-      resume_controller().SaveApplication(app_to_remove);
-    } else {
-      resume_controller().RemoveApplicationFromSaved(app_to_remove);
+    if (!IsLowVoltage()) {


@AByzhynar move low voltage check to ResumeCtrl

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T10:10:30Z

src/components/application_manager/src/resumption/resume_ctrl_impl.cc

-    LOG4CXX_INFO(logger_, "Application was dissconnected long before ign off");
-    result = false;
+  if (!CheckDelayBeforeIgnOff(saved_app)) {
+    LOG4CXX_INFO(logger_, "Application was disconnected long before ign off");


@AByzhynar rename to debug

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T11:53:27Z

src/components/application_manager/src/resumption/resume_ctrl_impl.cc

+      static_cast<time_t>(resumption_storage_->GetIgnOffTime());
+
+  if (0 == ign_off_time) {
+    return true;


@AByzhynar in case if function has multiple returns please log information about return reason

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T12:02:59Z

src/components/application_manager/src/resumption/resumption_data_json.cc

+  sync_primitives::AutoLock autolock(resumption_lock_);
+  Json::Value& resumption = GetResumptionData();
+  if (!resumption.isMember(strings::global_ign_on_counter)) {
+    resumption[strings::global_ign_on_counter] = 1;


@AByzhynar it is not expected that getter will change data.

@LuxoftAKutsan Absolutely agree

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-24T12:03:36Z

src/components/application_manager/src/resumption/resumption_data_json.cc

+  sync_primitives::AutoLock autolock(resumption_lock_);
+  Json::Value& resumption = GetResumptionData();
+  if (!resumption.isMember(strings::global_ign_off_counter)) {
+    resumption[strings::global_ign_off_counter] = 0;


@AByzhynar it is not expected that getter will change data.

@LuxoftAKutsan Done in 93ff02d

AKalinich-Luxoft · 2018-07-24T09:54:40Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl.h

@@ -124,6 +124,40 @@ class ResumeCtrl {
   */
  virtual void OnAwake() = 0;

+  /**
+   * @brief Returns Low Voltage signal timestamp
+   * @return Low Voltage event timestamp if event LOV VOLTAGE event occures


@AByzhynar Low Voltage event timestamp if LOV VOLTAGE event occures

@LuxoftAKutsan Done in 93ff02d

AKalinich-Luxoft · 2018-07-24T13:02:57Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl_impl.h

+   * @return Wake Up timestamp if Wake Up signal occures
+   * otherwise 0
+   */
+  time_t WakeUpTime() const;


@AByzhynar I would suggest to create enum like TimestampType{LOW_VOLTAGE, WAKE_UP}
And then you could reduce count of functions:

GetTime(TimestampType)

SaveTime(TimestampType)

ResetTime(TimestampType)

@AKalinich-Luxoft Disagree. it is the same type for all events. I don't see any reason to complicate the code.

AKalinich-Luxoft · 2018-07-24T13:06:12Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl_impl.h

+   * Ignition Off,
+   * otherwise return false
+   */
+  bool CheckDelayBeforeIgnOff(const smart_objects::SmartObject& saved_app);


@AByzhynar could these check functions be const?

@LuxoftAKutsan Done in 93ff02d

AKalinich-Luxoft · 2018-07-24T13:06:42Z

src/components/application_manager/include/application_manager/resumption/resume_ctrl_impl.h

+
+  /**
+   * @brief Returns Low Voltage signal timestamp
+   * @return Low Voltage event timestamp if event LOV VOLTAGE event occures


@AByzhynar redundant event word

@return Low Voltage event timestamp if LOV VOLTAGE event occures

AKalinich-Luxoft · 2018-07-24T13:22:59Z

src/components/application_manager/src/resumption/resume_ctrl_impl.cc

+  const uint32_t global_ign_on_count =
+      resumption_storage_->GetGlobalIgnOnCounter();
+  const uint32_t diff = global_ign_on_count - global_ign_off_count;
+  if (diff >= 2) {


@AByzhynar could be moved to a separate const with a meaningful name

@LuxoftAKutsan Done in 93ff02d

LuxoftAKutsan · 2018-07-25T07:44:04Z

src/components/application_manager/src/resumption/resume_ctrl_impl.cc

+
+void ResumeCtrlImpl::ResetLowVoltageTime() {
+  low_voltage_time_ = 0;
+  LOG4CXX_DEBUG(logger_, "Resetting Low Voltage timestamp :");


@AByzhynar redundant semicolon

@LuxoftAKutsan Done and squashed.

LuxoftAKutsan · 2018-07-25T07:44:16Z

src/components/application_manager/src/resumption/resume_ctrl_impl.cc

+
+void ResumeCtrlImpl::ResetWakeUpTime() {
+  wake_up_time_ = 0;
+  LOG4CXX_DEBUG(logger_, "Resetting Wake Up timestamp :");


@AByzhynar redundant semicolon

@LuxoftAKutsan Done and squashed.

Implemented resumption logic during Low Voltage (used app_info.dat file for resumption.Configurable in smartdevicelink.ini file)

AByzhynar requested review from AKalinich-Luxoft, LuxoftAKutsan and EKuliiev July 24, 2018 09:44

LuxoftAKutsan reviewed Jul 24, 2018

View reviewed changes

AByzhynar force-pushed the feature/resumption_during_low_voltage branch 2 times, most recently from 3c9dd5c to 94730fa Compare July 24, 2018 11:03

LuxoftAKutsan reviewed Jul 24, 2018

View reviewed changes

AKalinich-Luxoft suggested changes Jul 24, 2018

View reviewed changes

ZhdanovP force-pushed the feature/resumption_during_low_voltage branch from 2be4522 to e8c27a3 Compare July 24, 2018 16:48

AByzhynar force-pushed the feature/resumption_during_low_voltage branch from e8c27a3 to c073062 Compare July 24, 2018 17:10

ZhdanovP force-pushed the feature/resumption_during_low_voltage branch from c073062 to 7ec0f18 Compare July 24, 2018 17:19

AByzhynar force-pushed the feature/low_voltage branch from 65c39a3 to 4506e6d Compare July 24, 2018 18:02

AByzhynar force-pushed the feature/resumption_during_low_voltage branch 2 times, most recently from fc1c822 to 93ff02d Compare July 24, 2018 18:39

AKalinich-Luxoft approved these changes Jul 25, 2018

View reviewed changes

LuxoftAKutsan reviewed Jul 25, 2018

View reviewed changes

LuxoftAKutsan approved these changes Jul 25, 2018

View reviewed changes

AByzhynar force-pushed the feature/resumption_during_low_voltage branch 2 times, most recently from 5328fdc to 36d137b Compare July 25, 2018 11:18

AByzhynar and others added 2 commits July 25, 2018 14:20

Implement resumption during Low Voltage

9539f16

Implemented resumption logic during Low Voltage (used app_info.dat file for resumption.Configurable in smartdevicelink.ini file)

Add unit tests for resumption during low voltage

24b20e8

AByzhynar force-pushed the feature/resumption_during_low_voltage branch from 36d137b to 24b20e8 Compare July 25, 2018 12:07

AByzhynar merged commit ad32f74 into smartdevicelink:feature/low_voltage Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/resumption during low voltage #2412

Feature/resumption during low voltage #2412

AByzhynar commented Jul 24, 2018 •

edited by EKuliiev

Loading

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018 •

edited

Loading

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018 •

edited

Loading

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018 •

edited

Loading

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018 •

edited

Loading

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018 •

edited

Loading

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018

AByzhynar Jul 24, 2018

LuxoftAKutsan Jul 24, 2018

AByzhynar Jul 24, 2018

AKalinich-Luxoft Jul 24, 2018

AByzhynar Jul 24, 2018

AKalinich-Luxoft Jul 24, 2018

AByzhynar Jul 24, 2018

AKalinich-Luxoft Jul 24, 2018

AByzhynar Jul 24, 2018

AKalinich-Luxoft Jul 24, 2018

AKalinich-Luxoft Jul 24, 2018

AByzhynar Jul 24, 2018

LuxoftAKutsan Jul 25, 2018

AByzhynar Jul 25, 2018

LuxoftAKutsan Jul 25, 2018

AByzhynar Jul 25, 2018

Feature/resumption during low voltage #2412

Feature/resumption during low voltage #2412

Conversation

AByzhynar commented Jul 24, 2018 • edited by EKuliiev Loading

Risk

Testing Plan

Summary

CLA

Choose a reason for hiding this comment

AByzhynar Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar commented Jul 24, 2018 •

edited by EKuliiev

Loading

AByzhynar Jul 24, 2018 •

edited

Loading

AByzhynar Jul 24, 2018 •

edited

Loading

AByzhynar Jul 24, 2018 •

edited

Loading

AByzhynar Jul 24, 2018 •

edited

Loading

AByzhynar Jul 24, 2018 •

edited

Loading