Rebase#155
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.
| 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.
| 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 99 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_voltagePercentage = std::clamp(newValue[2], uint8_t(0), uint8_t(100)); // Clamp percentage between 0 - 100 | ||
|
|
There was a problem hiding this comment.
This function uses std::clamp, but this translation unit doesn’t include <algorithm>. Relying on transitive includes is brittle and can break builds depending on include order; add the correct header include.
| char *to_string() const; | ||
| uint8_t get_api_revision() const { return api_revision_; } |
There was a problem hiding this comment.
to_string() returns a raw char* from json_dumps(), which requires the caller to free() it. This ownership contract isn’t documented here and is easy to misuse/leak; consider returning std::string (as before) or a RAII wrapper, or at minimum document that the caller owns the returned buffer.
| 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(); |
There was a problem hiding this comment.
ctrlm_input_event_writer::get_meta_data() uses sysfs_name_ to build a sysfs path but doesn’t validate that the writer is initialized (or that sysfs_name_ is non-empty). If this is called before init(), it will probe an invalid path and log misleading errors. Add an initialized_/sysfs_name_.empty() guard at the start and return false (or initialize on-demand).
| Internal slot called when a notification from the remote device is sent | ||
| due to raw battery voltage changing. | ||
| */ | ||
| void GattRemoteControlService::onRawBatteryVoltageChanged(const std::vector<uint8_t> &newValue) { |
There was a problem hiding this comment.
onRawBatteryVoltageChanged() indexes newValue[0..2] without validating the notification payload length. Notifications can be malformed/short; add a size check (e.g., require at least 3 bytes) before indexing to avoid out-of-bounds reads.
| void GattRemoteControlService::onRawBatteryVoltageChanged(const std::vector<uint8_t> &newValue) { | |
| void GattRemoteControlService::onRawBatteryVoltageChanged(const std::vector<uint8_t> &newValue) { | |
| if (newValue.size() < 3) { | |
| XLOGD_ERROR("Invalid raw battery voltage notification length %zu, expected at least 3 bytes", newValue.size()); | |
| return; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 99 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char *ctrlm_rcp_ipc_net_status_t::to_string() const | ||
| { | ||
| return json_dumps(to_json(), JSON_ENCODE_ANY); | ||
| } |
There was a problem hiding this comment.
ctrlm_rcp_ipc_net_status_t::to_string() returns a raw char* from json_dumps(to_json(), ...) without freeing the json_t* returned by to_json(). This leaks both the dumped string (unless every caller free()s it) and the JSON object (never json_decref()d). Prefer returning a std::string (or otherwise documenting/centralizing ownership) and ensure the json_t* is decref'd after dumping.
| if(this->call_plugin_string("getLastWakeupReason", (void *)¶ms, &response)) { | ||
| if(response == "VOICE") { | ||
| wakeup_reason_voice = true; | ||
| } | ||
| } else { | ||
| XLOGD_ERROR("getLastWakeupReason call failed"); | ||
| } | ||
| sem_post(&this->semaphore); | ||
|
|
||
| XLOGD_DEBUG("voice_wakeup is %s", wakeup_reason_voice?"TRUE":"FALSE"); | ||
|
|
There was a problem hiding this comment.
getLastWakeupReason handling changed from a prefix check (strncmp(...,"VOICE",5)) to an exact match (response == "VOICE"). If the plugin returns values like "VOICE_*" (or any extended reason string), this will incorrectly report false. Consider preserving the previous prefix/starts-with behavior (or otherwise handling the full set of possible wakeup reason strings).
| // <server_message> - server message. | ||
| // <result> - flag to indicate if session was successful. | ||
| #define MARKER_VOICE_SESSION_STATS "ctrlm.voice.session.stats" | ||
| #define MARKER_VOICE_SESSION_STATS_VERSION "1" | ||
| #define MARKER_VOICE_SESSION_STATS_VERSION "2" |
There was a problem hiding this comment.
MARKER_VOICE_SESSION_STATS_VERSION was bumped to 2, but the marker field documentation above still describes the older payload (it ends at <result>). Please update the field list to reflect the new event layout (additional end reasons / protocol return code) so telemetry consumers can correctly parse v2.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 105 out of 106 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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; |
| if (value.size() == 3) { | ||
| onRawBatteryVoltageChanged(value); | ||
| } else { | ||
| XLOGD_ERROR("Raw battery voltage received has invalid length (%d bytes)", value.size()); |
| using namespace std; | ||
|
|
||
| #define OTA_MAX_RETRIES (2) | ||
| #define OTA_MAX_RETRIES (0) |
| if(sem_result == -1) { | ||
| if(errno == ETIMEDOUT) { | ||
| XLOGD_ERROR("Timeout waiting for %s initialization", name_get()); | ||
| } else if(errno == EINTR) { | ||
| XLOGD_ERROR("Interrupted while waiting for %s initialization", name_get()); | ||
| } else { | ||
| int errsv = errno; | ||
| XLOGD_ERROR("Error waiting for %s initialization <%s>", name_get(), strerror(errsv)); | ||
| } | ||
| init_result_ = CTRLM_HAL_RESULT_ERROR; | ||
| } else { | ||
| sem_destroy(&semaphore_); | ||
| } |
| char *ctrlm_rcp_ipc_upgrade_status_t::to_string() const | ||
| { | ||
| return json_dumps(to_json(), JSON_ENCODE_ANY); | ||
| } |
| char *ctrlm_rcp_ipc_validation_status_t::to_string() const | ||
| { | ||
| return json_dumps(to_json(), JSON_ENCODE_ANY); | ||
| } |
| 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; } |
|
|
||
| m_end_reason_stream = 0; | ||
| m_end_reason_protocol = 0; | ||
| m_end_reason_rcu = 0; | ||
| m_end_reason_session = 0; | ||
| m_end_reason_server = 0; | ||
| m_result = false; | ||
| m_ret_code_protocol = 0; |
| 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; |
| if(cert_valid) { | ||
| nopoll_conn_opts_set_ssl_protocol(opts, NOPOLL_METHOD_TLSV1_2); | ||
| nopoll_conn_opts_ssl_host_verify(opts, nopoll_false); //localhost will not match host specified in certificate | ||
|
|
||
| if(!nopoll_conn_opts_set_ssl_certs(opts, &tmp_cert[0], &tmp_cert[0], NULL, NULL)) { | ||
| XLOGD_ERROR("Failed to add cert/key files to nopoll_conn"); | ||
| nopoll_ctx_unref(g_ctrlmf_ws.nopoll_ctx); | ||
| nopoll_conn_opts_free(opts); | ||
| return(NULL); | ||
| } |
| 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; } |
| using namespace std; | ||
|
|
||
| #define OTA_MAX_RETRIES (2) | ||
| #define OTA_MAX_RETRIES (0) | ||
|
|
||
| ctrlm_obj_controller_t::ctrlm_obj_controller_t(ctrlm_controller_id_t controller_id, ctrlm_obj_network_t &network, unsigned long long ieee_address) : |
| if(verbose) { | ||
| XLOGD_INFO_OPTS(XLOG_OPTS_DEFAULT, 20 * 1024, "Loading Configuration for <%s> <%s>", file_path.c_str(), contents.c_str()); | ||
| } else { | ||
| XLOGD_INFO("Loading Configuration for <%s>", file_path.c_str()); | ||
| } | ||
| this->root = json_loads(contents.c_str(), JSON_REJECT_DUPLICATES, &json_error); | ||
| if(this->root != NULL) { | ||
| XLOGD_INFO("config loaded successfully as JSON"); | ||
| if(verbose) { | ||
| XLOGD_INFO("config loaded successfully as JSON for <%s>", file_path.c_str()); | ||
| } | ||
| 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()); | ||
| } |
| 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()); | ||
|
|
|
|
||
| #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) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 114 out of 115 changed files in this pull request and generated 16 comments.
Comments suppressed due to low confidence (3)
src/ipc/ctrlm_rcp_ipc_event.cpp:192
- This has the same ownership problem as the other
to_stringhelpers:json_dumpsreturns heap memory and the call sites logmsg.to_string()without freeing it (for examplectrlm_obj_network_t::iarm_event_rcu_firmware_status). Each firmware-status log will leak the allocated JSON string.
src/ipc/ctrlm_rcp_ipc_event.cpp:239 - This returns a heap-allocated JSON string with no ownership contract, and the existing validation-status caller logs
msg.to_string()without freeing it. That leaks memory for every validation event; keep the previousstd::stringownership model or ensure callers free the returned buffer.
src/factory/ctrlmf_ws.c:242 - If listener creation fails, the thread returns before
sem_post(params.semaphore), which leavesctrlmf_ws_init()blocked indefinitely waiting for initialization. This path should report failure to the waiting caller (and remove the temp cert when one was created) before exiting.
| #define BLE_RCU_ID_RANGE_MIN (NETWORK_ID_BASE_BLE) | ||
| #define BLE_RCU_ID_RANGE_MAX ((BLE_RCU_ID_RANGE_MIN)+BLE_MAX_MANAGED_RCUS+1) // +1 as a buffer for pairing |
| void GattRemoteControlService::onRawBatteryVoltageChanged(const std::vector<uint8_t> &newValue) { | ||
| m_unloadedVoltage = newValue[0]; | ||
| m_loadedVoltage = newValue[1]; | ||
| m_voltagePercentage = std::clamp(newValue[2], uint8_t(0), uint8_t(100)); // Clamp percentage between 0 - 100 |
| typedef struct { | ||
| unsigned char api_revision; ///< Revision of this API | ||
| ctrlm_network_id_t network_id; ///< IN - Identifier of network | ||
| unsigned int key_code; ///< IN - Key code from device | ||
| unsigned int pair_code; ///< IN - Pairing code from device |
| #define CTRLM_VOICE_SESSION_MSG_MAX_LENGTH (128) ///< Session message string maximum length | ||
| #define CTRLM_VOICE_QUERY_STRING_MAX_LENGTH (128) ///< Query string maximum name or value length | ||
| #define CTRLM_VOICE_QUERY_STRING_MAX_PAIRS (16) ///< Query string maximum number of name/value pairs | ||
| #define CTRLM_VOICE_QUERY_STRING_MAX_PAIRS (24) ///< Query string maximum number of name/value pairs |
| virtual ~ctrlm_voice_t(); | ||
|
|
||
| ctrlm_voice_session_response_status_t voice_session_req(ctrlm_network_id_t network_id, ctrlm_controller_id_t controller_id, ctrlm_voice_device_t device_type, ctrlm_voice_format_t format, voice_session_req_stream_params *stream_params, const char *controller_name, const char *sw_version, const char *hw_version, double voltage, bool command_status=false, ctrlm_timestamp_t *timestamp=NULL, ctrlm_voice_session_rsp_confirm_t *cb_confirm=NULL, void **cb_confirm_param=NULL, bool use_external_data_pipe=false, bool press_and_hold=true, const char *transcription_in=NULL, const char *audio_file_in=NULL, const uuid_t *uuid = NULL, bool low_latency=false, bool low_cpu_util=false, int audio_fd = -1); | ||
| ctrlm_voice_session_response_status_t voice_session_req(ctrlm_network_id_t network_id, ctrlm_controller_id_t controller_id, ctrlm_voice_device_t device_type, ctrlm_voice_format_t format, voice_session_req_stream_params *stream_params, const char *controller_name, const char *sw_version, const char *hw_version, double voltage, bool command_status=false, ctrlm_timestamp_t *timestamp=NULL, ctrlm_voice_session_rsp_confirm_t *cb_confirm=NULL, void **cb_confirm_param=NULL, bool use_external_data_pipe=false, bool press_and_hold=true, std::function<void(ctrlm_voice_start_audio_params_t *)> cb_start_audio=NULL, ctrlm_voice_start_audio_params_t *cb_audio_start_params=NULL, const char *transcription_in=NULL, const char *audio_file_in=NULL, const uuid_t *uuid = NULL, bool low_latency=false, bool low_cpu_util=false, int audio_fd = -1); |
| #include "ctrlm_voice_telemetry_events.h" | ||
|
|
||
| #ifdef BEEP_ON_KWD_ENABLED | ||
| #include "ctrlm_thunder_plugin_system_audio_player.h" |
| CTRLM_VOICE_DEVICE_MICROPHONE, | ||
| #ifdef CTRLM_LOCAL_MIC_TAP | ||
| CTRLM_VOICE_DEVICE_MICROPHONE_TAP, |
| case CTRLM_VOICE_DEVICE_MICROPHONE: | ||
| #endif | ||
| case CTRLM_VOICE_DEVICE_FF: { | ||
| url = &this->prefs.server_url_src_ff; | ||
| break; | ||
| } | ||
| #ifdef CTRLM_LOCAL_MIC_TAP | ||
| case CTRLM_VOICE_DEVICE_MICROPHONE_TAP: { | ||
| url = &this->prefs.server_url_src_mic_tap; | ||
| break; |
| g_ctrlm_db.deepsleep_close_db = true; // Default to true. This can be overridden by vendor layer config as required in the future. | ||
|
|
||
| if(g_ctrlm_db.deepsleep_close_db) { | ||
| errno_t safec_rc = strcpy_s(g_ctrlm_db.path, sizeof(g_ctrlm_db.path), db_path); | ||
| ERR_CHK(safec_rc); | ||
| sem_init(&g_ctrlm_db.ds_signal, 0, 0); | ||
| } |
| -DBLE_ENABLED=OFF \ | ||
| -DRF4CE_ENABLED=OFF \ |
No description provided.