-
Notifications
You must be signed in to change notification settings - Fork 3
[RDKEMW-6163] [RDKEMW-6164] CEC source and sink changes #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/RDKEMW-6163-k1
Are you sure you want to change the base?
Conversation
* RDKEMW-7169 - Gtest Add missing empty files * RDKEMW-7169 - Gtest Add missing empty files * RDKEMW-7169 - Gtest Add missing empty files * RDKEMW-7169 - Gtest Add missing empty files * RDKEMW-7169 - Gtest Add missing empty files --------- Co-authored-by: Srikanth <[email protected]>
1.4.6 release 1.4.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure to remove dsMgr.h
and other unused header from all plugins.
@@ -418,6 +425,11 @@ namespace WPEFramework | |||
{ | |||
//TODO(MROLLINS) this is probably per process so we either need to be running in our own process or be carefull no other plugin is calling it | |||
device::Manager::Initialize(); | |||
|
|||
#ifndef IO_HCEC_ENABLE_IARM | |||
device::Host::getInstance().Register(this, "WPE[HdmiCecSource]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to truncate clientName to less than 11 chars so that, small string optimization is used, take care at all instances. Also I see in other PR's WPE::
convention is used.
HdmiCecSink/HdmiCecSink.cpp
Outdated
InitializeIARM(); | ||
#else | ||
device::Host::getInstance().Register(this, "WPE[HdmiCecSink]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is device::manager::Initialize()
called before register ?
There was a problem hiding this 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 introduces HdmiCecSource and HdmiCecSink changes that provide alternative implementations for HDMI event handling by adding support for direct device host interfaces alongside existing IARM-based implementations.
Key changes include:
- Addition of conditional compilation support via IO_HCEC_ENABLE_IARM preprocessor flag
- Implementation of device::Host event interface callbacks as alternatives to IARM event handlers
- Enhanced error handling and initialization logic for device manager components
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
HdmiCecSource/HdmiCecSourceImplementation.h | Adds IDisplayDeviceEvents interface inheritance and OnDisplayHDMIHotPlug declaration |
HdmiCecSource/HdmiCecSourceImplementation.cpp | Implements conditional IARM vs device::Host event handling with preprocessor guards |
HdmiCecSink/HdmiCecSink.h | Adds IHdmiInEvents interface inheritance and OnHdmiInEventHotPlug declaration |
HdmiCecSink/HdmiCecSink.cpp | Refactors initialization with device manager setup and conditional IARM/device::Host implementations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
HdmiCecSink/HdmiCecSink.cpp
Outdated
#if 0 //just to verify compilation | ||
device::HdmiInput::getInstance().getHDMIARCPortId(&portId); | ||
#else | ||
portId = 0; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out code with #if 0
suggests unfinished implementation. This hardcoded portId = 0
assignment bypasses the actual device API call and could lead to incorrect behavior in production. Either implement the proper API call or add a TODO comment explaining why this is temporarily disabled.
#if 0 //just to verify compilation | |
device::HdmiInput::getInstance().getHDMIARCPortId(&portId); | |
#else | |
portId = 0; | |
#endif | |
// TODO: Replace hardcoded portId assignment with actual device API call when available. | |
// device::HdmiInput::getInstance().getHDMIARCPortId(&portId); | |
portId = 0; |
Copilot uses AI. Check for mistakes.
HdmiCecSink/HdmiCecSink.cpp
Outdated
LOGINFO("Number of Inputs [%d] \n", hdmiInput.numHdmiInputs ); | ||
m_numofHdmiInput = hdmiInput.numHdmiInputs; | ||
}else{ | ||
LOGINFO("Not able to get Numebr of inputs so defaulting to 3 \n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Numebr" is misspelled and should be "Number".
LOGINFO("Not able to get Numebr of inputs so defaulting to 3 \n"); | |
LOGINFO("Not able to get Number of inputs so defaulting to 3 \n"); |
Copilot uses AI. Check for mistakes.
RDKEMW-7712: Sync low latency for AVOutput initialization (#6338)
1.4.7 release 1.4.7
There was a problem hiding this 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 4 out of 4 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
isAnyPortConnected = true; | ||
} | ||
LOGINFO("update Port Status [%d] \n", i); | ||
hdmiInputs[i].update(device::HdmiInput::getInstance().isPortConnected(i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The isPortConnected call could throw an exception if the port index is invalid or the HdmiInput instance is not properly initialized. Consider adding bounds checking and exception handling around this call.
hdmiInputs[i].update(device::HdmiInput::getInstance().isPortConnected(i)); | |
bool isConnected = false; | |
try { | |
// If HdmiInput provides a method to get the number of ports, use it for bounds checking | |
int numPorts = device::HdmiInput::getInstance().getNumberOfPorts(); | |
if (i >= 0 && i < numPorts) { | |
isConnected = device::HdmiInput::getInstance().isPortConnected(i); | |
} else { | |
LOGERR("Port index [%d] is out of range [0, %d)", i, numPorts); | |
} | |
} catch (const std::exception& ex) { | |
LOGERR("Exception in isPortConnected(%d): %s", i, ex.what()); | |
} catch (...) { | |
LOGERR("Unknown exception in isPortConnected(%d)", i); | |
} | |
hdmiInputs[i].update(isConnected); |
Copilot uses AI. Check for mistakes.
HdmiCecSink/HdmiCecSink.cpp
Outdated
do { | ||
usleep(50000); // Sleep for 50ms before retrying | ||
portId = -1; | ||
device::HdmiInput::getInstance().getHDMIARCPortId(&portId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getHDMIARCPortId call lacks error handling. If this method throws an exception or fails, the retry logic may not work as expected. Consider wrapping this in a try-catch block to handle potential failures gracefully.
device::HdmiInput::getInstance().getHDMIARCPortId(&portId); | |
try { | |
device::HdmiInput::getInstance().getHDMIARCPortId(&portId); | |
} catch (const std::exception& ex) { | |
LOGERR("Exception in getHDMIARCPortId: %s (retry count[%d])", ex.what(), retryCount); | |
// Continue to next retry | |
} catch (...) { | |
LOGERR("Unknown exception in getHDMIARCPortId (retry count[%d])", retryCount); | |
// Continue to next retry | |
} |
Copilot uses AI. Check for mistakes.
HdmiCecSink/HdmiCecSink.cpp
Outdated
LOGINFO(" Add to vector [%d] \n", i); | ||
hdmiInputs.push_back(hdmiPort); | ||
} | ||
m_numofHdmiInput = device::HdmiInput::getInstance().getNumberOfInputs(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can throw exception, see
bc803aa
to
95d7e4f
Compare
e34b4c5
to
73a1113
Compare
73a1113
to
0951d07
Compare
No description provided.