Skip to content

Conversation

@egalla204
Copy link
Contributor

No description provided.

@egalla204 egalla204 requested a review from a team as a code owner October 24, 2025 00:43
Copilot AI review requested due to automatic review settings October 30, 2025 18:26
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for multiple IRDB (IR Database) vendors and the ability to set a preferred vendor. The changes enable the system to query all supported IRDB vendors and configure which vendor to use based on RCU (Remote Control Unit) capabilities.

Key changes include:

  • Added two new API functions: ctrlm_irdb_get_supported_vendor_info() to retrieve all installed IRDB vendors and ctrlm_irdb_set_preferred_vendor() to configure the preferred vendor
  • Implemented plugin loading support with fallback to stub implementations when the plugin methods are unavailable
  • Updated the BLE controller to set the preferred vendor based on RCU support bitmask

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/irdb/ctrlm_irdb_stub.h Added stub declarations for the two new vendor-related functions
src/irdb/ctrlm_irdb_stub.cpp Implemented stub functions that log "not implemented" and return false
src/irdb/ctrlm_irdb_plugin.h Added public API declarations with documentation comments
src/irdb/ctrlm_irdb_interface.h Added set_vendor() method to the interface class
src/irdb/ctrlm_irdb_interface.cpp Implemented plugin loading, function pointer setup, and interface methods with vendor info logging
src/ble/ctrlm_ble_controller.cpp Updated to call set_vendor() before retrieving vendor info and changed log level from INFO to WARN on failure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


if (g_irdb.pluginGetSupportedVendors) {
std::vector<ctrlm_irdb_vendor_info_t> supported_vendors;
if ((ret = (*g_irdb.pluginGetSupportedVendors)(supported_vendors)) == true) {
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The return value from pluginGetSupportedVendors overwrites the ret variable that was set by pluginOpen on line 293. If pluginOpen succeeds but pluginGetSupportedVendors fails, the function will incorrectly return false. Use a separate variable for the supported vendors check, e.g., bool supported_ret = (*g_irdb.pluginGetSupportedVendors)(supported_vendors);

Suggested change
if ((ret = (*g_irdb.pluginGetSupportedVendors)(supported_vendors)) == true) {
bool supported_ret = (*g_irdb.pluginGetSupportedVendors)(supported_vendors);
if (supported_ret == true) {

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings October 30, 2025 19:08
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/irdb/ctrlm_irdb_stub.cpp:1

  • Parameter passing inconsistency between header and implementation. The stub header declares this function taking ctrlm_irdb_vendor_info_t by value (line 37 in ctrlm_irdb_stub.h), but the implementation uses const ctrlm_irdb_vendor_info_t &info. Update the header to match the implementation and use const reference, which also aligns with the plugin API signature.
/*

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings October 30, 2025 19:23
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/irdb/ctrlm_irdb_interface.cpp:1

  • The set_vendor call passes a vendor_info struct with only the rcu_support_bitmask field populated and an empty name field. The API comment in ctrlm_irdb_plugin.h (line 95) mentions this 'will basically pass through the bitmask coming from the RCU', but it's unclear how the plugin should handle a partially populated struct. Consider documenting whether the name field should be ignored or if this pattern is expected.
/*

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +540 to 547
ctrlm_irdb_vendor_info_t vendor_info{};
vendor_info.rcu_support_bitmask = vendor_support_bitmask;
irdb->set_vendor(vendor_info);

if (irdb->get_vendor_info(vendor_info)) {
XLOGD_INFO("Controller <%s> IRDBs supported bitmask = <0x%X>, which %s support the loaded IRDB plugin vendor <%s>",
ieee_address_get().to_string().c_str(), vendor_support_bitmask,
isSupportedIrdb(vendor_info) ? "DOES" : "does NOT", vendor_info.name.c_str());
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

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

The vendor_info struct is reused for both setting and getting vendor information. After calling set_vendor(vendor_info) with only the rcu_support_bitmask set, the same variable is passed to get_vendor_info(vendor_info), which will overwrite it. This could lead to confusion since the name field is empty when setting but may be populated when getting. Consider using separate variables or adding a comment to clarify this pattern is intentional.

Suggested change
ctrlm_irdb_vendor_info_t vendor_info{};
vendor_info.rcu_support_bitmask = vendor_support_bitmask;
irdb->set_vendor(vendor_info);
if (irdb->get_vendor_info(vendor_info)) {
XLOGD_INFO("Controller <%s> IRDBs supported bitmask = <0x%X>, which %s support the loaded IRDB plugin vendor <%s>",
ieee_address_get().to_string().c_str(), vendor_support_bitmask,
isSupportedIrdb(vendor_info) ? "DOES" : "does NOT", vendor_info.name.c_str());
ctrlm_irdb_vendor_info_t vendor_info_set{};
vendor_info_set.rcu_support_bitmask = vendor_support_bitmask;
irdb->set_vendor(vendor_info_set);
ctrlm_irdb_vendor_info_t vendor_info_get{};
if (irdb->get_vendor_info(vendor_info_get)) {
XLOGD_INFO("Controller <%s> IRDBs supported bitmask = <0x%X>, which %s support the loaded IRDB plugin vendor <%s>",
ieee_address_get().to_string().c_str(), vendor_support_bitmask,
isSupportedIrdb(vendor_info_get) ? "DOES" : "does NOT", vendor_info_get.name.c_str());

Copilot uses AI. Check for mistakes.
egalla204 and others added 2 commits October 30, 2025 20:11
* Deploy cla action

* RDKEMW-7174 : update AMC APP key mapping (#98)

* RDKEMW-3409 : move certselector logic into ctrlm-main repo (#102)

* RDKEMW-6767: getNetStatus call time out due to SAT download retries (#97)

Reason for change: During initial setup, the device is attempting
to download the SAT token and timing out, which is holding up
the ctrlm message queue processing, which allows BLE pairing to
fail

Test Procedure: boot and attempt to pair BLE remote, keeping an eye
on ctrlm loggging. Look for "CTRLM : ERROR: call_plugin: Thunder
call failed <getServiceAccessToken> <11>, attempt 1 of 1"
We expect BLE pairing to succeed rather than timeout. If the
call_plugin log error occurs and BLE pairing succeed then the
code change has been exercised and the issue is resolved.

Risks: Low

Priority: P1

Signed-off-by: Jason Thomson <[email protected]>

* RDKEMW-7333: remove device from bluez during factory reset (#100)

When an RDK device is factory reset, controlMgr will send a message to the remote to also factory reset itself. Once controlMgr gets notified from the remote of a successful RCU factory reset, it needs to be requested to bluez to remove the device. This prevents a connection attempt from happening to the just factory-reset RCU before the RDK device reboots. This connection attempt will prevent the RCU from autopairing during the activation flow after the RDK factory reset.

* RDKEMW-7573 : remove ctrlm compile flags (#104)

* RDKEMW-7694 : remove ctrlm build flags - CPC, DUNFELL (#105)

* RDKEMW-7834 : remove ctrlm build flags - RF4CE_PACKET_ANALYSIS (#107)

* RDKEMW-7772 : remove ctrlm build flags - DISABLE_BLE_VOICE (#106)

* RDKEMW-7122 : Missing Thunder cflags in ctrlm implemenation (#103)

* RDKEMW-7979 : use version/branch from recipe (#109)

* RDKEMW-8349 : ctrlm release v1.1.4 (#113)

* RDKEMW-8133: Optional param name for voiceSessionRequest (#108)

* RDKEMW-8133: Optional param name for voiceSessionRequest

Reason for change: Adding optional name param for voiceSessionRequest
                   which is needed to track metadata about voice
                   sessions from various ipcontrol clients
Test Procedure: Use VoiceControl voiceSessionRequest method with name
                param
Risks: Low
Signed-off-by: Kelvin Lu <[email protected]>

* RDKEMW-8133: Optional param name for voiceSessionRequest

Reason for change: remove the required conditional of name
Test Procedure:
Risks:
Signed-off-by: Kelvin Lu <[email protected]>

* RDKEMW-8133: Optional param name for voiceSessionRequest

Reason for change: Clean up log messaging
Test Procedure:
Risks:
Signed-off-by: Kelvin Lu <[email protected]>

* RDKEMW-8133: Optional param name for voiceSessionRequest

Reason for change: Move obj != NULL block to prevent null dereference
Test Procedure:
Risks:
Signed-off-by: Kelvin Lu <[email protected]>

---------

Signed-off-by: Kelvin Lu <[email protected]>

* RDKEMW-8354: ctrlm-main crash while holding standby during OTA (#115)

* RDKEMW-8354: ctrlm-main crash while holding standby during OTA

Reason for change: crash due to null reference on repeating timer event
Test Procedure: see ticket
Risks: low
Signed-off-by: Kelvin Lu <[email protected]>

* RDKEMW-8354: ctrlm-main crash on timer

Reason for change: add comment for clarity
Test Procedure:
Risks:
Signed-off-by: Kelvin Lu <[email protected]>

---------

Signed-off-by: Kelvin Lu <[email protected]>

* Deploy cla action

* Deploy fossid_integration_stateless_diffscan_target_repo action

* RDKEMW-8815: only return SUCCESS for autolookup if it found at least 1 code.

* Revert "RDKEMW-8815: only return SUCCESS for autolookup if it found at least 1 code." (#124)

* RDKEMW-8815: only return SUCCESS for autolookup if it found at least 1 code. (#125)

It can often happen that the IRDB returns success but no codes are returned, this leads to confusing UI screens. So even if the IR database returned successfully, only return success to the plugin API if there is at least 1 code is present

* Update CODEOWNERS (#130)

* RDKEMW-8929 (#129)

* RDKEMW-8929: Refactor ctrlm_voice_ipc_t to inherit ctrlm_ipc_iarm_t

Reason for change: Inherit ctrlm_ipc_iarm_t
Test Procedure: Verify behavior of events before & after no diff
Risks: Low
Signed-off-by: Kelvin Lu <[email protected]>

* RDKEMW-7225: BLE pairing retries (#126)

if BLE pairing fails, retry 3 times or up to the pairing timeout (currently configured at 20 seconds), whichever comes first.

---------

Signed-off-by: Jason Thomson <[email protected]>
Signed-off-by: Kelvin Lu <[email protected]>
Co-authored-by: rdkcmf <[email protected]>
Co-authored-by: dwolaver <[email protected]>
Co-authored-by: jthomp007c <[email protected]>
Co-authored-by: Kelvin Lu <[email protected]>
Co-authored-by: Alan Ryan <[email protected]>
Co-authored-by: Stephen Barrett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants