-
Notifications
You must be signed in to change notification settings - Fork 3
RDKEMW-6331 : Fix HdmiCecSink COM-RPC issues #217
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: develop
Are you sure you want to change the base?
Conversation
Reason for change : COM-RPC Implementation with fixes for - getDeviceList does not return proper json tags in response. Populating powerStatus instead of portNumber. Added RequestAudioDevicePowerStatus API Signed-off-by: smanes0213 <[email protected]>
#include "ccec/MessageEncoder.hpp" | ||
#include "host.hpp" | ||
#include "UtilsgetRFCConfig.h" | ||
|
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.
Along with comparing the JSON RPC could you also share the custom build with DEV QA to perform the sanity check for HdmiCec feature and make sure no regression observed before we merge. Otherwise im ok with code changes
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.
I have shared the custom build with QA waiting for the results from their side.
device.osdName = HdmiCecSinkImplementation::_instance->deviceList[route[i]].m_osdName.toString().c_str(); | ||
device.vendorID = HdmiCecSinkImplementation::_instance->deviceList[route[i]].m_vendorID.toString().c_str(); | ||
|
||
paths.push_back(device); |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Variable copied when it could be moved
"device" is copied and then passed-by-reference as parameter to STL insertion function "std::vector<WPEFramework::Exchange::IHdmiCecSink::HdmiCecSinkActivePath, std::allocatorWPEFramework::Exchange::IHdmiCecSink::HdmiCecSinkActivePath >::push_back(std::vector<WPEFramework::Exchange::IHdmiCecSink::HdmiCecSinkActivePath, std::allocatorWPEFramework::Exchange::IHdmiCecSink::HdmiCecSinkActivePath >::value_type const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""device"")" instead of "device".
} | ||
|
||
|
||
const void HdmiCecSinkImplementation::InitializeIARM() |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Parse warning
type qualifier on return type is meaningless
Low Impact, CWE-398
PW.USELESS_TYPE_QUALIFIER_ON_RETURN_TYPE
for (int i = 0; i < m_numofHdmiInput; i++){ | ||
HdmiPortMap hdmiPort((uint8_t)i); | ||
LOGINFO(" Add to vector [%d] \n", i); | ||
hdmiInputs.push_back(hdmiPort); |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Variable copied when it could be moved
"hdmiPort" is copied and then passed-by-reference as parameter to STL insertion function "std::vector<WPEFramework::Plugin::HdmiPortMap, std::allocatorWPEFramework::Plugin::HdmiPortMap >::push_back(std::vector<WPEFramework::Plugin::HdmiPortMap, std::allocatorWPEFramework::Plugin::HdmiPortMap >::value_type const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""hdmiPort"")" instead of "hdmiPort".
HdmiCecSink/HdmiCecSink.cpp
Outdated
case NUMBER_9: | ||
_instance->smConnection->sendTo(LogicalAddress(logicalAddress), MessageEncoder().encode(UserControlPressed(UICommand::UI_COMMAND_NUM_9)), 100); | ||
break; | ||
if(nullptr != _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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Dereference before null check
Null-checking "this->_hdmiCecSink" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
Medium Impact, CWE-476
REVERSE_INULL
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.
Only dereference the pointer if not NULL.
{ | ||
audiodescriptor.Add(descriptor); | ||
} | ||
HdmiCecSinkImplementation::_instance->Send_ShortAudioDescriptor_Event(audiodescriptor); |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Variable copied when it could be moved
"audiodescriptor" is passed-by-value as parameter to "WPEFramework::Core::JSON::ArrayTypeWPEFramework::Core::JSON::Variant::ArrayType(WPEFramework::Core::JSON::ArrayTypeWPEFramework::Core::JSON::Variant const &)", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""audiodescriptor"")" instead of "audiodescriptor".
, _registeredEventHandlers(false) | ||
{ | ||
LOGWARN("Initlaizing HdmiCecSinkImplementation"); | ||
} |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Uninitialized scalar field
Non-static class member "cecEnableStatus" is not initialized in this constructor nor in any functions that it calls.
Medium Impact, CWE-457
UNINIT_CTOR
stopArc(); | ||
while(m_currentArcRoutingState != ARC_STATE_ARC_TERMINATED) | ||
{ | ||
usleep(500000); |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Waiting while holding a lock
Call to "usleep" might sleep while holding lock "lock._M_device".
Medium Impact, CWE-667
SLEEP
} | ||
|
||
std::unique_lock<std::mutex> lk(_instance->m_pollExitMutex); | ||
if ( _instance->m_ThreadExitCV.wait_for(lk, std::chrono::milliseconds(_instance->m_sleepTime)) == std::cv_status::timeout ) |
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.
Coverity Issue - Data race condition
A wait is performed without a loop. If there is a spurious wakeup, the condition may not be satisfied. [Note: The source code implementation of the function has been overridden by a builtin model.]
Medium Impact, CWE-none
BAD_CHECK_OF_WAIT_COND
How to fix
Check the wait condition in a loop, with the lock held. The lock must not be released between the condition and the wait.
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.
Will resolve this as part of this ticket : https://ccp.sys.comcast.net/browse/RDKEMW-8431
} | ||
msgProcessor = new HdmiCecSinkProcessor(*smConnection); | ||
msgFrameListener = new HdmiCecSinkFrameListener(*msgProcessor); | ||
if(smConnection) |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Dereference before null check
Null-checking "this->smConnection" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
Medium Impact, CWE-476
REVERSE_INULL
PowerManagerInterfaceRef _powerManagerPlugin; | ||
Core::Sink<PowerManagerNotification> _pwrMgrNotification; | ||
bool _registeredEventHandlers; | ||
const void InitializeIARM(); |
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.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Parse warning
type qualifier on return type is meaningless
Low Impact, CWE-398
PW.USELESS_TYPE_QUALIFIER_ON_RETURN_TYPE
{ | ||
stopArc(); | ||
while (m_currentArcRoutingState != ARC_STATE_ARC_TERMINATED) { | ||
usleep(500000); |
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.
Coverity Issue - Waiting while holding a lock
Call to "usleep" might sleep while holding lock "lock._M_device".
Medium Impact, CWE-667
SLEEP
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.
Will resolve this as part of this ticket : https://ccp.sys.comcast.net/browse/RDKEMW-8431
, _registeredEventHandlers(false) | ||
{ | ||
LOGWARN("Initializing HdmiCecSinkImplementation"); | ||
} |
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.
Coverity Issue - Uninitialized pointer field
Non-static class member "msgFrameListener" is not initialized in this constructor nor in any functions that it calls.
Medium Impact, CWE-457
UNINIT_CTOR
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.
Will resolve this as part of this ticket : https://ccp.sys.comcast.net/browse/RDKEMW-8431
Reason for change : COM-RPC Implementation with fixes for -
getDeviceList does not return proper json tags in response.
Populating powerStatus instead of portNumber.
Added RequestAudioDevicePowerStatus API
Risks: Medium