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.

Sign up for GitHub

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

Fix response to SetGlobalProperties in case of UNSUPPORTED TTS #1574

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix response to SetGlobalProperties in case of UNSUPPORTED TTS #1574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions ...omponents/application_manager/include/application_manager/commands/command_request_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,8 @@ class CommandRequestImpl : public CommandImpl,
* @return true if result code complies successful result code
* otherwise returns false
*/
bool PrepareResultForMobileResponse(ResponseInfo& out_first,
ResponseInfo& out_second) const;
virtual bool PrepareResultForMobileResponse(ResponseInfo& out_first,
ResponseInfo& out_second) const;

/**
* @brief If message from HMI contains returns this info
Expand All @@ -256,7 +256,7 @@ class CommandRequestImpl : public CommandImpl,
* interface that returns response.
* @return resulting code for sending to mobile application.
*/
mobile_apis::Result::eType PrepareResultCodeForResponse(
virtual mobile_apis::Result::eType PrepareResultCodeForResponse(
const ResponseInfo& first, const ResponseInfo& second);

protected:
Expand Down
23 changes: 23 additions & 0 deletions ...ation_manager/include/application_manager/commands/mobile/set_global_properties_request.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,29 @@ class SetGlobalPropertiesRequest : public CommandRequestImpl {
*/
bool PrepareResponseParameters(mobile_apis::Result::eType& result_code,
std::string& info);
/**
* @brief Checks result code from HMI for splitted RPC
* and returns parameter for sending to mobile app.
* @param ui_info contains result_code from HMI response and
* interface that returns ui response
* @param tts_info contains result_code from HMI response and
* interface that returns tts response
* @return true if result code complies successful result code
* otherwise returns false
*/
bool PrepareResultForMobileResponse(ResponseInfo& ui_info,
ResponseInfo& tts_info) const FINAL;

/**
* @brief Prepare result code for sending to mobile application
* @param ui_info contains result_code from HMI response and
* interface that returns ui response
* @param tts_info contains result_code from HMI response and
* interface that returns tts response.
* @return resulting code for sending to mobile application.
*/
mobile_apis::Result::eType PrepareResultCodeForResponse(
const ResponseInfo& ui_info, const ResponseInfo& tts_info) FINAL;

bool is_ui_send_;
bool is_tts_send_;
Expand Down
35 changes: 24 additions & 11 deletions src/components/application_manager/src/commands/mobile/set_global_properties_request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,30 @@ void SetGlobalPropertiesRequest::on_event(const event_engine::Event& event) {
}
}

bool SetGlobalPropertiesRequest::PrepareResultForMobileResponse(
ResponseInfo& ui_info, ResponseInfo& tts_info) const {
const bool result =
CommandRequestImpl::PrepareResultForMobileResponse(ui_info, tts_info);
if (ui_info.is_ok && tts_info.is_unsupported_resource) {
return true;
}
return result;
}
Copy link

@ghost ghost May 30, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

@pvvasilev Maybe
if (ui_info.is_ok && tts_info.is_unsupported_resource) {
return true;
}
return CommandRequestImpl::PrepareResultForMobileResponse(ui_info, tts_info);

Copy link
Author

Choose a reason for hiding this comment

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

@VProdanov No. The ui_info and tts_info are initialized in the CommandRequestImpl::PrepareResultForMobileResponse call.

Copy link

Choose a reason for hiding this comment

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

ok


mobile_apis::Result::eType
SetGlobalPropertiesRequest::PrepareResultCodeForResponse(
const ResponseInfo& ui_info, const ResponseInfo& tts_info) {
if (ui_info.is_ok && tts_info.is_unsupported_resource) {
return mobile_apis::Result::WARNINGS;
}
if (HmiInterfaces::STATE_AVAILABLE == tts_info.interface_state &&
tts_info.is_unsupported_resource) {
tts_response_info_ = "Unsupported phoneme type sent in a prompt";
Copy link
Contributor

Choose a reason for hiding this comment

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

@pvvasilev please remove this assignment. According to APPLINK-19591 SDL should transfer info string received from HMI response but not generate it by itself.

Copy link
Author

Choose a reason for hiding this comment

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

@AKalinich-Luxoft I do not see any mention of this in the cited ticket.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pvvasilev it's not mentioned but it should. Please also look at related requirement APPLINK-31653 which contains more detailed description. I hope it's applicable for all RPCs which contains TTSChunk element as well.

Copy link
Author

Choose a reason for hiding this comment

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

@AKalinich-Luxoft Again I do not see any mention of this. Please give me an exact requirement specifying this. Otherwise I do not agree with you. Also I think these requirements you are citing are not for the correct project.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pvvasilev these requirements are applicable for GENIVI & Ford-Specific (specified in Commitment field of ticket). In cited APPLINK-31653:

SDL must:
...
respond with "WARNINGS, success:true" + info: <message_from_HMI> to mobile app IN CASE
HMI respond with UNSUPPORTED_RESOURCE

I think it is exactly this case.
And requirement regarding info parameter: SDLOPEN-1309 req 4 and 5.

Copy link
Author

Choose a reason for hiding this comment

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

@this is not the same case. The case you are referring to is the if above this one. There the info is forwarded.

return mobile_apis::Result::WARNINGS;
}
return CommandRequestImpl::PrepareResultCodeForResponse(ui_info, tts_info);
}

bool SetGlobalPropertiesRequest::PrepareResponseParameters(
mobile_apis::Result::eType& result_code, std::string& info) {
LOG4CXX_AUTO_TRACE(logger_);
Expand All @@ -287,17 +311,6 @@ bool SetGlobalPropertiesRequest::PrepareResponseParameters(
HmiInterfaces::HMI_INTERFACE_TTS);
const bool result =
PrepareResultForMobileResponse(ui_properties_info, tts_properties_info);
if (result &&
(HmiInterfaces::STATE_AVAILABLE == tts_properties_info.interface_state) &&
(tts_properties_info.is_unsupported_resource)) {
result_code = mobile_apis::Result::WARNINGS;
tts_response_info_ = "Unsupported phoneme type sent in a prompt";
info = MergeInfos(tts_properties_info,
tts_response_info_,
ui_properties_info,
ui_response_info_);
return result;
}
result_code =
PrepareResultCodeForResponse(ui_properties_info, tts_properties_info);
info = MergeInfos(tts_properties_info,
Expand Down