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 83 out of 84 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 84 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* RDKEMW-14445 : Add session end and protocol return to telemetry Reason for change: ctrlm currently does not know why xrsr might have ended a session. Also it does not know what protocol error may have occurred only that one did. Test Procedure: check telemetry for new fields, parse them, confirm that new data exists and is correct Priority: P0 Signed-off-by: Jason Thomson <jason_thomson@comcast.com> * Fixing whitespace * Order of vars * Update telemetry markers version number * Reverting enum name change * Renaming vars for clarity, reordering vars for consistency * Fix whitespace * Adding stream end reason to telemetry log * Adding stream reason end to telemetry --------- Signed-off-by: Jason Thomson <jason_thomson@comcast.com>
* RDKEMW-5849 : remove deprecated "experience" code Reason for change: the app controls experience, ctrlm does is no longer involved Test Procedure: nothing new to test. Check that normal functionality is not affected Priority; P2 Signed-off-by: Jason Thomson <jason_thomson@comcast.com> * Align CTRLM_VOICE_QUERY_STRING_MAX_PAIRS with xrsr without including xrsr.h --------- Signed-off-by: Jason Thomson <jason_thomson@comcast.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 96 out of 97 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| ctrlm_hal_ble_VoiceStreamEnd_t streamEnd = CTRLM_HAL_BLE_VOICE_STREAM_END_ON_KEY_UP; | ||
| auto rcu = controllers_.at(id); | ||
|
|
||
| if (rcu->getPressAndHoldSupport()) { |
* RDKEMW-16711: Update Thunder plugin to use _string and _boolean Reason for change: Thunder API now returns a string for last wakeup reason and a boolean for Networked Standby Mode Enabled, rather than a JsonObject. Updating ctrlm accordingly Test Procedure: NSM testing Priority: P0 Risks: Low Signed-off-by: Jason Thomson <jason_thomson@comcast.com> * Update ctrlm_thunder_plugin.h Address co-pilot comments * Update ctrlm_thunder_plugin.h * Update ctrlm_thunder_plugin.h --------- Signed-off-by: Jason Thomson <jason_thomson@comcast.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 98 out of 99 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
src/irdb/ctrlm_irdb_interface.cpp:1
msg->successpoints to a stacksuccess_vec, which is then mutated by network handlers. Unlessctrlm_main_queue_handler_push_new(..., true)is guaranteed to synchronously execute the handler and return only after completion on the same thread, this is a use-after-return/data-race risk (and even if synchronous, the cross-thread mutation of astd::vectorwithout synchronization is unsafe unless execution is fully serialized). Prefer making the success container owned by the queued message (e.g., heap-owned inside the shared_ptr message object), and use an explicit completion mechanism (semaphore/future/promise) to wait before reading results.
src/irdb/ctrlm_irdb_interface.cpp:1msg->successpoints to a stacksuccess_vec, which is then mutated by network handlers. Unlessctrlm_main_queue_handler_push_new(..., true)is guaranteed to synchronously execute the handler and return only after completion on the same thread, this is a use-after-return/data-race risk (and even if synchronous, the cross-thread mutation of astd::vectorwithout synchronization is unsafe unless execution is fully serialized). Prefer making the success container owned by the queued message (e.g., heap-owned inside the shared_ptr message object), and use an explicit completion mechanism (semaphore/future/promise) to wait before reading results.
src/ipc/ctrlm_rcp_ipc_event.cpp:1to_string()now returns a rawchar*allocated byjson_dumps, which requires the caller tofree()it. This is error-prone and will likely introduce leaks (especially since the previous API returnedstd::string). Consider returningstd::string(or a RAII wrapper) to preserve safe ownership semantics, or at minimum document the ownership contract prominently and ensure all call sites free the returned buffer.
src/input_event/ctrlm_input_event_writer.cpp:1- The fixed
sysfs_name[16]buffer can be too small for the uinput sysfs name; truncation would break later resolution of/sys/devices/virtual/input/<sysfs_name_>and/dev/input/event*. Use a larger buffer (e.g.,UINPUT_MAX_NAME_SIZE/ a conservative size like 80+) and/or validate that the returned name was not truncated before storing it.
src/ipc/ctrlm_ipc_iarm.h:1 size = sizeof(T) + str_sizecan overflowsize_tfor large payloads, causing under-allocation and potential buffer overflow when writingpayload. Add an overflow check before allocation and enforce a reasonable maximum payload size for IARM event messages.
src/voice/ctrlm_voice_obj.h:1- Several members (e.g.,
m_timestamp,m_cb_confirm_voice_obj,m_cb_confirm_param,m_status,m_device_type) are not default-initialized, which makes it easy to accidentally use uninitialized data on new/alternative code paths (especially since instances are stack-allocated and partially filled in multiple call sites). Initialize all fields with safe defaults in-class (e.g., nullptr/invalid enums/zeroed timestamp) to eliminate UB risk and simplify future maintenance.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| 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()); |
There was a problem hiding this comment.
Logging the full configuration contents on JSON parse errors can leak sensitive values (tokens, keys, PII) and can also generate very large logs. Prefer logging only the error location plus a small bounded context window (or truncated contents), and consider redacting known sensitive fields.
No description provided.