Skip to content

Conversation

rababu2
Copy link
Contributor

@rababu2 rababu2 commented Aug 17, 2025

No description provided.

@rababu2 rababu2 requested a review from a team as a code owner August 17, 2025 17:53
@rababu2 rababu2 requested a review from a team August 17, 2025 17:53
@skamath skamath requested a review from Copilot August 21, 2025 05:50
Copy link
Contributor

@Copilot 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 implements a device settings source code restructuring related to RDKEMW6161, primarily introducing event notification interfaces and IARM implementation for composite input handling. The changes establish a new notification header file and add event management capabilities across multiple device classes.

  • Added comprehensive enums and typedefs for device settings notifications in a new header file
  • Introduced event listener interfaces for video, audio, HDMI, composite input, display, and host components
  • Implemented IARM-based composite input event handling with proper registration/unregistration mechanisms

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
rpc/include/dsMgrNtf.h New header defining enums and typedefs for audio, video, HDMI, composite, display events and capabilities
ds/videoOutputPort.cpp Added stub Register/UnRegister event methods with copyright update
ds/videoDevice.cpp Added stub Register/UnRegister event methods with copyright update
ds/include/videoOutputPort.hpp Added IEvent interface for resolution, HDCP, and video format events
ds/include/videoDevice.hpp Added IEvent interface for display framerate change events
ds/include/iarm/iarmCompositeIn.hpp New IARM composite input class with event handling capabilities
ds/include/host.hpp Added IEvent interface for sleep mode changes
ds/include/hdmiIn.hpp Added comprehensive IEvent interface for HDMI input events
ds/include/frontPanelTextDisplay.hpp Added IEvent interface for time format changes
ds/include/displayConnectionChangeListener.hpp Added event interfaces for display connection and HDCP events
ds/include/compositeIn.hpp Added IEvent interface and IARM implementation integration
ds/include/audioOutputPort.hpp Added comprehensive IEvent interface for audio events
ds/iarm/iarmCompositeIn.cpp Complete IARM implementation for composite input event handling
ds/host.cpp Added stub implementations for various Register/UnRegister methods
ds/hdmiIn.cpp Added stub Register/UnRegister methods with copyright update
ds/frontPanelConfig.cpp Added stub Register/UnRegister methods for front panel display
ds/compositeIn.cpp Integrated IARM implementation with proper lifecycle management
ds/audioOutputPort.cpp Added stub Register/UnRegister methods
ds/Makefile.am Added conditional compilation for IARM path
ds/Makefile Updated include paths for IARM directory

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

}HDMIInVideoFrameRate;

typedef struct _HDMIVideoPortResolution {
std::string name;
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

Using std::string in a C header file is problematic. The file uses extern "C" but includes C++ types, which can cause compilation issues when included from C code.

Suggested change
std::string name;
char name[256];

Copilot uses AI. Check for mistakes.

DisplayInVideoStereoScopicMode stereoScopicMode;
DisplayInVideoFrameRate frameRate;
bool interlaced;
}DisplayVideoPortResolution;
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

Using std::string in a C header file is problematic. The file uses extern "C" but includes C++ types, which can cause compilation issues when included from C code.

Suggested change
}DisplayVideoPortResolution;
#define DS_MAX_NAME_LEN 256
typedef struct _DisplayVideoPortResolution {
char name[DS_MAX_NAME_LEN];
DisplayTVResolution pixelResolution;
DisplayVideoAspectRatio aspectRatio;
DisplayInVideoStereoScopicMode stereoScopicMode;
DisplayInVideoFrameRate frameRate;
bool interlaced;
}DisplayVideoPortResolution;

Copilot uses AI. Check for mistakes.

// @text OnCompositeInVideoModeUpdate
// @param activePort: Active port
// @param videoResolution: See DisplayVideoPortResolution
virtual void OnCompositeInVideoModeUpdate(CompositeInPort activePort, DisplayVideoPortResolution videoResolution) = 0;
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

This pure virtual function (= 0) is inconsistent with other virtual functions in the IEvent interface that have default empty implementations. This will force all implementers to provide this method.

Suggested change
virtual void OnCompositeInVideoModeUpdate(CompositeInPort activePort, DisplayVideoPortResolution videoResolution) = 0;
virtual void OnCompositeInVideoModeUpdate(CompositeInPort activePort, DisplayVideoPortResolution videoResolution) { };

Copilot uses AI. Check for mistakes.

{
uint32_t retStatus=FAIL;

INT_INFO("CompositeInput::Register Entry \n");
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The log message says 'Register Entry' but this is in the UnRegister function. It should say 'UnRegister Entry'.

Suggested change
INT_INFO("CompositeInput::Register Entry \n");
INT_INFO("CompositeInput::UnRegister Entry \n");

Copilot uses AI. Check for mistakes.

ds/hdmiIn.cpp Outdated


/**
* @fn void CompositeInput::UnRegister(IEvent *listener)
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The function comment incorrectly refers to 'CompositeInput::UnRegister' when this is actually 'HdmiInput::UnRegister'.

Suggested change
* @fn void CompositeInput::UnRegister(IEvent *listener)
* @fn void HdmiInput::UnRegister(IEvent *listener)

Copilot uses AI. Check for mistakes.

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