[SDL-0189] Restructuring OnResetTimeout #3726

ychernysheva · 2021-06-23T16:10:31Z

Fixes #2422
Also fixes known issue #2829

This PR is ready for review.

Risk

This PR makes major API changes.

Testing Plan

Unit testing.
ATF testing.

Summary

Expand OnResetTimeout RPC for all interfaces.
Note: SDL will apply OnResetTimeout only in case when resetPeriod value is greater than the time remaining from the current timeout

Changelog

Breaking Changes

Remove deprecated functionality
Add Reset Timeout Handler
Create Request Controller interface

CLA

I have signed the CLA

…quest Controller interface.

…eate Request Controller interface.

…dant UTs

[SDL-0189] Fix onResetTimeout twice notification

…e_alert_with_soft_buttons [SDL-0189] Restrict timeout for subtle alert with soft buttons

[SDL-0189] fix crash sdl_core on close

atiwari9

Thanks for the PR, i have left some review comments. Also, can you please confirm if BC.OnResetTimeout is applicable to all RPCs including Alert, DialNumber, SubtleAlert etc.? Or do we have specific requirements barring that? I noticed that there are some tests in atf test scripts designed to ignore OnResetTimeout for these RPCs.

atiwari9 · 2021-06-24T13:42:18Z

src/components/application_manager/include/application_manager/request_controller_impl.h

- * requests.
- */
-class RequestController {
+class RequestControllerImpl : public RequestController {


Please add all the deleted comments as applicable.

@atiwari9 please note that RequestControllerImpl is now just a class containing public functions overridden from the interface class. We don't need to duplicate the description of such functions in the impl classes according to our coding style:
https://google.github.io/styleguide/cppguide.html#Function_Comments

When documenting function overrides, focus on the specifics of the override itself, rather than repeating the comment from the overridden function. In many of these cases, the override needs no additional documentation and thus no comment is required.

@AKalinich-Luxoft , @ychernysheva - Please let me know when the PR is ready to be reviewed after above mentioned changes.

atiwari9 · 2021-06-24T13:45:38Z

...r/rpc_plugins/rc_rpc_plugin/src/commands/hmi/rc_get_interior_vehicle_data_consent_request.cc

@@ -45,7 +45,10 @@ RCGetInteriorVehicleDataConsentRequest::RCGetInteriorVehicleDataConsentRequest(
                                                  params.application_manager_,
                                                  params.rpc_service_,
                                                  params.hmi_capabilities_,
-                                                  params.policy_handler_) {}
+                                                  params.policy_handler_) {


Shouldn't this be read from smartDeviceLink.ini config file. Why is this timeout doubled?

@atiwari9
Yes, initially default timeout value is taken from the SDL config file for all command requests:

sdl_core/src/components/application_manager/src/commands/command_impl.cc

Line 84 in 60e3354

, default_timeout_(application_manager.get_settings().default_timeout())

Later, some particular commands are adjusting the default timeout according to their internal needs.
In this case, timeout is doubled to give HMI extra time for showing user consent prompt popup (which is another +10 seconds) - please look at linked issue #2829 for more details and sequence diagram.

I understand that this may have been used as a work around earlier, when HMI probably didn't have a way to reset the timeout on SDL side, now it does. So we really don't and shouldn't still add such patches in SDL. If this is done to maintain the legacy implementation, i understand. But even in that case please log a bug/improvement item so that the onus can now be shifted back to HMI. After all, that's the whole point of enabling HMI to reset timeout for RPCs.

@atiwari9 ok, I think we can remove all these timeout adjustments from existing PR and rely on HMI to control the lifetime of corresponding requests properly. In that case, we need the following changes:

Remove timeout adjustment changes from affected SDL core requests

Update the same requests on the HMI side - if HMI notices that extra time from the user is required then HMI sends OnResetTimeout to SDL with a doubled timeout value so SDL can wait for enough time

Check with ATF scripts that updated implementation does not break the legacy code and feature works as expected

Please let me know, is that plan works for you?

atiwari9 · 2021-06-24T13:46:29Z

...ts/application_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/button_press_request.cc

@@ -54,6 +54,21 @@ ButtonPressRequest::ButtonPressRequest(

 ButtonPressRequest::~ButtonPressRequest() {}

+bool ButtonPressRequest::Init() {


Again, this should be HMI's responsibility. Why are we changing timeout in SDL?

@atiwari9
SDL should understand from its side whether the mobile request expired (and respond with GENERIC_ERROR) or HMI is still working on it. The reason for changing the timeout is the same as in the previous comment.

Please see comment: #3726 (comment)

atiwari9 · 2021-06-24T13:46:44Z

...n_manager/rpc_plugins/rc_rpc_plugin/src/commands/mobile/set_interior_vehicle_data_request.cc

@@ -54,6 +54,22 @@ SetInteriorVehicleDataRequest::SetInteriorVehicleDataRequest(

 SetInteriorVehicleDataRequest::~SetInteriorVehicleDataRequest() {}



Same comment as above. this should be HMI's responsibility. Why are we changing timeout in SDL?

@atiwari9
SDL should understand from its side whether the mobile request expired (and respond with GENERIC_ERROR) or HMI is still working on it. The reason for changing the timeout is the same as in the previous comment.

#3726 (comment)

AKalinich-Luxoft · 2021-06-24T17:49:54Z

Thanks for the PR, i have left some review comments. Also, can you please confirm if BC.OnResetTimeout is applicable to all RPCs including Alert, DialNumber, SubtleAlert etc.? Or do we have specific requirements barring that? I noticed that there are some tests in atf test scripts designed to ignore OnResetTimeout for these RPCs.

@atiwari9 it's hard to simply say yes or no here. Basically, BC.OnResetTimeout is applicable to all RPCs until some specific requirement blocks that. For example, we have a requirement that timeout reset can't be applied to DialNumber. Also, we have a requirement that timeout is not applicable to Alert/SubtleAlert in case if these requests contain soft buttons and so on...

More details and references will be provided for all these requirements in atf test scripts PR

atiwari9 · 2021-06-24T19:14:10Z

@ychernysheva - Also can you please log a bug for the case where Alert response is still ABORTED when TTS.Alert timeout is reset and UI.Alert finishes. Ideally it should wait for both UI and TTS Alert timeouts to be complete.

AKalinich-Luxoft · 2021-06-25T01:21:49Z

@ychernysheva - Also can you please log a bug for the case where Alert response is still ABORTED when TTS.Alert timeout is reset and UI.Alert finishes. Ideally it should wait for both UI and TTS Alert timeouts to be complete.

@atiwari9 I don't think this is a bug. It's a part of the existing SDL logic - in case if SDL receives a response from HMI on the UI part but the TTS part is still in progress, then SDL sends TTS.StopSpeaking to abort the TTS part, waits for HMI to abort TTS, and then responds with ABORTED code.
Implementation of the possibility to reset timeout for the TTS part only allows reproducing the case when TTS response is received after the UI part (however ideally TTS response should be received before UI part).
In other words, I don't think SDL should wait for both UI and TTS - response to the UI part is sufficient

atiwari9 · 2021-06-25T13:25:58Z

@ychernysheva - Also can you please log a bug for the case where Alert response is still ABORTED when TTS.Alert timeout is reset and UI.Alert finishes. Ideally it should wait for both UI and TTS Alert timeouts to be complete.

@atiwari9 I don't think this is a bug. It's a part of the existing SDL logic - in case if SDL receives a response from HMI on the UI part but the TTS part is still in progress, then SDL sends TTS.StopSpeaking to abort the TTS part, waits for HMI to abort TTS, and then responds with ABORTED code.
Implementation of the possibility to reset timeout for the TTS part only allows reproducing the case when TTS response is received after the UI part (however ideally TTS response should be received before UI part).
In other words, I don't think SDL should wait for both UI and TTS - response to the UI part is sufficient

Thinking further on it, i think the scenario where user has dismissed the Alert while TTS was playing does count as ABORTED, so it is fine. Though i am strongly for HMI to manage that instead of putting it on SDL. SDL should not determine the user experience HMI wants for its users, hence i am usually not a proponent of SDL side workarounds/patches as it creates a hard dependency. But since this is legacy implementation, we can let this particular case slide.

* Do not manage requests with NULL timeout

…ut_remove_default_timeout_adjustments Remove default timeout adjustments from core

AKalinich-Luxoft · 2021-06-29T02:09:15Z

@atiwari9 core changes according to comment #3726 (comment) have been added in f27c548 and fd71265

atiwari9

Aprproved.

LitvinenkoIra · 2021-06-29T18:34:50Z

@theresalech This PR is ready for Livio review. Thank you!

src/components/application_manager/include/application_manager/application_manager_impl.h

src/components/application_manager/include/application_manager/commands/command_request_impl.h

src/components/application_manager/include/application_manager/request_controller_impl.h

src/components/application_manager/include/application_manager/request_timeout_handler_impl.h

src/components/application_manager/src/smart_object_keys.cc

src/components/include/application_manager/application_manager.h

src/components/interfaces/HMI_API.xml

AKalinich-Luxoft · 2021-07-23T18:37:13Z

@ShobhitAd all comments have been processed. Could you please proceed with your review here?

iCollin · 2021-07-28T19:12:29Z

@AKalinich-Luxoft Hello,

With these changes OnResetTimeout with no resetPeriod specified will set the timeout equal to the defaultTimeout from the ini. Previously, the HMI requesting a ResetTimeout would set the timeout to a command's modified default_timeout_ property, which is greater than defaultTimeout. Because of this change, if the HMIs now send a TIMEOUT message after the defaultTimeout, Core will have sent mobile a GENERIC_ERROR response before the HMI's response can be processed, resulting in Mobile not receiving responses it should.

In order to handle the time the message spends on the wire (in transport), in a message queue, and finally being processed I propose we add some additional time to these timers, ideally configurable from the INI.

At the moment these TIMEOUT responses can be received by mobile because the HMI is decrementing all incoming timeouts by 1000ms, which will be removed.

Update default timeout logic accordingly for all commands. Added new file to ini file.

AKalinich-Luxoft · 2021-07-30T18:49:23Z

@AKalinich-Luxoft Hello,

With these changes OnResetTimeout with no resetPeriod specified will set the timeout equal to the defaultTimeout from the ini. Previously, the HMI requesting a ResetTimeout would set the timeout to a command's modified default_timeout_ property, which is greater than defaultTimeout. Because of this change, if the HMIs now send a TIMEOUT message after the defaultTimeout, Core will have sent mobile a GENERIC_ERROR response before the HMI's response can be processed, resulting in Mobile not receiving responses it should.

In order to handle the time the message spends on the wire (in transport), in a message queue, and finally being processed I propose we add some additional time to these timers, ideally configurable from the INI.

At the moment these TIMEOUT responses can be received by mobile because the HMI is decrementing all incoming timeouts by 1000ms, which will be removed.

@iCollin all changes have been implemented in 031c65f
We added a default timeout compensation param to ini file which is defaulted to 1000ms.
This timeout compensation is applied to all commands timeouts initially and also applied after OnResetTimeout notification.
All affected unit tests were updated accordingly.

src/components/application_manager/rpc_plugins/rc_rpc_plugin/src/commands/rc_command_request.cc

...application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/hmi/navi_start_stream_request.cc

src/components/config_profile/include/config_profile/profile.h

ShobhitAd · 2021-08-06T13:50:30Z

@AKalinich-Luxoft thank you for addressing the review comments. Please resolve the merge conflicts on the PR

…ing_OnResetTimeout

AKalinich-Luxoft · 2021-08-06T14:01:27Z

@AKalinich-Luxoft thank you for addressing the review comments. Please resolve the merge conflicts on the PR

@ShobhitAd done

ShobhitAd · 2021-08-06T17:20:54Z

@AKalinich-Luxoft a small comment regarding the rpc_spec commit change.
It looks like the rpc_spec commit used in the PR is smartdevicelink/rpc_spec@d4990d8 but the latest rpc_spec commit for develop is smartdevicelink/rpc_spec@12c2d1d
Could you update the rpc_spec submodule commit in the PR to point to the latest commit on rpc_spec develop

AKalinich-Luxoft · 2021-08-06T17:35:50Z

@ShobhitAd fixed in c011a9c
Sorry, forgot to fetch this repo

ydementieiev and others added 14 commits June 4, 2020 15:46

Add Reset Timeout Handler, remove deprecated functionality, create Re…

1011d00

…quest Controller interface.

fixup! Add Reset Timeout Handler, remove deprecated functionality, cr…

97e5527

…eate Request Controller interface.

Merge branch 'develop' into feature/restructuring_OnResetTimeout

95225be

Fix compilation errors after merge of develop branch

dd59333

Remove deprecated functions from SubtleAlert request and remove redun…

91b4dfc

…dant UTs

Add changes to HMI API

cb3cf2a

Increase timeout in case if SIVD need Consent popup

493b960

Increase timeout for ButtonPress request

266abf2

Restrict timeout for Subtle Alert with soft buttons

70a4e4f

update timeout for hmi request

970427a

Merge pull request #187 from LuxoftSDL/fix/OnResetTimeout_Notification

1b7415d

[SDL-0189] Fix onResetTimeout twice notification

Merge pull request #188 from LuxoftSDL/fix/restrict_timeout_for_subtl…

943b5de

…e_alert_with_soft_buttons [SDL-0189] Restrict timeout for subtle alert with soft buttons

careful stopping of the request controller

57d7846

Merge pull request #190 from LuxoftSDL/fix/crash_SDL_on_close

c9ae084

[SDL-0189] fix crash sdl_core on close

ychernysheva mentioned this pull request Jun 23, 2021

Feature/Restructuring OnResetTimeout #2754

Closed

3 tasks

atiwari9 suggested changes Jun 24, 2021

View reviewed changes

Fix OnResetTimeout for Alerts with soft buttons (#192)

aef4c59

* Do not manage requests with NULL timeout

atiwari9 mentioned this pull request Jun 25, 2021

[SDL-0189] Scripts for Restructuring OnResetTimeout feature - Updates for 8.0 release smartdevicelink/sdl_atf_test_scripts#2562

Merged

1 task

LitvinenkoIra mentioned this pull request Jun 25, 2021

Feature/sdl 0189 restructuring on reset timeout smartdevicelink/generic_hmi#411

Merged

7 tasks

AKalinich-Luxoft added 3 commits June 28, 2021 10:55

Revert double timeouts from core

f27c548

Apply smaller timeout to requests

fd71265

Merge pull request #193 from LuxoftSDL/fix/restructuring_onresettimeo…

25c3fb1

…ut_remove_default_timeout_adjustments Remove default timeout adjustments from core

atiwari9 approved these changes Jun 29, 2021

View reviewed changes

ShobhitAd requested changes Jul 16, 2021

View reviewed changes

Address Livio comments

57e4acc

AKalinich-Luxoft mentioned this pull request Jul 20, 2021

Fix/ccb 3141 change command request inheritance structure #3591

Merged

3 tasks

ShobhitAd approved these changes Jul 26, 2021

View reviewed changes

dboltovskyi mentioned this pull request Jul 30, 2021

Increase default timeout smartdevicelink/sdl_atf#235

Merged

AKalinich-Luxoft added 2 commits July 30, 2021 14:47

Add timeout compensation parameter

031c65f

Update default timeout logic accordingly for all commands. Added new file to ini file.

Fix affected unit tests

d63e933

ValeriiMalkov mentioned this pull request Aug 2, 2021

Feature/sdl 0189 restructuring on reset timeout smartdevicelink/sdl_hmi#575

Merged

1 task

ShobhitAd requested changes Aug 5, 2021

View reviewed changes

Address Livio comments

55d7500

AKalinich-Luxoft added 2 commits August 6, 2021 09:58

Merge remote-tracking branch 'origin/develop' into feature/restructur…

3867df8

…ing_OnResetTimeout

Update rpc_spec

228dbe2

Update rpc_spec one more time

c011a9c

ShobhitAd approved these changes Aug 6, 2021

View reviewed changes

ShobhitAd merged commit 2532805 into smartdevicelink:develop Aug 11, 2021

This was referenced Aug 11, 2021

[SDL 0189] Restructuring OnResetTimeout #2422

Closed

SDL doesn't increase the timeout if the driver’s permission pop up is needed [GetInteriorVehicleDataConsent] #2829

Closed

iCollin mentioned this pull request Aug 18, 2021

Remove unnecessary default_timeout_ adjustments after 0189 #3759

Closed

dboltovskyi mentioned this pull request Sep 23, 2021

SDL must start <defaultTimeout> for VR.PerformInteraction #1926

Closed

9 tasks

VjKlepikov mentioned this pull request Mar 30, 2022

Test fail during Reregister App resumption in script 013_resume_one_rpc_late_response_unexpected_disconnect.lua smartdevicelink/sdl_atf_test_scripts#2652

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDL-0189] Restructuring OnResetTimeout #3726

[SDL-0189] Restructuring OnResetTimeout #3726

ychernysheva commented Jun 23, 2021

atiwari9 left a comment

atiwari9 Jun 24, 2021

AKalinich-Luxoft Jun 24, 2021

atiwari9 Jun 28, 2021

atiwari9 Jun 24, 2021

AKalinich-Luxoft Jun 24, 2021

atiwari9 Jun 25, 2021

AKalinich-Luxoft Jun 25, 2021

atiwari9 Jun 24, 2021

AKalinich-Luxoft Jun 24, 2021

atiwari9 Jun 25, 2021

atiwari9 Jun 24, 2021

AKalinich-Luxoft Jun 24, 2021

atiwari9 Jun 25, 2021

AKalinich-Luxoft commented Jun 24, 2021

atiwari9 commented Jun 24, 2021

AKalinich-Luxoft commented Jun 25, 2021

atiwari9 commented Jun 25, 2021

AKalinich-Luxoft commented Jun 29, 2021

atiwari9 left a comment

LitvinenkoIra commented Jun 29, 2021

AKalinich-Luxoft commented Jul 23, 2021

iCollin commented Jul 28, 2021

AKalinich-Luxoft commented Jul 30, 2021

ShobhitAd commented Aug 6, 2021

AKalinich-Luxoft commented Aug 6, 2021

ShobhitAd commented Aug 6, 2021 •

edited

Loading

AKalinich-Luxoft commented Aug 6, 2021

		@@ -54,6 +54,21 @@ ButtonPressRequest::ButtonPressRequest(

		ButtonPressRequest::~ButtonPressRequest() {}

		bool ButtonPressRequest::Init() {

		@@ -54,6 +54,22 @@ SetInteriorVehicleDataRequest::SetInteriorVehicleDataRequest(

		SetInteriorVehicleDataRequest::~SetInteriorVehicleDataRequest() {}

[SDL-0189] Restructuring OnResetTimeout #3726

[SDL-0189] Restructuring OnResetTimeout #3726

Conversation

ychernysheva commented Jun 23, 2021

Risk

Testing Plan

Summary

Changelog

Breaking Changes

CLA

atiwari9 left a 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

AKalinich-Luxoft commented Jun 24, 2021

atiwari9 commented Jun 24, 2021

AKalinich-Luxoft commented Jun 25, 2021

atiwari9 commented Jun 25, 2021

AKalinich-Luxoft commented Jun 29, 2021

atiwari9 left a comment

Choose a reason for hiding this comment

LitvinenkoIra commented Jun 29, 2021

AKalinich-Luxoft commented Jul 23, 2021

iCollin commented Jul 28, 2021

AKalinich-Luxoft commented Jul 30, 2021

ShobhitAd commented Aug 6, 2021

AKalinich-Luxoft commented Aug 6, 2021

ShobhitAd commented Aug 6, 2021 • edited Loading

AKalinich-Luxoft commented Aug 6, 2021

ShobhitAd commented Aug 6, 2021 •

edited

Loading