Fix SDL behavior during low voltage #3029

AKalinich-Luxoft · 2019-09-13T19:55:40Z

Fixes #2996

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Covered by ATF test scripts

Summary

There were found a several problems during SDL testing with ATF scripts:

SDL ignores messages from mobile/HMI during low voltage but as they are still delivered to the independent external container (e.g. socket) during Low Voltage - when SDL wakes up - it is not aware what data was in the container and starts processing them.
Sometimes SDL ignores messages on wake up due to timings
SDL low voltage process is not stopping properly by exit() function
SDL does not send OnAppUnregistered after wake up to HMI

To solve mentioned problems, the following changes were done:

exit() was replaced with _Exit() which allow to force close the child process which has fully done its job.
Suspend transport events processing threads during low voltage, such as unexpected disconnect etc. These events should be processed when SDL wakes up completely.
Suspend all client listening threads and shut down sockets for a TCP connections to prevent any possibility to receive message during low voltage. These threads will be resumed on wake up.
Set low_voltage flag to false in the end of wakeup() function to prevent any timing issues.
Don't add pending requests/notifications into request controller if low voltage event has happened, as it may happen at any moment
Don't handle received pending messages from mobile/HMI in RPC handler if low voltage event has happened as it may happen at any moment

CLA

I have signed the CLA

AByzhynar · 2019-09-17T13:00:18Z

@theresalech PR is ready for Livio team review. Thank you.

dboltovskyi · 2019-09-17T14:18:41Z

@theresalech Please notice there is an update in scripts related to the issue which needs to be taken into account as well: smartdevicelink/sdl_atf_test_scripts#2261

iCollin · 2019-09-17T21:57:23Z

There is quite a bit of duplicated code in this PR. It would be more maintainable to add a parameter to existing functions in Transport Manager, Transport Adapter and TCP Client Listener that determines the functionality than to have these duplicate functions.

Suspend all client listening threads and shut down sockets for a TCP connections to prevent any possibility to receive message during low voltage. These threads will be resumed on wake up.

While shutting down sockets was added in this PR, the TCP Client Listener thread will be joined in both current version and this branch. In fact, this PR removes the joining of the Interface Listener thread which is happening currently.

AByzhynar · 2019-09-18T15:03:58Z

@iCollin Please check the reply. Thank you.

There is quite a bit of duplicated code in this PR. It would be more maintainable to add a parameter to existing functions in Transport Manager, Transport Adapter and TCP Client Listener that determines the functionality than to have these duplicate functions.

Sure. You are right. We will try to combine duplicated code into functions.

Suspend all client listening threads and shut down sockets for a TCP connections to prevent any possibility to receive message during low voltage. These threads will be resumed on wake up.

While shutting down sockets was added in this PR, the TCP Client Listener thread will be joined in both current version and this branch. In fact, this PR removes the joining of the Interface Listener thread which is happening currently.

This was made intentionally as all the threads will be suspended during Low Voltage and all we need to do is just shutdown and close appropriate sockets and re-create them on WakeUp. So when the thread will be woken up - it will use new socket.

theresalech · 2019-09-19T13:53:25Z

@AByzhynar please let us know when this is ready for re-review

AByzhynar · 2019-09-20T11:01:07Z

@iCollin Duplicated code removed in 768a22c

AByzhynar · 2019-09-20T11:01:26Z

@theresalech PR is ready for re-review

AByzhynar · 2019-09-20T11:02:53Z

@iCollin Please don't merge after approval. I would like to squash commits to make history clear and understandable. Just APPROVE - and I will squash them and let you know that PR can be merged. Thank you.

iCollin · 2019-09-20T14:46:44Z

@iCollin Please don't merge after approval. I would like to squash commits to make history clear and understandable. Just APPROVE - and I will squash them and let you know that PR can be merged. Thank you.

I will squash and merge after I approve this PR, should get around to review in a few hours.

AByzhynar · 2019-09-20T14:47:41Z

@iCollin Please don't merge after approval. I would like to squash commits to make history clear and understandable. Just APPROVE - and I will squash them and let you know that PR can be merged. Thank you.

I will squash and merge after I approve this PR, should get around to review in a few hours.

Ok. Thank you. We have checked all unit tests as well as all regression tests.

iCollin · 2019-09-23T14:45:05Z

src/components/transport_manager/src/transport_manager_impl.cc

-    } else {
-      ret = (*it)->StopClientListening();
-    }
+    (*it)->ChangeClientListening(required_action);


Suggested change

(*it)->ChangeClientListening(required_action);

ret = (*it)->ChangeClientListening(required_action);

There were found a several problems during SDL testing with ATF scripts: 1. SDL does not ignore messages from mobile/HMI during low voltage and processes them on wake up 2. Sometimes SDL ignores messages on wake up due to timings 3. SDL low voltage process is not stopping properly by exit() function 4. SDL does not send OnAppUnregistered after wake up to HMI To solve mentioned problems, the following changes were done: 1. exit() was replaced with _Exit() which allow to force close the process 2. Suspend transport events processing threads during low voltage, such as unexpected disconnect etc. These events should be processed when SDL wakes up completely. 3. Suspend all client listening threads and shut down sockets for a TCP connections to prevent any possibility to receive message during low voltage. These threads will be resumed on wake up. 4. Set low_voltage flag to false in the end of wakeup() function to prevent any timing issues. 5. Don't add pending requests/notifications into request controller if low voltage event has happened, as it may happen at any moment 6. Don't handle received pending messages from mobile/HMI in RPC handler if low voltage event has happened as it may happen at any moment 7. Updated logic of few test case scenarios. Updated unit tests according to code changes

AByzhynar · 2019-09-24T14:35:06Z

src/components/transport_manager/src/transport_manager_impl.cc

-    }
+  TransportAdapter::Error ret = TransportAdapter::Error::UNKNOWN;
+
+  for (auto adapter_ptr : transport_adapters_) {


@iCollin I have updated the code and make it even more short and simple

AByzhynar · 2019-09-24T14:35:57Z

src/components/transport_manager/src/transport_manager_impl.cc

+  TransportAdapter::Error ret = TransportAdapter::Error::UNKNOWN;
+
+  for (auto adapter_ptr : transport_adapters_) {
+    ret = adapter_ptr->ChangeClientListening(required_action);


@iCollin as well as here

iCollin · 2019-09-24T14:58:00Z

We would appreciate if you did not force push on sdl_core repository. It makes it difficult to review a PR because we cannot tell what was changed since the last review. In general, please do not rebase, do not force push against sdl_core. Instead, you can merge the current develop branch into your PR branch. Please pass this information along to your team members. Thank you

AKalinich-Luxoft requested review from AByzhynar, LuxoftAKutsan, mked-luxoft, IGapchuk, mkorniichuk and SNiukalov September 13, 2019 19:55

mkorniichuk approved these changes Sep 16, 2019

View reviewed changes

IGapchuk approved these changes Sep 16, 2019

View reviewed changes

SNiukalov approved these changes Sep 17, 2019

View reviewed changes

AByzhynar approved these changes Sep 17, 2019

View reviewed changes

mked-luxoft approved these changes Sep 17, 2019

View reviewed changes

alexkutsan approved these changes Sep 17, 2019

View reviewed changes

LuxoftAKutsan approved these changes Sep 17, 2019

View reviewed changes

AByzhynar requested a review from iCollin September 20, 2019 13:10

iCollin reviewed Sep 23, 2019

View reviewed changes

AByzhynar force-pushed the fix/fix_sdl_low_voltage_behavior branch from 107b8ad to 8c71bbb Compare September 24, 2019 14:31

AByzhynar reviewed Sep 24, 2019

View reviewed changes

iCollin approved these changes Sep 24, 2019

View reviewed changes

iCollin merged commit b454f5c into develop Sep 24, 2019

iCollin deleted the fix/fix_sdl_low_voltage_behavior branch September 24, 2019 17:44

theresalech mentioned this pull request Sep 25, 2019

SDL doesn't WAKE_UP after LOW_VOLTAGE signal #2996

Closed

Jack-Byrne mentioned this pull request Oct 14, 2019

Core crash during ignition_off by ATF script execution #2859

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SDL behavior during low voltage #3029

Fix SDL behavior during low voltage #3029

AKalinich-Luxoft commented Sep 13, 2019 •

edited by AByzhynar

Loading

AByzhynar commented Sep 17, 2019

dboltovskyi commented Sep 17, 2019

iCollin commented Sep 17, 2019

AByzhynar commented Sep 18, 2019

theresalech commented Sep 19, 2019

AByzhynar commented Sep 20, 2019 •

edited

Loading

AByzhynar commented Sep 20, 2019

AByzhynar commented Sep 20, 2019 •

edited

Loading

iCollin commented Sep 20, 2019

AByzhynar commented Sep 20, 2019 •

edited

Loading

iCollin Sep 23, 2019

AByzhynar Sep 24, 2019

AByzhynar Sep 24, 2019

iCollin commented Sep 24, 2019

	(*it)->ChangeClientListening(required_action);
	ret = (*it)->ChangeClientListening(required_action);

Fix SDL behavior during low voltage #3029

Fix SDL behavior during low voltage #3029

Conversation

AKalinich-Luxoft commented Sep 13, 2019 • edited by AByzhynar Loading

Risk

Testing Plan

Summary

CLA

AByzhynar commented Sep 17, 2019

dboltovskyi commented Sep 17, 2019

iCollin commented Sep 17, 2019

AByzhynar commented Sep 18, 2019

theresalech commented Sep 19, 2019

AByzhynar commented Sep 20, 2019 • edited Loading

AByzhynar commented Sep 20, 2019

AByzhynar commented Sep 20, 2019 • edited Loading

iCollin commented Sep 20, 2019

AByzhynar commented Sep 20, 2019 • edited Loading

iCollin Sep 23, 2019

Choose a reason for hiding this comment

AByzhynar Sep 24, 2019

Choose a reason for hiding this comment

AByzhynar Sep 24, 2019

Choose a reason for hiding this comment

iCollin commented Sep 24, 2019

AKalinich-Luxoft commented Sep 13, 2019 •

edited by AByzhynar

Loading

AByzhynar commented Sep 20, 2019 •

edited

Loading

AByzhynar commented Sep 20, 2019 •

edited

Loading

AByzhynar commented Sep 20, 2019 •

edited

Loading