Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new high-level project overview document and introduces a set of “agentic engineering skills”/review guidelines under .github/, along with ignoring generated review artifacts.
Changes:
- Add
docs/PROJECT_OVERVIEW.mddescribing architecture, APIs, build, and testing for the NetworkManager Thunder plugin - Add multiple
.github/skills/**and.github/instructions/**documents for code review, quality checks, and analysis workflows - Add
.gitignoreentry to ignorereviews/artifacts
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/PROJECT_OVERVIEW.md | New project overview/architecture/API/build/testing documentation for the plugin |
| .gitignore | Ignores reviews/ directory (review artifacts) |
| .github/skills/triage-logs/SKILL.md | Skill doc for log-triage workflows (Telemetry 2.0 oriented) |
| .github/skills/thread-safety-analyzer/SKILL.md | Skill doc for analyzing concurrency/thread safety issues |
| .github/skills/technical-documentation-writer/SKILL.md | Skill doc/template for producing embedded project documentation |
| .github/skills/quality-checker/SKILL.md | Skill doc for running container-based quality checks |
| .github/skills/quality-checker/README.md | README for the quality-checker skill |
| .github/skills/platform-portability-checker/SKILL.md | Skill doc for cross-platform portability review |
| .github/skills/memory-safety-analyzer/SKILL.md | Skill doc for memory safety analysis |
| .github/skills/code-review/references/thread-patterns.md | Reference patterns for thread-safety reviews |
| .github/skills/code-review/references/review-checklist.md | General embedded code review checklist |
| .github/skills/code-review/references/quick-assessment-guide.md | Risk scoring / quick assessment reference |
| .github/skills/code-review/references/memory-patterns.md | Reference patterns for memory safety reviews |
| .github/skills/code-review/references/example-review-template.md | Example review output template |
| .github/skills/code-review/references/common-pitfalls.md | Reference list of common pitfalls (Telemetry-oriented examples) |
| .github/skills/code-review/SKILL.md | Main code-review skill doc and process |
| .github/skills/code-review/README.md | README for the code-review skill |
| .github/instructions/shell-scripts.instructions.md | Shell scripting standards doc |
| .github/instructions/cpp-testing.instructions.md | C++/gtest testing standards doc (Telemetry-oriented paths/examples) |
| .github/instructions/c-embedded.instructions.md | Embedded C standards doc (Telemetry-oriented references/examples) |
| .github/instructions/build-system.instructions.md | Autotools build standards doc (Telemetry-oriented examples) |
| .github/agents/legacy-refactor-specialist.agent.md | Agent profile for legacy refactoring workflows |
| .github/agents/embedded-c-expert.agent.md | Agent profile for embedded C review/development workflows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+25
| --- | ||
| applyTo: "source/test/**/*.cpp,source/test/**/*.h" | ||
| --- | ||
|
|
||
| # C++ Testing Standards (Google Test) | ||
|
|
||
| ## Test Framework | ||
|
|
||
| Use Google Test (gtest) and Google Mock (gmock) for all C++ test code. | ||
|
|
||
| ## Test Organization | ||
|
|
||
| ### File Structure | ||
| - One test file per source file: `foo.c` → `test/FooTest.cpp` | ||
| - Test fixtures for complex setups | ||
| - Mocks in separate files when reusable | ||
|
|
||
| ```cpp | ||
| // GOOD: Test file structure | ||
| // filepath: source/test/utils/UtilsTest.cpp | ||
|
|
||
| extern "C" { | ||
| #include <utils/vector.h> | ||
| #include <utils/t2collection.h> | ||
| } |
Comment on lines
+129
to
+132
| - Periodic HTTP requests to test endpoints (default: clients3.google.com) | ||
| - Captive portal detection | ||
| - Limited vs. full internet detection | ||
| - Configurable test interval (default: 6 seconds) |
Comment on lines
+263
to
+304
| ### COM-RPC Interface | ||
|
|
||
| Defined in `INetworkManager.h`, provides type-safe C++ interface: | ||
|
|
||
| ```cpp | ||
| namespace Exchange | ||
| { | ||
| struct INetworkManager: virtual public Core::IUnknown | ||
| { | ||
| // Interface management | ||
| virtual uint32_t GetAvailableInterfaces( | ||
| IInterfaceDetailsIterator*& interfaces) const = 0; | ||
| virtual uint32_t GetPrimaryInterface(string& interface) const = 0; | ||
| virtual uint32_t SetInterfaceState( | ||
| const string& interface, const bool enabled) = 0; | ||
|
|
||
| // IP configuration | ||
| virtual uint32_t GetIPSettings( | ||
| const string& interface, | ||
| const string& ipversion, | ||
| IPAddress& result) const = 0; | ||
| virtual uint32_t SetIPSettings( | ||
| const string& interface, | ||
| const IPAddress& address) = 0; | ||
|
|
||
| // WiFi operations | ||
| virtual uint32_t StartWiFiScan(const FrequencyType frequency) = 0; | ||
| virtual uint32_t WiFiConnect(const WIFIConnectParam& param) = 0; | ||
| virtual uint32_t WiFiDisconnect() = 0; | ||
|
|
||
| // Connectivity | ||
| virtual uint32_t IsConnectedToInternet( | ||
| string& ipversion, | ||
| InternetStatus& result) = 0; | ||
|
|
||
| // Diagnostics | ||
| virtual uint32_t Ping( | ||
| const string& endpoint, | ||
| const uint32_t packets, | ||
| string& result) = 0; | ||
| }; | ||
| } |
| { | ||
| "jsonrpc": "2.0", | ||
| "id": 1, | ||
| "method": "NetworkManager.1.GetAvailableInterfaces" |
Comment on lines
+378
to
+385
| Backend is selected at build time via CMake options: | ||
|
|
||
| ```cmake | ||
| # Gnome NetworkManager backend (default for Linux) | ||
| cmake -DUSE_GNOME_NETWORK_MANAGER=ON .. | ||
|
|
||
| # RDK Network Service Manager backend | ||
| cmake -DUSE_RDK_NSM=ON .. |
Comment on lines
+154
to
+163
| ### Thread Overview | ||
|
|
||
| | Thread Name | Purpose | Priority | Stack Size | | ||
| |------------|---------|----------|------------| | ||
| | Thunder Main | Plugin lifecycle, JSONRPC dispatch | Normal | Default | | ||
| | GLib Main Loop | DBus event processing (Gnome backend) | High | 64KB | | ||
| | Connectivity Monitor | Internet connectivity tests | Low | 64KB | | ||
| | STUN Client | Public IP discovery requests | Low | 32KB | | ||
| | WiFi Scan | Asynchronous WiFi scanning | Normal | 64KB | | ||
|
|
Comment on lines
+166
to
+186
| ```cpp | ||
| // Backend access synchronization | ||
| static std::mutex backend_mutex; // Serializes backend operations | ||
| static std::mutex interface_mutex; // Protects interface list | ||
|
|
||
| // Connectivity monitoring | ||
| static std::condition_variable connectivity_cv; // Connectivity test sync | ||
| static std::mutex connectivity_mutex; | ||
|
|
||
| // STUN operations | ||
| static std::mutex stun_mutex; | ||
| ``` | ||
|
|
||
| ### Lock Ordering | ||
|
|
||
| To prevent deadlocks, always acquire locks in this order: | ||
|
|
||
| 1. `backend_mutex` (backend operations) | ||
| 2. `interface_mutex` (interface list) | ||
| 3. Component-specific locks (connectivity, STUN) | ||
|
|
Comment on lines
+249
to
+275
| root["📁 telemetry"] | ||
| root --> source["📁 source"] | ||
| root --> test["📁 test"] | ||
| root --> makefile["📄 Makefile.am (+2/-1)"] | ||
|
|
||
| source --> bulkdata["📁 bulkdata"] | ||
| source --> scheduler["📁 scheduler"] | ||
| source --> ccsp["📁 ccspinterface"] | ||
|
|
||
| bulkdata --> profile["📄 profile.c (+45/-12) ⚠️"] | ||
| bulkdata --> reportprof["📄 reportprofiles.c (+8/-3)"] | ||
|
|
||
| scheduler --> schedulerc["📄 scheduler.c (+120/-80) 🔴"] | ||
|
|
||
| ccsp --> rbus["📄 rbusInterface.c (+5/-2)"] | ||
|
|
||
| test --> schedtest["📄 scheduler_test.cpp (+95/-0) ✅"] | ||
|
|
||
| classDef critical fill:#ff6b6b,stroke:#c92a2a,color:#fff | ||
| classDef warning fill:#ffd43b,stroke:#f08c00,color:#000 | ||
| classDef safe fill:#51cf66,stroke:#2f9e44,color:#fff | ||
| classDef neutral fill:#e0e0e0,stroke:#9e9e9e,color:#000 | ||
|
|
||
| class schedulerc critical | ||
| class profile warning | ||
| class schedtest safe | ||
| class reportprof,rbus,makefile neutral |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.