Feature/sdl 0315 add rpc conflict management

[Sohei-Suzuki-Nexty]

Fixes #3670

This PR is [ready] for review.

Risk

This PR makes [no] API changes.

Testing Plan

ATF scripts ※There are some errors because it is incomplete.

Summary

This is a changes made by SDL 0315 Add rpc conflict management to SDL core.

Tasks Remaining:

Changes due to the addition of a new policy table.

CLA

• [x] I have signed the CLA

[Sohei-Suzuki-Nexty]

Hi, @iCollin -san I have asked a question on slack before. I'm trying to add a new policy "interrupt_manager_config" in the change of sdl0315, but it doesn't load well. We have created a PR, so please check the code.

[iCollin]

@Sohei-Suzuki-Nexty Hello, I am not able to compile this PR in current state.

It seems that many methods are not defined on their interfaces, meaning clients of such interfaces cannot use these methods. For example, pure virtuals for GetRpcPriority, GetAppPriority and GetHmiStatusPriority should be added to policy_manager.h, while a pure virtual GetInterruptManager should be added to application_manager.h.

I'd be happy to help you look into why values are not loading from the preloaded policy table once this PR is able to compile. If you need help with a specific compilation error feel free to reach out to me on the SDL Slack channel, and once this PR is in a good state I can look into loading problems here on this PR.

Thank you, Collin

[Sohei-Suzuki-Nexty]

Hi, @iCollin -san I'm sorry. There were not enough files committed. Added the missing modified file. I think you can compile with this, so please check the code.

[iCollin]

Hello @Sohei-Suzuki-Nexty,

Thank you for the updates. After some testing, it seems to me that the issue is SaveInterruptManagerConfig is not called in regular policy mode. With that added, I see the SQL query fail, I believe because the parameters are not bound to the query.

Hopefully this is what you were looking for! Let me know if I can help further.

[Sohei-Suzuki-Nexty]

Hi, @iCollin -san Thank you for your confirmation. When I added SaveInterruptManagerConfig and checked it, the following error was confirmed in UnitTest.

・SQLPTRepresentationTest.GenerateSnapshot_DefaultContentOfModuleMeta_MetaInfoPresentInSnapshot ・SQLPTRepresentationTest.GenerateSnapshot_SetMetaInfo_NoSoftwareVersionInSnapshot ・SQLPTRepresentationTest.GenerateSnapshot_SetHardwareVersion_NoHardwareVersionInSnapshot

You say "I see the SQL query fail, I believe because the parameters are not bound to the query.", but I'm not familiar with SQL so I haven't been able to identify where the problem is.

In "sql_pt_queries.cc", I added the parameter as well as other parameters. Is there a problem here? I would like to know more specifically.

[iCollin]

Hello @Sohei-Suzuki-Nexty,

Some SQL knowledge will be required to implement this feature. I took another look and I can see the reason the update statement in SaveInterruptManagerConfig is failing currently is because there is no table interrupt_manager_config which the update statement is trying to modify. After the prepare statement, the parameters of the query need to be bound (replacing the ? values) in order for query.Exec to succeed.

[Sohei-Suzuki-Nexty]

@iCollin -san Thank you for your frequent answers. Could you tell me a little more about it?

there is no table interrupt_manager_config which the update statement is trying to modify.

Regarding the above, it is necessary to add a process such as CREATE TABLE IF NOT EXISTS interrupt_manager_config to the kCreateSchemaset in sql_pt_queries.cc, in order to create the interrupt_manager_config table. Is my understanding correct?

After the prepare statement, the parameters of the query need to be bound (replacing the? Values) in order for query.Exec to succeed.

Also, about the above, I do not know what kind of parameter setting is appropriate.

I would like to load the following table that I added to sdl_preloaded_pt.json, but could you tell me what specific modifications are needed to use it?

"interrupt_manager_config":
    {
      "rpc_priority":
      {
          "DialNumber": 1,
          "Alert": 2,
          "PerformAudioPassThru": 2,
          "PerformInteraction": 3,
          "ScrollableMessage": 3,
          "Slider": 3,
          "Speak": 3
      },
      "app_priority":
      {
          "EMERGENCY": 0, 
          "NAVIGATION": 1,
          "VOICE_COMMUNICATION": 2,
          "COMMUNICATION": 3,
          "NORMAL": 4,
          "NONE": 5
      },
      "hmi_status_priority":
      {
          "FULL": 1,
          "LIMITED": 2,
          "BACKGROUND": 3,
          "NONE": 4
      }
  }

[Sohei-Suzuki-Nexty]

@iCollin -san Could you confirm the comment above? We need to resolve this issue as soon as possible. If it takes a long time to answer, can you let me know when you can answer? We apologize for the inconvenience, but thank you for your cooperation.

[iCollin]

Hello @Sohei-Suzuki-Nexty,

My apologies I was on vacation this past week. I will be OOO 11/24 - 11/28 as well.

Regarding the above, it is necessary to add a process such as CREATE TABLE IF NOT EXISTS interrupt_manager_config to the kCreateSchemaset in sql_pt_queries.cc , in order to create the interrupt_manager_config table. Is my understanding correct?

In order for queries to work in the current state, yes that would fix the issue of saying UPDATE, INSERT, etc on a table that does not exist. However, the purpose for this table interrupt_manager_config is not clear to me, what information would it contain?

could you tell me what specific modifications are needed to use it?

Unfortunately I am not budgeted time for this feature. I can answer direct questions as part of Slack Support, but I cannot implement this feature at this time. It may be helpful to follow the implementation of the table notifications_per_minute_by_priority for these new priority tables.

[Sohei-Suzuki-Nexty]

@iCollin -san Thank you for your answer.

However, the purpose for this table interrupt_manager_config is not clear to me, what information would it contain?

The table interrupt_manager_config is a table that holds the priority of RPC because it determines the priority on Core for simultaneous RPCs and sends only the RPC with higher priority to HMI. It is added as a table that holds the three setting value of rpc_priority, app_priority, and hmi_status_priority.

Each role is as follows rpc_priority: 　Priority setting value for RPC

app_priority: 　Priority setting value by appType 　Used to determine if rpc_priority is the same

hmi_status_priority 　Priority setting value at each hmi level 　Used to determine if app_priority is also the same

The purpose is to be able to send one RPC according to the priority read from interrupt_manager_config when RPCs occur at the same time.

For details, please refer to the PR below. https://github.com/smartdevicelink/sdl_evolution/issues/1068

In our view, we are trying to add interrupt_manager_config at the same level as the existing module_config. I think it is necessary to create an interrupt_manager_config table for the above purpose. Is this idea correct?

It may be helpful to follow the implementation of the table notifications_per_minute_by_priority for these new priority tables.

Thank you for your advice. We will refer to notifications_per_minute_by_priority. If we still have some questions, We would like to add specific questions again.

I have one additional question.

I can answer direct questions as part of Slack Support,

I'd like to confirm it just in case. Does the above mean that you want to communicate on Slack, or is it okay to ask questions on this PR as it is?

[iCollin]

@Sohei-Suzuki-Nexty

I think it is necessary to create an interrupt_manager_config table for the above purpose. Is this idea correct?

Hello, table interrupt_manager_config may be necessary, but I do not understand what values the table will contain? For example, I see this insert statement for the table:

const std::string kInsertInterruptManagerConfig =
    "INSERT INTO `interrupt_manager_config` (`rpc_priority`, `app_priority`, "
    "`hmi_status_priority`) "
    "  VALUES (?, ?, ?)";

What would those values be? Integers? What would they be used for?

My understanding is that the actual priority values are stored in the tables rpc_priority, app_priority and hmi_status_priority.

Does the above mean that you want to communicate on Slack, or is it okay to ask questions on this PR as it is?

My apologies I was trying to explain that is how I characterize my time assisting with this and other features that are not yet in review. I am happy to answer questions here or on slack, it is just easier for me to see the code changes here in a PR instead of Slack.

[Sohei-Suzuki-Nexty]

@iCollin -san Thank you for your answer.

My understanding is that the actual priority values are stored in the tables rpc_priority, app_priority and hmi_status_priority.

Your understanding is correct. The actual priority values are stored in the rpc_priority, app_priority, and hmi_status_priority tables.

The table interrupt_manager_config was considered as a table that puts together three tables, rpc_priority, app_priority, and hmi_status_priority, referring to module_config.

From your reply, I understand that if I get each configuration value directly from the rpc_priority, app_priority, hmi_status_priority tables, I don't need to create an interrupt_manager_config table, is this correct?

[iCollin]

@Sohei-Suzuki-Nexty, My pleasure!

From your reply, I understand that if I get each configuration value directly from the rpc_priority, app_priority, hmi_status_priority tables, I don't need to create an interrupt_manager_config table, is this correct?

That makes sense to me, I am not sure what the additional table would be used for in the database.

[Sohei-Suzuki-Nexty]

@iCollin -san Thank you for your confirmation.

I modify it so that it doesn't create the interrupt_manager_config table. In SaveInterruptManagerConfig, I'd like to fix it only to the process of calling each priority table, is this correct?

bool SQLPTRepresentation::SaveInterruptManagerConfig(
    const policy_table::InterruptManagerConfig& config) {
  SDL_LOG_AUTO_TRACE();

  if (!SaveRpcPriority(config.rpc_priority)) {
    return false;
  }

  if (!SaveAppPriority(config.app_priority)) {
    return false;
  }

  if (!SaveHmiStatusPriority(config.hmi_status_priority)) {
    return false;
  }
  return true;
}

With this fix, the following Unit Test errors will be resolved. ・SQLPTRepresentationTest.GenerateSnapshot_DefaultContentOfModuleMeta_MetaInfoPresentInSnapshot ・SQLPTRepresentationTest.GenerateSnapshot_SetMetaInfo_NoSoftwareVersionInSnapshot ・SQLPTRepresentationTest.GenerateSnapshot_SetHardwareVersion_NoHardwareVersionInSnapshot

However, I haven't been able to get the value well yet. I'm trying to get it as below, but it's failing. Is there a problem here?

rpc :: policy_table_interface_base :: rpc_priority_type
CacheManager :: GetRpcPriority () const {
  SDL_LOG_AUTO_TRACE ();
  sync_primitives :: AutoLock auto_lock (cache_lock_);
  return pt_-> policy_table.interrupt_manager_config.rpc_priority;
} 

I have committed modified files.Please check it.

[Sohei-Suzuki-Nexty]

@iCollin -san I've committed the modified code, did you see it? I would appreciate it if you could check it together with the above comments. I am sorry to trouble you, but I appreciate your support.

[iCollin]

@Sohei-Suzuki-Nexty

In SaveInterruptManagerConfig, I'd like to fix it only to the process of calling each priority table, is this correct?

That looks fine to me, yes

However, I haven't been able to get the value well yet.

Unfortunately I do not have time to debug this at the moment but from the code snippet I see that the value is being accessed as the following: return pt_-> policy_table.interrupt_manager_config.rpc_priority;

Since interrupt_manager_config table was removed, should the access look more like pt_->policy_table.rpc_priority?

I am not sure but I don't know what the member interrupt_manager_config would be with that table removed.

I hope this helps and feel free to reach out with more questions, I will be in the office all next week to answer.

[Sohei-Suzuki-Nexty]

@iCollin - san Thank you for your answer.

Since interrupt_managerconfig table was removed, should the access look more like pt->policy_table.rpc_priority?

When I modified it like pt_-> policy_table.rpc_priority, the following error occurred.

error: ‘struct rpc::policy_table_interface_base::PolicyTable’ has no member named ‘rpc_priority’

I presume that the cause is a problem with the definition method in types.h. Currently, each priority is defined as an element of the InterruptManagerConfig structure.

InterruptManagerConfig
struct InterruptManagerConfig : CompositeType {
 public:
  rpc_priority_type rpc_priority;
  app_priority_type app_priority;
  hmi_status_priority_type hmi_status_priority;

From the current situation, I speculated that if I delete the interrupt_manager_config table, the definition in types should also be changed to a definition for each individual priority, is my guess correct?

[iCollin]

@Sohei-Suzuki-Nexty I do not believe it is necessary to remove interrupt_manager_config from the types definition but it makes sense to me to do this. I don't expect that will fix the value, I suspect there is another problem in the loading process.

[Sohei-Suzuki-Nexty]

@iCollin - san After modifying the type definition and reviewing the reading process with reference to the advice given, I was able to get the expected settings. This solved the problem. Thanks for your cooperation.