Feature/low voltage #2418

AByzhynar · 2018-07-25T12:49:22Z

Fixes #2233

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Unit tests are provided
ATF scripts are provided: smartdevicelink/sdl_atf_test_scripts#1953

Breaking Changes

None

Enhancements

None

Bug Fixes

None

Tasks Remaining:

None

Summary

Implemented SDL behavior during Low Voltage event including resumption flow.

CLA

I have signed the CLA

LuxoftAKutsan · 2018-07-25T12:52:37Z

src/appMain/life_cycle_impl.h

+  void StopComponents() OVERRIDE;
+
+  /**
+   * Makes appropriate actions when Low Voltage signal received:


@AByzhynar please avoid Doxygen comments overriden functions

@LuxoftAKutsan Thank you. Removed in 0f82294

AByzhynar · 2018-07-26T15:47:58Z

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

+  if (is_ign_off_record_missed) {
+    LOG4CXX_WARN(logger_,
+                 "Some IGN OFF records missed. Possibly due to Low Voltage");
+    return false;


@yang1070 I will reset global counters after normal IGNITION OFF flow

@yang1070 We found a betters and more simple solution. We will use only one IGN_ON counter and will reset it in normal IGNITION OFF flow. Code will be provided in separate commit later.

AByzhynar · 2018-07-26T15:52:45Z

src/components/application_manager/test/resumption/resume_ctrl_test.cc

@@ -1203,6 +1229,227 @@ TEST_F(ResumeCtrlTest, GetSavedAppHmiLevel_AskedAppFound_INVALID_ENUM) {
            res_ctrl_->GetSavedAppHmiLevel(kTestPolicyAppId_, kMacAddress_));
 }

+TEST_F(ResumeCtrlTest, ResumptiLowVoltage_AppInFull_Restored) {


@ZhdanovP Check typo here please

@yang1070 @AByzhynar the names were fixed at 7f61588

yang1070 · 2018-07-26T17:26:38Z

src/appMain/life_cycle_impl.cc

  return true;
 }

+void LifeCycleImpl::LowVoltage() {
+  LOG4CXX_AUTO_TRACE(logger_);


please keep this log, but identify and remove others logs that happens after sdl receives low voltage signal.
we can keep the logs after sdl receives wake up signal.

yang1070 · 2018-07-26T17:52:56Z

src/appMain/life_cycle_impl.h

+  protocol_handler::ProtocolHandlerImpl* protocol_handler_;
+  connection_handler::ConnectionHandlerImpl* connection_handler_;
+  application_manager::ApplicationManagerImpl* app_manager_;
+  std::unique_ptr<LowVoltageSignalsHandler> low_voltage_signals_handler_;


why this one is unique_ptr, but others are normal pointers?

@yang1070 because std::unique_ptr<LowVoltageSignalsHandler> low_voltage_signals_handler_; is the new code and the other pointers are old code. Therefore I don't want to make changes unrelated to feature.

yang1070 · 2018-07-27T19:44:46Z

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

@@ -335,6 +347,37 @@ void ResumeCtrlImpl::OnAwake() {
  StartSavePersistentDataTimer();
 }

+void ResumeCtrlImpl::SaveLowVoltageTime() {
+  ResetLowVoltageTime();


why do this extra reset?
low_voltage_time_ = 0;
low_voltage_time_= new value;

@yang1070 Right. Removed in bbd93f1

yang1070 · 2018-07-27T19:45:14Z

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

+}
+
+void ResumeCtrlImpl::SaveWakeUpTime() {
+  ResetWakeUpTime();


why do this extra reset?

@yang1070 Right. Removed in bbd93f1

AByzhynar · 2018-07-28T17:48:43Z

@yang1070 Logic for suspending SDL process implemented in cab72dd

AByzhynar · 2018-07-30T14:13:42Z

@yang1070 Part of resumption with SQL data base implemented in bf9412e

yang1070 · 2018-07-30T19:14:08Z

src/components/application_manager/include/application_manager/smart_object_keys.h

@@ -313,6 +313,9 @@ extern const char* resume_vr_grammars;

 extern const char* ign_off_count;

+extern const char* global_ign_on_counter;
+extern const char* global_ign_off_counter;


do we still need this counter?

@yang1070 Removed in bf125f9

yang1070 · 2018-07-30T19:25:11Z

src/components/application_manager/src/smart_object_keys.cc

@@ -276,6 +276,8 @@ const char* last_ign_off_time = "last_ign_off_time";
 const char* resume_vr_grammars = "resumeVrGrammars";

 const char* ign_off_count = "ign_off_count";
+const char* global_ign_on_counter = "global_ign_on_counter";
+const char* global_ign_off_counter = "global_ign_off_counter";


remove global_ign_off_counter?

@yang1070 Agree. Thanks. Will remove it.

@yang1070 Removed in bf125f9

yang1070 · 2018-07-30T19:29:07Z

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

-  resumption_storage_->IncrementIgnOffCount();
-  FinalPersistData();
+  if (!application_manager_.IsLowVoltage()) {
+    resumption_storage_->IncrementIgnOffCount();


still need IncrementIgnOffCount()?

@yang1070 This is different counter. Not for low Voltage handling. It existed before and used for different purposes.

yang1070

do we still need ign off counter?

AByzhynar · 2018-07-31T12:37:02Z

@yang1070 We don't need global ignition counter. We have removed it.

yang1070

The change looks good.

yang1070 · 2018-07-31T12:39:51Z

@theresalech Ford has reviewed, tested and approved this change. It is ready for Livio to review.

jacobkeeler · 2018-08-10T00:46:22Z

src/appMain/life_cycle_impl.cc

+#ifdef DBUS_HMIADAPTER
+    , dbus_adapter_(NULL)
+    , dbus_adapter_thread_(NULL)
+#endif  // DBUS_HMIADAPTER


remove, dbus adapter was removed

@jacobkeeler all mentions of DBUS_HMIADAPTER removed

jacobkeeler · 2018-08-10T00:46:48Z

src/appMain/life_cycle_impl.cc

+  return true;
+}
+#endif  // DBUS_HMIADAPTER
+


dbus adapter was removed

@jacobkeeler 'Removing Dbus' - is the change not related to the feature. I think that it is more correct way - to remove it in separate PR.
Or should we remove it within this PR?

This code was removed in a previous PR, but this PR seems to be adding this logic back in where it previously existed. I am just asking that it not be added in again.

@jacobkeeler fixed 0f738f7

jacobkeeler · 2018-08-10T00:47:05Z

src/appMain/life_cycle_impl.cc

+    delete dbus_adapter_thread_;
+    dbus_adapter_thread_ = NULL;
+  }
+#endif  // DBUS_HMIADAPTER


dbus adapter was removed

@jacobkeeler fixed 0f738f7

jacobkeeler · 2018-08-10T00:59:16Z

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

@@ -33,6 +33,8 @@
 #ifndef SRC_COMPONENTS_APPLICATION_MANAGER_INCLUDE_APPLICATION_MANAGER_RESUMPTION_RESUME_CTRL_IMPL_H_
 #define SRC_COMPONENTS_APPLICATION_MANAGER_INCLUDE_APPLICATION_MANAGER_RESUMPTION_RESUME_CTRL_IMPL_H_

+#include "application_manager/resumption/resume_ctrl.h"


Why move the include?

@jacobkeeler according to SDL C++ Coding Style

jacobkeeler · 2018-08-14T23:33:16Z

src/appMain/low_voltage_signals_handler.h

+  void HandleSignal(const int signo);
+
+  /**
+   * @brief Returns signals mask required for handlning


typo, should be handling

@jacobkeeler fixed 50d98d3

jacobkeeler · 2018-08-14T23:48:49Z

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

+   */
+  bool CheckLowVoltageRestrictions(const smart_objects::SmartObject& saved_app);
+
+  DEPRECATED bool DisconnectedJustBeforeIgnOff(


This can just be removed since this is being included in the 5.0.0 release

@jacobkeeler removed cca195b

jacobkeeler · 2018-08-14T23:50:20Z

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

@@ -409,10 +454,37 @@ class ResumeCtrlImpl : public ResumeCtrl,

  /**
   * @brief CheckDelayAfterIgnOn should check if SDL was started less
-   * then N seconds ago. N will be readed from profile.
+   * then N seconds ago. N will be read from profile.


may as well fix the typo with than while you are at it.

@jacobkeeler fixed d176706

jacobkeeler · 2018-08-14T23:50:47Z

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

+
+  /**
+   * @brief CheckDelayBeforeIgnOff checks if app was unregistered less
+   * then N seconds before Ignition OFF. N will be read from profile.


than N seconds

@jacobkeeler fixed d176706

jacobkeeler · 2018-08-14T23:51:19Z

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

+
+  /**
+   * @brief CheckDelayAfterWakeUp should check if app was registered
+   * during N seconds after WakeUp signal. N will be read from profile.


during the first N seconds

@jacobkeeler fixed 8043e15

jacobkeeler · 2018-08-15T00:10:31Z

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

+      resumption_storage_->GetGlobalIgnOnCounter();
+  const uint32_t allowed_ign_on_value = 2;
+  const bool is_emergency_ign_off_occurred =
+      global_ign_on_count >= allowed_ign_on_value;


should be allowed_ign_on_value = 1 and global_ign_on_count > allowed_ign_on_value

@jacobkeeler in that case constant allowed_ign_on_value should be renamed and it is hard for me to imagine the name in that case : maximum_not_allowed_ignition_on_occured ?

@jacobkeeler ad50326

jacobkeeler · 2018-08-15T00:13:44Z

src/components/application_manager/src/resumption/resumption_data_db.cc

+    }
+  }
+  LOG4CXX_ERROR(logger_, "Problem with prepare query");
+  return global_ign_on_counter;


Just return 0 here and reduce scope of global_ign_on_counter

@jacobkeeler 6160ea0

jacobkeeler · 2018-08-15T00:17:36Z

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

+    LOG4CXX_DEBUG(logger_, "Global Ign On Counter = " << global_ign_on_counter);
+    return global_ign_on_counter;
+  }
+  return 1;


seems default would be 0

@jacobkeeler if SDL is alive, at least one ignition on occurred.

Equivalent change should be made to resumption_data_db.cc then, since the default is 0 there.

@jacobkeeler fixed 19cca45

jacobkeeler · 2018-08-15T00:34:56Z

src/components/application_manager/test/resumption/resume_ctrl_test.cc

+  const mobile_apis::HMILevel::eType restored_test_type = eType::HMI_FULL;
+  const uint32_t time_offset = 5;
+  const uint32_t time_stamp =
+      time(nullptr) - resumption_delay_before_ign_ + time_offset;


Seems like this should be - time_offset rather than + time_offset based on the description. Although it does seem that this test would be prone to occasional failures, since Core grabs the timestamp separately from the test (as far as I can see).

@jacobkeeler On the border it means that app was unregistered in the frame of 30 seconds before low voltage and application should restore it's HMI level. So there should be + time_offset.

Here is the example: [----|-----u-----L]

| - 30 seconds frame before the low voltage

u - unregistering application

L - low voltage event

In case if there will be - time_offset there will be the following picture :
[---u---|----L], and Application will not restore it's HMI level.

I understood "on the border" as "exactly 30 seconds before LOW_VOLTAGE".
Another thing to note here is that the time_offset is added to the delay in this particular test case, so the window is 35 seconds.

EXPECT_CALL(app_mngr_settings_, resumption_delay_before_ign()) .WillOnce(Return(resumption_delay_before_ign_ + time_offset));

If this is just testing when an app unregisters within the time frame, then this case is redundant, as that case is covered by ResumptionLowVoltage_AppInFullUnregisteredWithinTimeFrame_HMILevelRestored

@jacobkeeler fixed 33e6dd4

@jacobkeeler removed 5224964

jacobkeeler · 2018-08-15T00:44:13Z

src/components/transport_manager/src/transport_manager_impl.cc

@@ -532,6 +532,8 @@ int TransportManagerImpl::Reinit() {
  LOG4CXX_AUTO_TRACE(logger_);
  DisconnectAllDevices();
  TerminateAllAdapters();
+  device_to_adapter_map_.clear();
+  connection_id_counter_ = 0;


These changes don't appear relevant.

After LowVoltage and Wake Up we should clear old devices from list

jacobkeeler · 2018-08-16T19:35:36Z

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

  const bool is_emergency_ign_off_occurred =
-      global_ign_on_count >= allowed_ign_on_value;
+      global_ign_on_count > the_first_ignition;
+  // In case of resumption it should be minimum second ignition cycle.


This comment does not match the functionality. This count is reset on IGNITION_OFF, so a value greater than 1 means that the project was shut down by force. A value of 1 does not meant that it is the first ignition cycle.

@jacobkeeler fixed 06ce5d9

jacobkeeler · 2018-08-16T19:36:55Z

src/components/application_manager/src/resumption/resumption_data_db.cc

      LOG4CXX_DEBUG(logger_,
                    "Global Ign On Counter = " << global_ign_on_counter);
      return global_ign_on_counter;
+    } else {
+      LOG4CXX_ERROR(logger_,
+                    "Problem with exec query : " << kSelectGlobalIgnOnCounter);


This else case is already covered, it will just continue on to the end of the function. This will just result in this log being printed twice

@jacobkeeler fixed 24e9b7c

Created Life cycle class interface in order: - to make unit-testing available - to decrease components coupling

Implemented Low Voltage feature based on POSIX real-time signals

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

Removed redundant functions Simplified ingintion cycle data correctness check

Implemented Low Voltage functionality handling in separate process in order to have possibility to suspend entire SDL process in case of Low Voltage signal After receipt of WakeUp signal SDL process is beeing waken up and resumes all its activities.

ResumptionLowVoltage_AppInFullUnregisteredOnTheBorderOfTimeFrame_HMILevelRestored unit test Remove unit test beause of possible sporadic failures

AByzhynar requested review from yang1070 and LuxoftAKutsan July 25, 2018 12:50

LuxoftAKutsan reviewed Jul 25, 2018

View reviewed changes

AByzhynar commented Jul 26, 2018

View reviewed changes

yang1070 reviewed Jul 26, 2018

View reviewed changes

ZhdanovP force-pushed the feature/low_voltage branch from 7f61588 to f9a741b Compare July 27, 2018 13:26

AStasiuk requested review from askirk, mrapitis, yang1070 and robinmk July 27, 2018 15:45

yang1070 reviewed Jul 27, 2018

View reviewed changes

AByzhynar force-pushed the feature/low_voltage branch from f9a741b to 0f82294 Compare July 28, 2018 17:04

AByzhynar force-pushed the feature/low_voltage branch 2 times, most recently from 79c68f2 to 1c66a5e Compare July 30, 2018 11:10

yang1070 reviewed Jul 30, 2018

View reviewed changes

yang1070 suggested changes Jul 30, 2018

View reviewed changes

yang1070 approved these changes Jul 31, 2018

View reviewed changes

theresalech requested a review from jacobkeeler July 31, 2018 13:06

jacobkeeler requested changes Aug 10, 2018

View reviewed changes

jacobkeeler requested changes Aug 15, 2018

View reviewed changes

smartdevicelink deleted a comment from JenkinsSDLOnCloud Aug 15, 2018

LuxoftAKutsan force-pushed the feature/low_voltage branch from 8043e15 to f51a338 Compare August 16, 2018 18:03

jacobkeeler reviewed Aug 16, 2018

View reviewed changes

AByzhynar and others added 15 commits August 21, 2018 12:29

Edit error in function name

d3b1ab4

Create Life Cycle class interface

af2f7df

Created Life cycle class interface in order: - to make unit-testing available - to decrease components coupling

Initial implementation of Low Voltage feature

d1308d1

Implemented Low Voltage feature based on POSIX real-time signals

Implement unit tests for Low Voltage Signals handler

b003a5f

Implement resumption during Low Voltage

5ca9746

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

890d962

Remove redundant function descriptions from Impl class

d930a54

Fix unit tests names

c6d7882

Simplify resumption logic

79b4ac8

Removed redundant functions Simplified ingintion cycle data correctness check

Add new unit tests for Low Voltage Signals Handler

1b49007

Add implementation of resumption DB for low voltage

e8d9e67

Fix unit test

83e5c25

Remove unused variables

429972e

Remove deprecated DisconnectedJustBeforeIgnOff method

0d5978f

LuxoftAKutsan force-pushed the feature/low_voltage branch from 33e6dd4 to 0d5978f Compare August 21, 2018 09:30

LuxoftAKutsan and others added 3 commits August 21, 2018 14:34

Remove

5224964

ResumptionLowVoltage_AppInFullUnregisteredOnTheBorderOfTimeFrame_HMILevelRestored unit test Remove unit test beause of possible sporadic failures

Merge branch 'develop' into feature/low_voltage

9b34f54

Fix style

c1252c6

jacobkeeler approved these changes Aug 21, 2018

View reviewed changes

jacobkeeler merged commit 7af24ab into develop Aug 21, 2018

jacobkeeler mentioned this pull request Aug 21, 2018

[SDL 0170] SDL behavior in case of LOW_VOLTAGE event #2233

Closed

jacobkeeler deleted the feature/low_voltage branch August 24, 2018 19:38

Feature/low voltage #2418

Feature/low voltage #2418

Conversation

AByzhynar commented Jul 25, 2018 • edited Loading

Risk

Testing Plan

Breaking Changes

Enhancements

Bug Fixes

Tasks Remaining:

Summary

CLA

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

ZhdanovP Jul 27, 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

AByzhynar Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AByzhynar commented Jul 28, 2018 • edited Loading

AByzhynar commented Jul 30, 2018

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

yang1070 left a comment

Choose a reason for hiding this comment

AByzhynar commented Jul 31, 2018

yang1070 left a comment

Choose a reason for hiding this comment

yang1070 commented Jul 31, 2018

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 Aug 10, 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

LuxoftAKutsan Aug 16, 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

AByzhynar commented Jul 25, 2018 •

edited

Loading

ZhdanovP Jul 27, 2018 •

edited

Loading

AByzhynar Jul 28, 2018 •

edited

Loading

AByzhynar commented Jul 28, 2018 •

edited

Loading

AByzhynar Aug 10, 2018 •

edited

Loading

LuxoftAKutsan Aug 16, 2018 •

edited

Loading

jacobkeeler Aug 16, 2018 •

edited

Loading