-
Notifications
You must be signed in to change notification settings - Fork 4
RDKEMW-11903: Add custom coding guidelines for entservices-repos #185
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
Conversation
Reason for change: To enhance copilot reviews using repo specific guidelines Test Procedure:NA Risks: Low version: Patch Signed-off-by: Soundaryaa [email protected]
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 custom coding guidelines for the entservices repositories to enhance Copilot code reviews. The guidelines provide comprehensive documentation for Thunder plugin development standards and best practices.
Key Changes:
- Added seven instruction files covering plugin lifecycle, implementation patterns, configuration, and CMake usage
- Created copilot-instructions.md to standardize how review comments should reference these guidelines
- Established guidelines for module naming, inter-plugin communication, and onboarding compliance
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
.github/instructions/Pluginmodule.instructions.md |
Defines MODULE_NAME conventions and declaration requirements for Thunder plugins |
.github/instructions/Pluginlifecycle.instructions.md |
Specifies mandatory Initialize, Deinitialize, and Deactivated lifecycle methods with resource management rules |
.github/instructions/Pluginimplementation.instructions.md |
Documents inter-plugin communication patterns using COM-RPC and on-demand interface acquisition |
.github/instructions/Pluginconfig.instructions.md |
Outlines plugin configuration file structure and mandatory properties (callsign, autostart, mode) |
.github/instructions/Plugincmake.instructions.md |
Enforces NAMESPACE variable usage instead of hardcoded framework names in CMake files |
.github/instructions/PluginOnboardingCompliance.instructions.md |
Details Coverity scan and CI workflow updates required when adding new plugins |
.github/instructions/Plugin.instructions.md |
Covers interface implementation, service registration, JSON-RPC stub usage, and out-of-process failure handling |
.github/copilot-instructions.md |
Establishes format for linking review comments to specific guideline sections in instruction files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint32_t ret = m_SystemPluginObj->Invoke<JsonObject, JsonObject>(THUNDER_RPC_TIMEOUT, _T("getFriendlyName"), params, Result); | ||
| ``` | ||
|
|
||
| Use COM-RPC for plugin event registration by passing a C++ callback interface pointer for low-latency communication. It is important to register for StateChange notifications to monitor the notifying plugin's lifecycle. This allows you to safely release the interface pointer upon deactivation and prevents accessing a non-existent service. |
Copilot
AI
Dec 22, 2025
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.
Missing word in sentence. It should read "This allows you to safely release" instead of "This allows you safely release".
| } | ||
| ``` | ||
|
|
||
| If the notifying plugin supports only JSON-RPC, then use a specialized smart link type when subscribing to its events. This method allows the framework to efficiently handle Plugin statechange events. |
Copilot
AI
Dec 22, 2025
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 word "statechange" should be written as two words: "state change" for better readability and consistency with standard terminology.
| If the notifying plugin supports only JSON-RPC, then use a specialized smart link type when subscribing to its events. This method allows the framework to efficiently handle Plugin statechange events. | |
| If the notifying plugin supports only JSON-RPC, then use a specialized smart link type when subscribing to its events. This method allows the framework to efficiently handle Plugin state change events. |
| @@ -0,0 +1,49 @@ | |||
| --- | |||
| applyTo: "**/*.config,**/*.conf.in" | |||
Copilot
AI
Dec 22, 2025
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 config file naming pattern shows ".config" and ".conf.in" as separate patterns, but based on the content and example at line 34, it should be "/*.conf.in" only. The pattern "/*.config" doesn't match the documented ".conf.in" file extension.
| applyTo: "**/*.config,**/*.conf.in" | |
| applyTo: "**/*.conf.in" |
|
|
||
| These autogenerated stubs are the Exchange::J* C++ classes (for example, Exchange::JHdcpProfile and JsonData_HdcpProfile.h) that are produced by the Thunder JSON‑RPC code generator from the IPluginName* interface headers; they expose the C++ interface over JSON‑RPC so you do not have to call Register() for each method manually. | ||
|
|
||
| Plugins using autogenerated JSON-RPC stubs (Exchange::J* classes) must register and unregister them in Initialize() and Deinitialize() methods.It should not be done in constructor and destructor. |
Copilot
AI
Dec 22, 2025
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 requirement description uses "It should not be done" which is passive voice and ambiguous about who is responsible. Consider rephrasing to active voice: "Do not register and unregister them in the constructor and destructor."
| Plugins using autogenerated JSON-RPC stubs (Exchange::J* classes) must register and unregister them in Initialize() and Deinitialize() methods.It should not be done in constructor and destructor. | |
| Plugins using autogenerated JSON-RPC stubs (Exchange::J* classes) must register and unregister them in Initialize() and Deinitialize() methods. Do not register or unregister them in the constructor or destructor. |
|
|
||
| ### Deactivated | ||
|
|
||
| Each plugin should implement the deactivated method. In Deactivated, it should be checked if remote connectionId matches your plugin's connectionId. If it matches your plugin's connectionId, the plugin should submit a deactivation job to handle the out-of-process failure gracefully. |
Copilot
AI
Dec 22, 2025
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 word "Plugin" should be lowercase when referring to the method name. The sentence should read "Each plugin should implement the Deactivated method" (with capital D for the method name) rather than "the deactivated method".
| Each plugin should implement the deactivated method. In Deactivated, it should be checked if remote connectionId matches your plugin's connectionId. If it matches your plugin's connectionId, the plugin should submit a deactivation job to handle the out-of-process failure gracefully. | |
| Each plugin should implement the Deactivated method. In Deactivated, it should be checked if remote connectionId matches your plugin's connectionId. If it matches your plugin's connectionId, the plugin should submit a deactivation job to handle the out-of-process failure gracefully. |
| _userSettingsPlugin = _service->QueryInterface<WPEFramework::Exchange::IUserSettings>(); | ||
| ``` | ||
|
|
||
| SHOULD not use JSON-RPC or LinkType for inter-plugin communication, as they introduce unnecessary overhead. |
Copilot
AI
Dec 22, 2025
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 phrase "SHOULD not use" should be changed to "should not use" (lowercase) for consistency with standard documentation conventions, or use "MUST NOT" if this is a strict requirement following RFC 2119 terminology.
| SHOULD not use JSON-RPC or LinkType for inter-plugin communication, as they introduce unnecessary overhead. | |
| should not use JSON-RPC or LinkType for inter-plugin communication, as they introduce unnecessary overhead. |
| } | ||
| ``` | ||
|
|
||
| If the notifying plugin supports only JSON-RPC, then use a specialized smart link type when subscribing to its events. This method allows the framework to efficiently handle Plugin statechange events. |
Copilot
AI
Dec 22, 2025
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.
Missing article before "specialized". The sentence should read "then use a specialized smart link type" instead of "then use specialized smart link type".
|
|
||
| ### Requirement | ||
|
|
||
| - If the plugin runs as out-of-process, then it should implement RPC::IRemoteConnection::"INotification interface inside your plugin. |
Copilot
AI
Dec 22, 2025
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.
There's a double colon typo in the requirement. Change 'RPC::IRemoteConnection::"INotification interface' to 'RPC::IRemoteConnection::INotification interface' (remove the extra quote mark).
| - If the plugin runs as out-of-process, then it should implement RPC::IRemoteConnection::"INotification interface inside your plugin. | |
| - If the plugin runs as out-of-process, then it should implement RPC::IRemoteConnection::INotification interface inside your plugin. |
Reason for change: To enhance copilot reviews using repo specific guidelines Test Procedure:NA Risks: Low version: Patch Signed-off-by: Soundaryaa [email protected]
Reason for change: To enhance copilot reviews using repo specific guidelines
Test Procedure:NA
Risks: Low
version: Patch
Signed-off-by: Soundaryaa [email protected]