Rebase#155
Open
muraliselvaraj2020 wants to merge 78 commits into
Open
Conversation
…C_DISABLE_VIA_PRIVACY (#120)
* RDKEMW-8929: Refactor base ipc class Reason for change: Test Procedure: Risks: Signed-off-by: Kelvin Lu <Kelvin_Lu@comcast.com> * Updating based on Copilot review comments --------- Signed-off-by: Kelvin Lu <Kelvin_Lu@comcast.com>
There was a problem hiding this comment.
Pull request overview
This pull request represents a major "rebase" effort that refactors the codebase to remove numerous compile-time flags and replace them with runtime configuration. The changes focus on voice control features, power management, and device management infrastructure.
Changes:
- Removes compile-time flags (CTRLM_LOCAL_MIC, CTRLM_LOCAL_MIC_TAP, BEEP_ON_KWD_ENABLED, NETWORKED_STANDBY_MODE_ENABLED, etc.) and replaces them with runtime checks
- Refactors IPC/IARM infrastructure to use a common base class with API revision management
- Updates logging to use "AUTOMATION" variants of logging macros throughout the codebase
- Adds controller ID range management for RF4CE and BLE networks
- Introduces callback mechanism for audio streaming initiation
- Updates factory test infrastructure to support dynamic audio analysis plugins
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/voice/ipc/ctrlm_voice_ipc_iarm_thunder.cpp | Refactors voice IPC to use runtime feature detection, adds API revision setting |
| src/voice/ctrlm_voice_obj.cpp | Replaces compile flags with runtime configuration for local mic and beep features |
| src/voice/ctrlm_voice_obj.h | Adds new member variables for runtime feature flags and audio start callback support |
| src/rf4ce/ctrlm_rf4ce_network.cpp | Major refactoring to extract audio streaming logic, add controller ID range management |
| src/ble/ctrlm_ble_network.cpp | Similar refactoring for BLE audio streaming, adds controller ID migration logic |
| src/ipc/ctrlm_rcp_ipc_iarm_thunder.cpp | Updates to use new broadcast_iarm_event signatures and API revision management |
| src/ipc/ctrlm_ipc_iarm.h/cpp | Adds API revision management and refactors event broadcasting |
| src/factory/.c/.cpp | Updates factory test infrastructure for dynamic plugin loading |
| CMakeLists.txt | Removes numerous build options and simplifies configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if(it->response != CTRLM_THREAD_MONITOR_RESPONSE_ALIVE) { | ||
| XLOGD_TELEMETRY("Thread %s is unresponsive", it->name); | ||
| XLOGD_AUTOMATION_TELEMETRY("Thread %s is unresponsive", it->name); |
There was a problem hiding this comment.
The logging macro XLOGD_AUTOMATION_TELEMETRY is being used but may not be defined. This could cause compilation errors. Verify that this logging macro is properly defined in the logging infrastructure.
Comment on lines
+205
to
+209
| if (it.second == CTRLM_IARM_CALL_RESULT_SUCCESS) { | ||
| return true; | ||
| } | ||
| } | ||
| return true; | ||
| return false; |
There was a problem hiding this comment.
The logic change for result checking appears to be inverted. The original code returned true if all results were SUCCESS and false if any failed. The new code returns true if ANY result is SUCCESS and false if all failed. This could cause incorrect behavior when processing multiple network results.
Comment on lines
+770
to
+771
| false, NULL, NULL, NULL, (fd >= 0) ? true : false, true, NULL, NULL, | ||
| str_transcription.empty() ? NULL : str_transcription.c_str(), str_audio_file.empty() ? NULL : str_audio_file.c_str(), &request_uuid, request_config.low_latency, request_config.low_cpu_util, fd); |
There was a problem hiding this comment.
The function ctrlm_voice_ipc_iarm_thunder_t::voice_session_request has a complex function call signature with many parameters. Lines 770-771 add two NULL parameters before the transcription and audio file parameters. This changes the function signature but it's not clear if all call sites have been updated correctly. Verify that the function parameter order matches the declaration in the header file.
Comment on lines
79
to
82
| ret = true; | ||
| } else { | ||
| XLOGD_ERROR("JSON ERROR: Line <%u> Column <%u> Text <%s>", json_error.line, json_error.column, json_error.text); | ||
| XLOGD_ERROR("JSON ERROR: Line <%u> Column <%u> Text <%s> Contents <%s>", json_error.line, json_error.column, json_error.text, contents.c_str()); | ||
| } |
Comment on lines
+175
to
184
| cert_key_fp = fdopen(cert_key_fd, "w"); | ||
| if(cert_key_fp == NULL) { | ||
| err_store = errno; | ||
| XLOGD_ERROR("failed to remove temp cert <%s>", strerror(err_store)); | ||
| XLOGD_ERROR("fdopen failed: <%s>", strerror(err_store)); | ||
| if(0 != unlink(&tmp_cert[0])) { | ||
| err_store = errno; | ||
| XLOGD_ERROR("failed to remove temp cert <%s>", strerror(err_store)); | ||
| } | ||
| break; | ||
| } |
Comment on lines
+114
to
+116
| // <end_reason_rcu> - audio stream success/error code. | ||
| // <end_reason_session> - session success/error code. | ||
| // <end_reason_server> - server success/error code. |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
| #ifdef ANSI_CODES_DISABLED | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL) | ||
| #else | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL | XLOG_OPTS_COLOR) |
Comment on lines
+237
to
+240
| char *ctrlm_rcp_ipc_validation_status_t::to_string() const | ||
| { | ||
| return json_dumps(to_json(), JSON_ENCODE_ANY); | ||
| } |
| #ifdef ANSI_CODES_DISABLED | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL) | ||
| #else | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL | XLOG_OPTS_COLOR) |
Comment on lines
+114
to
+130
| bool has_valid_cert = false; | ||
| ctrlmf_ws_init(audio_frame_size, CTRLMF_WS_PORT_INT, true, &g_audio_cap.callbacks, &has_valid_cert); | ||
|
|
||
| // Set the FFV url to our websocket | ||
| std::string url_new = has_valid_cert ? CTRLMF_WSS_URL : CTRLMF_WS_URL; | ||
|
|
||
| if(g_audio_cap.use_mic_tap) { | ||
| if(!g_audio_cap.obj_ctrlm->configure_voice_mic_tap(url_new, true)) { | ||
| XLOGD_ERROR("unable to set mic tap url"); | ||
| return(false); | ||
| } | ||
| } else { | ||
| if(!g_audio_cap.obj_ctrlm->configure_voice_hf(url_new, true)) { | ||
| XLOGD_ERROR("unable to set hf url"); | ||
| return(false); | ||
| } | ||
| } |
Comment on lines
175
to
+179
| bool ctrlm_input_event_writer::get_meta_data(struct stat &file_meta_data) { | ||
| int ret = fstat(fd_, &file_meta_data); | ||
| std::ostringstream oss; | ||
| oss << "/sys/devices/virtual/input/" << sysfs_name_; | ||
| std::string dir_path = oss.str(); | ||
| XLOGD_DEBUG("virtual input path = %s", dir_path.c_str()); |
Comment on lines
+190
to
+193
| char *ctrlm_rcp_ipc_upgrade_status_t::to_string() const | ||
| { | ||
| return json_dumps(to_json(), JSON_ENCODE_ANY); | ||
| } |
Comment on lines
23
to
26
| #include <stdio.h> | ||
|
|
||
| #ifdef ANSI_CODES_DISABLED | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL) | ||
| #else | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL | XLOG_OPTS_COLOR) | ||
| #endif | ||
|
|
Comment on lines
120
to
122
| ret = true; | ||
| XLOGD_DEBUG("EDID data <%d> bytes: \n<%s>", edid_size, ss.str().c_str()); | ||
| XLOGD_INFO("EDID data <%d> bytes: \n<%s>", edid_size, ss.str().c_str()); | ||
| } else { |
Comment on lines
+78
to
82
| this->local_config = local_config; | ||
| ret = true; | ||
| } else { | ||
| XLOGD_ERROR("JSON ERROR: Line <%u> Column <%u> Text <%s>", json_error.line, json_error.column, json_error.text); | ||
| XLOGD_ERROR("JSON ERROR: Line <%u> Column <%u> Text <%s> Contents <%s>", json_error.line, json_error.column, json_error.text, contents.c_str()); | ||
| } |
Comment on lines
+104
to
+108
| } | ||
| json_t *append_root = json_loads(contents.c_str(), JSON_REJECT_DUPLICATES, &json_error); | ||
| if(append_root == NULL) { | ||
| XLOGD_ERROR("JSON ERROR: Line <%u> Column <%u> Text <%s> Contents <%s>", json_error.line, json_error.column, json_error.text, contents.c_str()); | ||
| } else { |
Comment on lines
112
to
+116
| virtual json_t *to_json() const; | ||
|
|
||
| uint8_t get_api_revision() const { return api_revision_; } | ||
| bool get_result() const { return (result_ == CTRLM_IARM_CALL_RESULT_SUCCESS) ? true : false; } | ||
| void set_result(ctrlm_iarm_call_result_t result) { result_ = result; } | ||
| ctrlm_network_id_t get_net_id() const { return net_id_; } | ||
| void set_net_id(ctrlm_network_id_t net_id) { net_id_ = net_id; } | ||
| void populate_status(const ctrlm_obj_network_t &network); | ||
| char *to_string() const; | ||
| uint8_t get_api_revision() const { return api_revision_; } | ||
| bool get_result() const { return (result_ == CTRLM_IARM_CALL_RESULT_SUCCESS) ? true : false; } |
Comment on lines
200
to
204
| bool get_result(void) { | ||
| if (network_id_ != CTRLM_MAIN_NETWORK_ID_ALL) { | ||
| return (result_map_[network_id_] == CTRLM_IARM_CALL_RESULT_SUCCESS); | ||
| } else { | ||
| for (const auto &it : result_map_) { |
| #ifdef ANSI_CODES_DISABLED | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL) | ||
| #else | ||
| #define XLOG_OPTS_DEFAULT (XLOG_OPTS_DATE | XLOG_OPTS_TIME | XLOG_OPTS_LF | XLOG_OPTS_MOD_NAME | XLOG_OPTS_LEVEL | XLOG_OPTS_COLOR) |
| } | ||
| ret = true; | ||
| XLOGD_DEBUG("EDID data <%d> bytes: \n<%s>", edid_size, ss.str().c_str()); | ||
| XLOGD_INFO("EDID data <%d> bytes: \n<%s>", edid_size, ss.str().c_str()); |
Comment on lines
+209
to
+213
| ss << m_end_reason_rcu << ","; | ||
| ss << m_end_reason_session << ","; | ||
| ss << m_end_reason_server << ","; | ||
| ss << "\"" << m_server_message << "\","; | ||
| ss << m_result << "]]"; | ||
| ss << m_result << ","; |
Comment on lines
+317
to
+321
| m_end_reason_rcu = 0; | ||
| m_end_reason_session = 0; | ||
| m_end_reason_server = 0; | ||
| m_result = false; | ||
| m_ret_code_protocol = 0; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.