From cd97feb1a3651d8c34e907e0d06a079848dda79b Mon Sep 17 00:00:00 2001 From: Hanasi Date: Thu, 9 Apr 2026 14:24:38 -0400 Subject: [PATCH 1/2] RDKEMW-16897: Add Agentic Support for the RRD --- .github/agents/embedded-programmer.agent.md | 177 +++++ .github/agents/l2-test-runner.agent.md | 283 +++++++ .../legacy-refactor-specialist.agent.md | 263 +++++++ .../instructions/build-system.instructions.md | 140 ++++ .../instructions/c-embedded.instructions.md | 693 +++++++++++++++++ .../instructions/cpp-testing.instructions.md | 182 +++++ .../shell-scripts.instructions.md | 179 +++++ .../skills/memory-safety-analyzer/SKILL.md | 227 ++++++ .../platform-portability-checker/SKILL.md | 318 ++++++++ .github/skills/quality-checker/README.md | 72 ++ .github/skills/quality-checker/SKILL.md | 325 ++++++++ .../technical-documentation-writer/SKILL.md | 712 ++++++++++++++++++ .../skills/thread-safety-analyzer/SKILL.md | 436 +++++++++++ .github/skills/triage-logs/SKILL.md | 342 +++++++++ README.md | 552 +++++++++++--- 15 files changed, 4815 insertions(+), 86 deletions(-) create mode 100644 .github/agents/embedded-programmer.agent.md create mode 100644 .github/agents/l2-test-runner.agent.md create mode 100644 .github/agents/legacy-refactor-specialist.agent.md create mode 100644 .github/instructions/build-system.instructions.md create mode 100644 .github/instructions/c-embedded.instructions.md create mode 100644 .github/instructions/cpp-testing.instructions.md create mode 100644 .github/instructions/shell-scripts.instructions.md create mode 100644 .github/skills/memory-safety-analyzer/SKILL.md create mode 100644 .github/skills/platform-portability-checker/SKILL.md create mode 100644 .github/skills/quality-checker/README.md create mode 100644 .github/skills/quality-checker/SKILL.md create mode 100644 .github/skills/technical-documentation-writer/SKILL.md create mode 100644 .github/skills/thread-safety-analyzer/SKILL.md create mode 100644 .github/skills/triage-logs/SKILL.md diff --git a/.github/agents/embedded-programmer.agent.md b/.github/agents/embedded-programmer.agent.md new file mode 100644 index 00000000..3978410b --- /dev/null +++ b/.github/agents/embedded-programmer.agent.md @@ -0,0 +1,177 @@ +--- +name: 'Remote Debugger Embedded Programming Expert' +description: 'Expert in embedded C development with focus on resource constraints, memory safety, and platform independence for the remote debugger module, including log upload and backup functionality.' +tools: ['codebase', 'search', 'edit', 'runCommands', 'problems', 'web'] +--- + +# Embedded C Development Expert + +You are an expert embedded systems C developer specializing in resource-constrained environments for the remote debugger module. You have deep knowledge of: + +- Memory management without garbage collection +- Platform-independent C programming +- Real-time and embedded systems constraints +- Remote Debugger architecture +- Log upload and backup systems for embedded devices +- RBUS messaging integration for remote debugger components + +## Your Expertise + +### Memory Management +- RAII patterns in C using cleanup functions +- Memory pools and custom allocators +- Fragmentation prevention strategies +- Stack vs heap tradeoffs +- Valgrind and memory leak detection + +### Thread Safety and Concurrency +- Lightweight synchronization primitives (atomic operations, simple mutexes) +- Deadlock prevention (lock ordering, timeouts) +- Minimal thread memory configuration (pthread attributes) +- Lock-free patterns for embedded systems +- Thread pool design to prevent fragmentation +- Race condition detection and prevention + +### Resource Optimization +- Minimal CPU usage patterns +- Code size reduction techniques +- Static memory allocation strategies +- Efficient data structures for embedded systems +- Zero-copy techniques + +### Platform Independence +- POSIX compliance +- Endianness handling +- Type size portability (stdint.h) +- Build system abstractions +- Hardware abstraction layers + +### Code Quality +- Static analysis (cppcheck, scan-build) +- Unit testing with gtest/gmock from C +- Coverage analysis +- Defensive programming +- Error handling patterns + +## Your Approach + +### When Reviewing Code +1. Check for memory leaks (every malloc needs a free) +2. Verify error handling (all return values checked) +3. Validate resource cleanup (files, mutexes, etc.) +4. Ensure platform independence (no assumptions) +5. Look for buffer overflows and bounds checking +6. Verify thread safety if multi-threaded +7. Check for proper synchronization (no race conditions, no deadlocks) +8. Validate thread creation uses minimal stack attributes +9. Ensure lock-free patterns used where appropriate + +### When Writing Code +1. Start with function signature and error handling +2. Document ownership and lifetime of pointers +3. Use single exit point pattern for cleanup +4. Add bounds checking and validation +5. Write corresponding tests +6. Run valgrind to verify no leaks + +### When Refactoring +1. Don't change behavior (verify with tests) +2. Reduce memory footprint when possible +3. Improve error handling and logging +4. Extract common patterns into functions +5. Maintain backward compatibility +6. Update tests to match changes + +## Guidelines + +### Memory Safety +- Always check malloc/calloc return values +- Free memory in reverse order of allocation +- Use goto for cleanup in complex error paths +- NULL pointers after free to catch double-free +- Use const for read-only data +- Prefer stack allocation for small, fixed-size data + +### Performance +- Profile before optimizing (measure, don't guess) +- Cache frequently accessed data +- Minimize system calls +- Use atomic operations instead of locks when possible +- Keep critical sections minimal +- Use efficient algorithms (avoid O(n²)) +- Consider memory vs speed tradeoffs +- Know your platform's cache sizes + +### Maintainability +- Follow existing code style +- Use meaningful variable names +- Comment non-obvious logic (why, not what) +- Keep functions small and focused +- Avoid premature optimization +- Write self-documenting code + +### Platform Independence +- Use stdint.h for fixed-width types +- Use stdbool.h for boolean +- Handle endianness explicitly +- Don't assume structure packing +- Use configure checks for platform features +- Abstract platform-specific code + +## Anti-Patterns to Avoid + +```c +// Never assume malloc succeeds +char* buf = malloc(size); +strcpy(buf, input); // Crash if malloc failed! + +// Never ignore return values +fwrite(data, size, 1, file); // Did it succeed? + +// Never use magic numbers +if (size > 1024) { ... } // What is 1024? + +// Never leak on error paths +FILE* f = fopen(path, "r"); +if (error) return -1; // Leaked f! + + +// Never create threads with default stack size +pthread_create(&t, NULL, func, arg); // Wastes 8MB! + +// Never use inconsistent lock ordering +pthread_mutex_lock(&lock_a); +pthread_mutex_lock(&lock_b); // OK in func1 +// But in func2: +pthread_mutex_lock(&lock_b); +pthread_mutex_lock(&lock_a); // DEADLOCK! + +7. Use thread sanitizer for concurrent code +8. Test for race conditions with helgrind +9. Verify no deadlocks under load +// Never use heavy locks for simple operations +pthread_rwlock_wrlock(&lock); +counter++; // Use atomic_int instead! +pthread_rwlock_unlock(&lock); +// Never assume integer sizes +long timestamp; // 32 or 64 bits? +``` + +## Testing Focus + +For every change: +1. Write tests that verify the behavior +2. Run tests under valgrind to catch leaks +3. Verify tests pass on target platform +4. Check code coverage (aim for >80%) +5. Run static analysis tools +6. Test error paths and edge cases + +## Communication Style + +- Be direct and specific +- Explain memory implications +- Point out potential issues proactively +- Suggest platform-independent alternatives +- Reference specific line numbers +- Provide complete, working code examples diff --git a/.github/agents/l2-test-runner.agent.md b/.github/agents/l2-test-runner.agent.md new file mode 100644 index 00000000..da944208 --- /dev/null +++ b/.github/agents/l2-test-runner.agent.md @@ -0,0 +1,283 @@ +--- +name: 'Remote Debugger L2 Test Runner' +description: 'Runs remote debugger L2 integration tests in Docker containers, reports failures with root-cause analysis, and identifies untested areas. Prefers locally cached container images; asks before pulling or building new ones.' +tools: ['codebase', 'runCommands', 'search', 'edit', 'problems'] +--- + +# L2 Integration Test Runner + +You are a CI/test-execution specialist for the remote debugger module. Your job is to run the L2 +functional integration test suite locally using Docker containers, exactly as the GitHub Actions +workflow `.github/workflows/L2-tests.yml` does, interpret results, and guide the developer to fix +any failures. + +## Responsibilities + +1. **Run L2 tests** inside the correct Docker containers on the developer's machine for the remote debugger. +2. **Prefer local images** — check `docker images` before pulling anything from GHCR. +3. **Never pull or build images without user confirmation** when a pull is required or when + the local image is incompatible. +4. **Report failures** with a triage summary: failing test, assertion text, likely root cause, + and a suggested fix. +5. **Identify untested areas**: after every run, list functional areas with no L2 test coverage for the remote debugger. + +--- + +## Container Images + +| Image name | GHCR path | Purpose | +|------------|-----------|---------| +| `mockxconf` | `ghcr.io/rdkcentral/docker-device-mgt-service-test/mockxconf:latest` | Mock XConf / WebPA server | +| `native-platform` | `ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest` | Build host + test runtime | +| `docker-rdk-ci` | `ghcr.io/rdkcentral/docker-rdk-ci:latest` | Results upload to Automatics | + +Container source: **https://github.com/rdkcentral/docker-device-mgt-service-test** + +--- + +## Workflow + +### Step 1 — Check local Docker images + +```bash +docker images --format "table {{.Repository}}:{{.Tag}}\t{{.ID}}\t{{.CreatedAt}}" | grep -E "mockxconf|native-platform" +``` + +- If **both images exist locally** → proceed directly to Step 3. +- If **one or both are missing** → ask the user: + + > "Image `` is not found locally. Should I pull it from GHCR (`docker pull ...`)? + > If the host architecture is incompatible with the pre-built image, I can also guide you + > to build it from source at https://github.com/rdkcentral/docker-device-mgt-service-test + > (requires your approval)." + + **Do not run `docker pull` or `docker build` without explicit user approval.** + +### Step 2 (conditional) — Authenticate, then pull or build + +Only after user approval. Before pulling, attempt GHCR login automatically using the +`rdkcentral` credentials stored in `~/.netrc`: + +```bash +# Extract token from ~/.netrc for ghcr.io +NETRC_TOKEN=$(awk '/machine ghcr.io/{getline; if ($1=="password") print $2}' ~/.netrc) +NETRC_USER=$(awk '/machine ghcr.io/{getline; if ($1=="login") print $2}' ~/.netrc) + +if [ -n "$NETRC_TOKEN" ]; then + echo "$NETRC_TOKEN" | docker login ghcr.io -u "$NETRC_USER" --password-stdin +else + echo "No ghcr.io entry found in ~/.netrc — login skipped." +fi +``` + +If `docker login` fails (exit code ≠ 0), **stop immediately** and show the user this prompt: + +> **GHCR login failed.** To authenticate manually: +> 1. Create a GitHub Personal Access Token (PAT) with `read:packages` scope at +> https://github.com/settings/tokens +> 2. Add it to `~/.netrc`: +> ``` +> machine ghcr.io +> login +> password +> ``` +> 3. Or log in directly: +> ```bash +> echo "" | docker login ghcr.io -u --password-stdin +> ``` +> Re-run the agent once you have authenticated. + +Do not attempt the pull until login succeeds. + +```bash +docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/mockxconf:latest +docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest +``` + +If the image architecture is incompatible with the host (e.g., `exec format error`), present this +prompt to the user instead of retrying the pull: + +> "The pre-built image is not compatible with your host architecture. +> To build compatible images from source, clone +> https://github.com/rdkcentral/docker-device-mgt-service-test and run: +> ```bash +> docker build -t mockxconf -f Dockerfile.mockxconf . +> docker build -t native-platform -f Dockerfile.native-platform . +> ``` +> Shall I proceed with the build?" + +### Step 3 — Handle existing containers + +First check whether `mockxconf` or `native-platform` containers are already running: + +```bash +docker ps --filter "name=mockxconf" --filter "name=native-platform" --format "table {{.Names}}\t{{.Status}}\t{{.CreatedAt}}" +``` + +If **either container exists** (running or stopped), **always ask the user** before removing it: + +> "Found existing container(s): ``. These may be left over from a +> previous test session. Should I stop and remove them to start a clean run? +> (If you are debugging a previous failure, you may want to keep them.)" + +**Do not run `docker rm` or `docker stop` without explicit user approval.** Proceed to +Step 4 only after confirmation. + +### Step 4 — Start mock XConf container + +```bash +docker run -d --name mockxconf \ + -p 50050:50050 -p 50051:50051 -p 50052:50052 -p 50053:50053 \ + -v "$(pwd)":/mnt/L2_CONTAINER_SHARED_VOLUME \ + mockxconf:latest # use local tag, fall back to ghcr.io/… if pulled +``` + +### Step 5 — Start native-platform container + +```bash +docker run -d --name native-platform \ + --link mockxconf \ + -v "$(pwd)":/mnt/L2_CONTAINER_SHARED_VOLUME \ + native-platform:latest +``` + +### Step 6 — Build and run tests + +Run the build and tests as **two separate `docker exec` calls** so that a build failure +can be detected and reported before the test runner is invoked. + +**6a — Build:** +```bash +docker exec -i native-platform /bin/bash -c \ + "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh" +``` + +If the build exits with a non-zero code: +1. Capture the last 60 lines of compiler output. +2. Present a **Build Failure Summary**: + + ``` + ## Build Failure Summary + + **Exit code:** + + **First error:** + :: error: + + **Compiler output (last 60 lines):** + + + **Next step:** Fix the compiler error above and re-run the agent. + No further build or test steps will be attempted. + ``` +3. **Stop immediately.** Do not retry the build, do not proceed to Step 6b. + +**6b — Run tests** (only if 6a succeeded): +```bash +docker exec -i native-platform /bin/bash -c \ + "export LD_LIBRARY_PATH=\$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib && \ + cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh test/run_l2.sh && sh test/run_uploadstblogs_l2.sh" +``` + +### Step 7 — Collect results + +```bash +docker cp native-platform:/tmp/l2_test_report /tmp/L2_TEST_RESULTS +``` + +### Step 8 — Analyse and report + +Parse JSON reports in `/tmp/L2_TEST_RESULTS/` and produce the outputs described below. + +--- + +## Output Format + +### A. Test Run Summary + +| Suite | Total | Passed | Failed | Errors | +|-------|-------|--------|--------|--------| +| remote-debugger start/stop | N | N | N | N | +| bootup_sequence | N | N | N | N | +| file_existence | N | N | N | N | +| log_upload | N | N | N | N | +| uploadRRDLogs_normal | N | N | N | N | +| uploadRRDLogs_error_handling | N | N | N | N | +| uploadRRDLogs_retry | N | N | N | N | +| uploadRRDLogs_strategies | N | N | N | N | +| uploadRRDLogs_security | N | N | N | N | +| uploadRRDLogs_resource_mgmt | N | N | N | N | +| usb_logupload | N | N | N | N | + +### B. Failure Analysis (one entry per failed test) + +``` +## FAIL: [.json] + +**Assertion:** + + +**Likely cause:** +<2–3 sentence root-cause hypothesis based on test code and source> + +**Suggested fix:** + +``` + +### C. Untested Functionality + +After each run, audit project components against the test suites and list areas with no L2 coverage. +Always check these areas at minimum: + +| Area | Source path | L2 coverage? | +|------|------------|-------------| +| Remote Debugger startup and initialization | `rrdMain.c`, `rrd_config.c` | ✅ | +| Bootup sequence | `remote-debugger.service` integration | ✅ | +| Remote Debugger settings file creation | Configuration files | ✅ | +| Log upload on reboot (true case) | `src/` | ✅ | +| Log upload on reboot (false case) | `src/` | ✅ | +| uploadRRDLogs trigger | `src/rrd_upload.c` | ✅ | +| uploadRRDLogs normal upload | `src/rrd_upload.c` | ✅ | +| uploadRRDLogs error handling | `src/` | ✅ | +| uploadRRDLogs retry logic | `src/` | ✅ | +| uploadRRDLogs upload strategies | `src/` | ✅ | +| uploadRRDLogs security (mTLS/OAuth) | `src/` | ✅ | +| uploadRRDLogs resource management | `src/` | ✅ | +| USB log upload | `scripts/uploadRRDLogs.sh` | ✅ | +| RBUS integration | `src/rrdRbus.h` | partial | +| Cron job parsing | `src/` | partial | +| Scheduled job management | `src/` | partial | +| Backup logs functionality | `src/` | ❌ | +| Archive manager operations | `src/` | partial | +| MD5 checksum operations | `src/` | partial | + +Update this table with actual results from each run (`✅` / `❌` / `partial`). + +--- + +## Rules and Constraints + +- **Never** run `docker pull` or `docker build` without explicit user approval. +- **Never** remove or stop `mockxconf` or `native-platform` containers without asking the user, + even if they look stale — they may be intentionally kept for debugging. +- **Never** stop or remove any container other than `mockxconf` / `native-platform` under any + circumstances. +- **Never** modify source files as part of a test run — only suggest edits. +- **Always** attempt GHCR login from `~/.netrc` before any `docker pull`; if login fails, show + the credential steps prompt and stop. +- **Always** clean up (`docker rm -f mockxconf native-platform`) at the end of a successful run, + unless the user asks to keep containers for debugging. +- If `build_inside_container.sh` fails: capture output, show the Build Failure Summary, and stop. + **Do not retry the build.** Do not attempt any workaround or source patch. +- If architecture incompatibility is detected, present the build-from-source prompt (see Step 2) + and wait for user approval before doing anything else. + +--- + +## Example Invocations + +- "Run the L2 tests and tell me what failed." +- "Run L2 tests using the images I already have." +- "Which parts of remote-debugger are not covered by L2 tests?" +- "L2 tests failed on `test_log_upload_onreboot_true_case` — what should I fix?" +- "Run uploadSTBLogs L2 tests only." diff --git a/.github/agents/legacy-refactor-specialist.agent.md b/.github/agents/legacy-refactor-specialist.agent.md new file mode 100644 index 00000000..7f60b09f --- /dev/null +++ b/.github/agents/legacy-refactor-specialist.agent.md @@ -0,0 +1,263 @@ +--- +name: 'Remote Debugger Refactoring Specialist' +description: 'Expert in safely refactoring legacy C/C++ code for the remote debugger module, ensuring maintainability and API compatibility without regressions.' +tools: ['codebase', 'search', 'edit', 'runCommands', 'problems', 'usages'] +--- + +# Legacy Code Refactoring Specialist + +You are a specialist in working with legacy embedded C/C++ code for the remote debugger module. You follow Michael Feathers' "Working Effectively with Legacy Code" principles adapted for embedded systems. + +## Your Mission + +Improve code quality, reduce technical debt, and enhance maintainability of the remote debugger while: +- **Zero regressions**: All existing tests must continue to pass +- **API stability**: Maintain backward compatibility +- **Resource constraints**: Don't increase memory footprint +- **Production safety**: Code ships to millions of devices + +## Your Process + +### 1. Understand Before Changing +- Read and analyze the remote debugger codebase thoroughly +- Identify all entry points and dependencies +- Map data flow and control flow +- Document current behavior with tests +- Find all callers using search tools + +### 2. Establish Safety Net +- Write characterization tests for existing behavior +- Run tests before ANY changes +- Use static analysis tools (cppcheck, valgrind) +- Create test coverage baseline +- Document any undefined behavior found + +### 3. Make Changes Incrementally +- One small change at a time +- Run full test suite after each change +- Verify memory usage hasn't increased +- Check for new static analysis warnings +- Commit frequently with clear messages + +### 4. Refactoring Patterns + +#### Extract Function +```c +// BEFORE: Long function with mixed concerns +int process_data(const char* input) { + // 200 lines of code doing multiple things + // Parsing, validation, transformation, storage +} + +// AFTER: Extracted, focused functions +static int validate_input(const char* input); +static int parse_data(const char* input, data_t* out); +static int store_data(const data_t* data); + +int process_data(const char* input) { + data_t data; + + if (validate_input(input) != 0) return -1; + if (parse_data(input, &data) != 0) return -1; + if (store_data(&data) != 0) return -1; + + return 0; +} +``` + +#### Introduce Seam (for testing) +```c +// BEFORE: Hard to test due to tight coupling +void process() { + FILE* f = fopen("/etc/config", "r"); + // ... process file ... + fclose(f); +} + +// AFTER: Dependency injection +typedef struct { + FILE* (*open_file)(const char* path); + // ... other dependencies ... +} dependencies_t; + +void process_with_deps(const dependencies_t* deps) { + FILE* f = deps->open_file("/etc/config"); + // ... process file ... + fclose(f); +} + +// Production code +FILE* real_open(const char* path) { return fopen(path, "r"); } +dependencies_t prod_deps = { .open_file = real_open }; + +void process() { + process_with_deps(&prod_deps); +} + +// Test code can inject mocks +``` + +#### Reduce God Object +```c +// BEFORE: Huge structure with everything +typedef struct { + char config_path[256]; + int config_version; + FILE* log_file; + void* data_buffer; + size_t buffer_size; + // ... 50 more fields ... +} context_t; + +// AFTER: Separate concerns +typedef struct { + char path[256]; + int version; +} config_t; + +typedef struct { + FILE* file; +} logger_t; + +typedef struct { + void* buffer; + size_t size; +} data_buffer_t; + +// Compose only what's needed +typedef struct { + config_t* config; + logger_t* logger; + data_buffer_t* buffer; +} context_t; +``` + +### 5. Memory Optimization Patterns + +#### Replace Heap with Stack +```c +// BEFORE: Unnecessary heap allocation +char* format_message(const char* fmt, ...) { + char* buf = malloc(256); + // ... format into buf ... + return buf; // Caller must free +} + +// AFTER: Use stack (if size is known and reasonable) +#define MSG_MAX_SIZE 256 + +int format_message(char* buf, size_t size, const char* fmt, ...) { + // ... format into buf ... + return strlen(buf); +} + +// Caller: +char msg[MSG_MAX_SIZE]; +format_message(msg, sizeof(msg), "Error: %d", code); +``` + +#### Memory Pool for Frequent Allocations +```c +// BEFORE: Frequent malloc/free causing fragmentation +for (int i = 0; i < 1000; i++) { + event_t* e = malloc(sizeof(event_t)); + process_event(e); + free(e); +} + +// AFTER: Pre-allocated pool +#define EVENT_POOL_SIZE 10 + +typedef struct { + event_t events[EVENT_POOL_SIZE]; + bool used[EVENT_POOL_SIZE]; +} event_pool_t; + +event_t* event_pool_acquire(event_pool_t* pool); +void event_pool_release(event_pool_t* pool, event_t* event); + +// Usage +event_pool_t pool = {0}; +for (int i = 0; i < 1000; i++) { + event_t* e = event_pool_acquire(&pool); + process_event(e); + event_pool_release(&pool, e); +} +``` + +## Regression Prevention + +### Before Any Refactoring +1. Ensure all remote debugger tests pass +2. Run valgrind (no leaks in current code) +3. Measure memory footprint baseline +4. Document current behavior + +### During Refactoring +1. Make one logical change at a time +2. Run tests after EVERY change +3. Use git to create checkpoint commits +4. Monitor memory usage + +### After Refactoring +1. All tests still pass +2. No new memory leaks (valgrind) +3. Memory footprint same or better +4. No new compiler warnings +5. Static analysis clean +6. Code review by human + +## Communication + +### When Proposing Changes +- Explain the problem being solved +- Show before/after comparison +- Highlight safety measures +- Document any risks +- Estimate memory impact + +### When Blocked +- Explain what's preventing progress +- Suggest alternatives +- Ask for clarification on requirements +- Note any missing tests + +### Code Review Focus +- Point out missing error handling +- Identify memory leak risks +- Note API compatibility concerns +- Suggest additional test cases +- Highlight complexity that could be simplified + +## Emergency Procedures + +If tests start failing: +1. **STOP** immediately +2. Review the last change +3. Use git diff to see what changed +4. Revert if cause isn't obvious +5. Fix the issue before continuing + +If memory leaks detected: +1. **STOP** the refactoring +2. Run valgrind to identify leak +3. Fix the leak +4. Verify fix with valgrind +5. Resume refactoring + +If API breaks: +1. **REVERT** the breaking change +2. Find alternative approach +3. Use wrapper functions if needed +4. Maintain old API alongside new + +## Success Criteria + +You've succeeded when: +- All remote debugger tests pass +- No memory leaks (valgrind clean) +- Code is more maintainable +- No API breaks +- Memory footprint same or improved +- Complexity metrics improved +- Test coverage maintained or improved diff --git a/.github/instructions/build-system.instructions.md b/.github/instructions/build-system.instructions.md new file mode 100644 index 00000000..4efca56a --- /dev/null +++ b/.github/instructions/build-system.instructions.md @@ -0,0 +1,140 @@ +--- +applyTo: "**/Makefile.am,**/configure.ac,**/*.ac,**/*.mk" +--- + +# Build System Standards (Autotools) + +## Autotools Best Practices + +### configure.ac +- Check for required headers and functions +- Provide clear error messages for missing dependencies +- Support cross-compilation +- Allow feature toggles + +```autoconf +# GOOD: Check for required features +AC_CHECK_HEADERS([pthread.h], [], + [AC_MSG_ERROR([pthread.h is required])]) + +AC_CHECK_LIB([pthread], [pthread_create], [], + [AC_MSG_ERROR([pthread library is required])]) + +# GOOD: Optional features with clear naming +AC_ARG_ENABLE([gtest], + AS_HELP_STRING([--enable-gtest], [Enable Google Test support]), + [enable_gtest=$enableval], + [enable_gtest=no]) + +AM_CONDITIONAL([WITH_GTEST_SUPPORT], [test "x$enable_gtest" = "xyes"]) +``` + +### Makefile.am +- Use non-recursive makefiles when possible +- Minimize intermediate libraries +- Support parallel builds +- Link only what's needed + +```makefile +# GOOD: Minimal linking +bin_PROGRAMS = dcmd uploadstblogs uploadlogsnow + +dcmd_SOURCES = dcm.c dcm_utils.c dcm_parseconf.c dcm_cronparse.c dcm_schedjob.c dcm_rbus.c +dcmd_CFLAGS = -DFEATURE_SUPPORT_RDKLOG +dcmd_LDADD = -lrbus -lpthread -ldl + +uploadstblogs_SOURCES = uploadstblogs/src/uploadstblogs.c +uploadstblogs_LDADD = \ + $(top_builddir)/uploadstblogs/src/libuploadstblogs.la \ + -lcurl -lssl -lcrypto -lrbus + +# GOOD: Conditional compilation +if WITH_GTEST_SUPPORT +SUBDIRS += src/unittest +endif +``` + +## Cross-Compilation Support + +### Platform Detection +```autoconf +# Support different target platforms +case "$host" in + *-linux*) + AC_DEFINE([PLATFORM_LINUX], [1], [Linux platform]) + ;; + *-arm*) + AC_DEFINE([PLATFORM_ARM], [1], [ARM platform]) + ;; +esac +``` + +### Compiler Flags +```makefile +# Platform-specific optimizations +if TARGET_ARM +AM_CFLAGS += -march=armv7-a -mfpu=neon +endif + +# Debug vs Release +if DEBUG_BUILD +AM_CFLAGS += -g -O0 -DDEBUG +else +AM_CFLAGS += -O2 -DNDEBUG +endif +``` + +## Dependency Management + +### Package Config +```autoconf +# Use pkg-config for external dependencies +PKG_CHECK_MODULES([DBUS], [dbus-1 >= 1.6]) +AC_SUBST([DBUS_CFLAGS]) +AC_SUBST([DBUS_LIBS]) +``` + +### Header Organization +```makefile +# Include paths +AM_CPPFLAGS = -I$(top_srcdir)/include \ + -I$(top_srcdir)/uploadstblogs/include \ + -I$(top_srcdir)/backup_logs/include \ + -I$(top_srcdir)/usbLogUpload/include \ + $(DBUS_CFLAGS) +``` + +## Build Performance + +### Parallel Builds +- Support `make -j` +- Avoid circular dependencies +- Use order-only prerequisites when appropriate + +### Incremental Builds +- Proper dependency tracking +- Don't force full rebuilds unless necessary +- Use libtool for shared libraries + +## Testing Integration + +```makefile +# Test targets +check-local: + @echo "Running memory leak tests..." + @for test in $(TESTS); do \ + valgrind --leak-check=full \ + --error-exitcode=1 \ + ./$$test || exit 1; \ + done + +# Code coverage +if ENABLE_COVERAGE +AM_CFLAGS += --coverage +AM_LDFLAGS += --coverage +endif + +coverage: check + $(LCOV) --capture --directory . --output-file coverage.info + $(GENHTML) coverage.info --output-directory coverage +``` diff --git a/.github/instructions/c-embedded.instructions.md b/.github/instructions/c-embedded.instructions.md new file mode 100644 index 00000000..236cb44f --- /dev/null +++ b/.github/instructions/c-embedded.instructions.md @@ -0,0 +1,693 @@ +--- +applyTo: "**/*.c,**/*.h" +--- + +# C Programming Standards for Embedded Systems + +## Memory Management + +### Allocation Rules +- **Prefer stack allocation** for fixed-size, short-lived data +- **Use malloc/free** only when necessary; always pair them +- **Check all allocations**: Never assume malloc succeeds +- **Free in reverse order** of allocation to reduce fragmentation +- **Use memory pools** for frequent same-size allocations +- **Zero memory after free** to catch use-after-free bugs in debug builds + +```c +// GOOD: Stack allocation for fixed-size data +char buffer[256]; + +// GOOD: Checked heap allocation with cleanup +char* data = malloc(size); +if (!data) { + log_error("Failed to allocate %zu bytes", size); + return ERR_NO_MEMORY; +} +// ... use data ... +free(data); +data = NULL; // Prevent double-free + +// BAD: Unchecked allocation +char* data = malloc(size); +strcpy(data, input); // Crash if malloc failed +``` + +### Memory Leak Prevention +- Every function that allocates must document ownership transfer +- Use goto for single exit point in complex error handling +- Implement cleanup functions for complex structures +- Use valgrind regularly during development + +```c +// GOOD: Single exit point with cleanup +int process_data(const char* input) { + int ret = 0; + char* buffer = NULL; + FILE* file = NULL; + + buffer = malloc(BUFFER_SIZE); + if (!buffer) { + ret = ERR_NO_MEMORY; + goto cleanup; + } + + file = fopen(input, "r"); + if (!file) { + ret = ERR_FILE_OPEN; + goto cleanup; + } + + // ... processing ... + +cleanup: + free(buffer); + if (file) fclose(file); + return ret; +} +``` + +## Resource Constraints + +### Code Size Optimization +- Avoid inline functions unless proven beneficial +- Share common code paths +- Use function pointers for conditional logic in tables +- Strip debug symbols in release builds + +### CPU Optimization +- Minimize system calls +- Cache frequently accessed data +- Use efficient algorithms (prefer O(n) over O(n²)) +- Avoid floating point on devices without FPU +- Profile before optimizing (don't guess) + +### Memory Optimization +- Use bitfields for boolean flags +- Pack structures to minimize padding +- Use const for read-only data (goes in .rodata) +- Prefer static buffers with maximum sizes when bounds are known +- Implement object pools for frequently created/destroyed objects + +```c +// GOOD: Packed structure +typedef struct __attribute__((packed)) { + uint8_t flags; + uint16_t id; + uint32_t timestamp; + char name[32]; +} telemetry_event_t; + +// GOOD: Const data in .rodata +static const char* const ERROR_MESSAGES[] = { + "Success", + "Out of memory", + "Invalid parameter", + // ... +}; +``` + +## Platform Independence + +### Never Assume +- Pointer size (use uintptr_t for pointer arithmetic) +- Byte order (use htonl/ntohl for network data) +- Structure packing (use __attribute__((packed)) or #pragma pack) +- Integer sizes (use int32_t, uint64_t from stdint.h) +- Boolean type (use stdbool.h) + +```c +// GOOD: Platform-independent types +#include +#include + +typedef struct { + uint32_t id; // Always 32 bits + uint64_t timestamp; // Always 64 bits + bool enabled; // Standard boolean +} config_t; + +// GOOD: Endianness handling +uint32_t network_value = htonl(host_value); + +// BAD: Assumptions +int id; // Size varies by platform +long timestamp; // 32 or 64 bits depending on platform +``` + +### Abstraction Layers +- Use platform abstraction for OS-specific code +- Isolate hardware dependencies +- Use configure.ac to detect platform capabilities + +## Error Handling + +### Return Value Convention +- Return 0 for success, negative for errors +- Use errno for system call failures +- Define error codes in header files +- Never ignore return values + +```c +// GOOD: Consistent error handling +typedef enum { + T2ERROR_SUCCESS = 0, + T2ERROR_FAILURE = -1, + T2ERROR_INVALID_PARAM = -2, + T2ERROR_NO_MEMORY = -3, + T2ERROR_TIMEOUT = -4 +} T2ERROR; + +T2ERROR init_telemetry() { + if (!validate_config()) { + return T2ERROR_INVALID_PARAM; + } + + if (allocate_resources() != 0) { + return T2ERROR_NO_MEMORY; + } + + return T2ERROR_SUCCESS; +} +``` + +### Logging +- Use severity levels appropriately +- Log errors with context (function, line, errno) +- Avoid logging in hot paths +- Make logging configurable at runtime +- Never log sensitive data + +```c +// GOOD: Contextual error logging +if (ret != 0) { + T2Error("%s:%d Failed to initialize: %s (errno=%d)", + __FUNCTION__, __LINE__, strerror(errno), errno); + return T2ERROR_FAILURE; +} +``` + +## Thread Safety and Concurrency + +### Critical Principles + +- **Minimize synchronization overhead**: Use lightweight primitives +- **Prevent deadlocks**: Establish lock ordering, use timeouts +- **Avoid memory fragmentation**: Configure thread stack sizes appropriately +- **Reduce contention**: Design for lock-free patterns where possible +- **Document thread safety**: Mark functions as thread-safe or not + +### Thread Creation with Minimal Memory + +Always create threads with attributes that specify required memory: + +```c +// GOOD: Thread with minimal stack size +#include + +#define THREAD_STACK_SIZE (64 * 1024) // 64KB instead of default (often 8MB) + +pthread_t thread; +pthread_attr_t attr; + +// Initialize attributes +pthread_attr_init(&attr); + +// Set minimal stack size (reduces memory fragmentation) +pthread_attr_setstacksize(&attr, THREAD_STACK_SIZE); + +// Detached threads free resources immediately when done +pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); + +// Create thread +int ret = pthread_create(&thread, &attr, thread_function, arg); +if (ret != 0) { + T2Error("Failed to create thread: %s", strerror(ret)); + pthread_attr_destroy(&attr); + return T2ERROR_FAILURE; +} + +// Clean up attributes +pthread_attr_destroy(&attr); + +// BAD: Default thread (wastes memory) +pthread_create(&thread, NULL, thread_function, arg); // Uses 8MB stack! +``` + +### Lightweight Synchronization + +Prefer lightweight synchronization primitives to avoid deadlocks and overhead: + +```c +// GOOD: Simple mutex with minimal overhead +typedef struct { + pthread_mutex_t lock; + int counter; +} thread_safe_counter_t; + +int init_counter(thread_safe_counter_t* c) { + // Use default attributes (lightest weight) + pthread_mutex_init(&c->lock, NULL); + c->counter = 0; + return 0; +} + +void increment_counter(thread_safe_counter_t* c) { + pthread_mutex_lock(&c->lock); + c->counter++; + pthread_mutex_unlock(&c->lock); +} + +void cleanup_counter(thread_safe_counter_t* c) { + pthread_mutex_destroy(&c->lock); +} + +// GOOD: Use atomic operations when possible (no locks needed) +#include + +typedef struct { + atomic_int counter; // Lock-free! +} lockfree_counter_t; + +void increment_lockfree(lockfree_counter_t* c) { + atomic_fetch_add(&c->counter, 1); // No mutex overhead +} +``` + +### Deadlock Prevention + +Follow strict rules to prevent deadlocks: + +```c +// GOOD: Consistent lock ordering +typedef struct { + pthread_mutex_t lock_a; + pthread_mutex_t lock_b; + // ... data ... +} resource_t; + +// RULE: Always acquire locks in alphabetical order (a, then b) +void multi_lock_operation(resource_t* r) { + pthread_mutex_lock(&r->lock_a); // First: lock_a + pthread_mutex_lock(&r->lock_b); // Second: lock_b + + // ... critical section ... + + pthread_mutex_unlock(&r->lock_b); // Release in reverse order + pthread_mutex_unlock(&r->lock_a); +} + +// GOOD: Use trylock with timeout to avoid indefinite blocking +#include + +int safe_lock_with_timeout(pthread_mutex_t* lock, int timeout_ms) { + struct timespec ts; + clock_gettime(CLOCK_REALTIME, &ts); + ts.tv_sec += timeout_ms / 1000; + ts.tv_nsec += (timeout_ms % 1000) * 1000000; + + int ret = pthread_mutex_timedlock(lock, &ts); + if (ret == ETIMEDOUT) { + T2Error("Lock timeout - potential deadlock detected"); + return -1; + } + return ret; +} + +// BAD: Different lock order in different functions (DEADLOCK RISK!) +void bad_function_1(resource_t* r) { + pthread_mutex_lock(&r->lock_a); + pthread_mutex_lock(&r->lock_b); // Order: a, b + // ... +} + +void bad_function_2(resource_t* r) { + pthread_mutex_lock(&r->lock_b); + pthread_mutex_lock(&r->lock_a); // Order: b, a - DEADLOCK! + // ... +} +``` + +### Avoid Heavy Synchronization + +Heavy synchronization causes performance issues and fragmentation: + +```c +// BAD: Reader-writer lock for simple counter (overkill) +pthread_rwlock_t heavy_lock; +int counter; + +void heavy_increment() { + pthread_rwlock_wrlock(&heavy_lock); // Too heavy! + counter++; + pthread_rwlock_unlock(&heavy_lock); +} + +// GOOD: Use appropriate synchronization level +atomic_int light_counter; // Lock-free for simple operations + +void light_increment() { + atomic_fetch_add(&light_counter, 1); // No lock overhead +} + +// BAD: Fine-grained locking everywhere (lock thrashing) +typedef struct { + pthread_mutex_t lock; + int value; +} each_field_locked_t; // Don't do this! + +// GOOD: Coarse-grained locking for related data +typedef struct { + pthread_mutex_t lock; + int value_a; + int value_b; + int value_c; // All protected by one lock +} properly_locked_t; +``` + +### Lock-Free Patterns + +Use lock-free patterns to avoid synchronization overhead: + +```c +// GOOD: Lock-free flag +#include + +typedef struct { + atomic_bool shutdown_requested; +} thread_control_t; + +void request_shutdown(thread_control_t* ctrl) { + atomic_store(&ctrl->shutdown_requested, true); +} + +bool should_shutdown(thread_control_t* ctrl) { + return atomic_load(&ctrl->shutdown_requested); +} + +// GOOD: Lock-free queue for single producer, single consumer +typedef struct { + atomic_int read_index; + atomic_int write_index; + void* buffer[256]; +} spsc_queue_t; + +bool spsc_enqueue(spsc_queue_t* q, void* item) { + int write = atomic_load(&q->write_index); + int next_write = (write + 1) % 256; + + if (next_write == atomic_load(&q->read_index)) { + return false; // Queue full + } + + q->buffer[write] = item; + atomic_store(&q->write_index, next_write); + return true; +} +``` + +### Minimize Critical Sections + +Keep locked sections as short as possible: + +```c +// BAD: Long critical section +void bad_process(data_t* shared) { + pthread_mutex_lock(&shared->lock); + + // Heavy computation while holding lock (BAD!) + for (int i = 0; i < 1000000; i++) { + compute_something(); + } + + shared->value = result; + pthread_mutex_unlock(&shared->lock); +} + +// GOOD: Minimal critical section +void good_process(data_t* shared) { + // Do heavy computation WITHOUT lock + int result = 0; + for (int i = 0; i < 1000000; i++) { + result += compute_something(); + } + + // Lock only for the update + pthread_mutex_lock(&shared->lock); + shared->value = result; + pthread_mutex_unlock(&shared->lock); +} +``` + +### Thread-Safe Initialization + +Use pthread_once for thread-safe initialization: + +```c +// GOOD: Thread-safe singleton initialization +static pthread_once_t init_once = PTHREAD_ONCE_INIT; +static config_t* global_config = NULL; + +static void init_config_once(void) { + global_config = malloc(sizeof(config_t)); + // ... initialize config ... +} + +config_t* get_config(void) { + pthread_once(&init_once, init_config_once); + return global_config; +} + +// BAD: Double-checked locking (broken in C without memory barriers) +static pthread_mutex_t init_lock; +static config_t* config = NULL; + +config_t* bad_get_config(void) { + if (config == NULL) { // First check (no lock) + pthread_mutex_lock(&init_lock); + if (config == NULL) { // Second check + config = malloc(sizeof(config_t)); // Race condition! + } + pthread_mutex_unlock(&init_lock); + } + return config; +} +``` + +### Thread Safety Documentation + +Always document thread safety expectations: + +```c +// GOOD: Clear thread safety documentation + +/** + * Process telemetry event + * @param event Event to process + * @return 0 on success, negative on error + * + * Thread Safety: This function is thread-safe and may be called + * from multiple threads concurrently. + */ +int process_event(const event_t* event) { + // Uses internal locking +} + +/** + * Initialize event processor + * @return 0 on success, negative on error + * + * Thread Safety: NOT thread-safe. Must be called once during + * initialization before any worker threads start. + */ +int init_event_processor(void) { + // No locking - initialization only +} + +/** + * Get current statistics + * @param stats Output buffer for statistics + * + * Thread Safety: Caller must hold stats_lock before calling. + * Use get_stats_safe() for automatic locking. + */ +void get_stats_unlocked(stats_t* stats) { + // Assumes caller holds lock +} +``` + +### Memory Fragmentation Prevention + +Configure thread pools to prevent fragmentation: + +```c +// GOOD: Thread pool with pre-allocated threads +#define THREAD_POOL_SIZE 4 +#define WORK_QUEUE_SIZE 256 + +typedef struct { + pthread_t threads[THREAD_POOL_SIZE]; + pthread_attr_t thread_attr; + // ... work queue ... +} thread_pool_t; + +int init_thread_pool(thread_pool_t* pool) { + // Configure thread attributes once + pthread_attr_init(&pool->thread_attr); + pthread_attr_setstacksize(&pool->thread_attr, THREAD_STACK_SIZE); + pthread_attr_setdetachstate(&pool->thread_attr, PTHREAD_CREATE_JOINABLE); + + // Create fixed number of threads (no dynamic allocation) + for (int i = 0; i < THREAD_POOL_SIZE; i++) { + int ret = pthread_create(&pool->threads[i], &pool->thread_attr, + worker_thread, pool); + if (ret != 0) { + // Cleanup already created threads + cleanup_partial_pool(pool, i); + return -1; + } + } + + return 0; +} + +// BAD: Creating threads dynamically (causes fragmentation) +void bad_handle_request(request_t* req) { + pthread_t thread; + pthread_create(&thread, NULL, handle_one_request, req); + pthread_detach(thread); // New thread for each request! +} +``` + +### Testing Thread Safety + +```c +// GOOD: Test for race conditions +#include + +TEST(ThreadSafety, ConcurrentIncrement) { + thread_safe_counter_t counter = {0}; + init_counter(&counter); + + const int NUM_THREADS = 10; + const int INCREMENTS_PER_THREAD = 1000; + pthread_t threads[NUM_THREADS]; + + // Create multiple threads + for (int i = 0; i < NUM_THREADS; i++) { + pthread_create(&threads[i], NULL, + increment_n_times, &counter); + } + + // Wait for all threads + for (int i = 0; i < NUM_THREADS; i++) { + pthread_join(threads[i], NULL); + } + + // Verify no race conditions + EXPECT_EQ(counter.counter, NUM_THREADS * INCREMENTS_PER_THREAD); + + cleanup_counter(&counter); +} +``` + +### Static Analysis for Concurrency + +```bash +# Use thread sanitizer to detect race conditions +gcc -g -fsanitize=thread source.c -o program +./program + +# Use helgrind (valgrind) to detect synchronization issues +valgrind --tool=helgrind ./program + +# Check for deadlocks +valgrind --tool=helgrind --track-lockorders=yes ./program +``` + +## Code Style + +### Naming Conventions +- Functions: `snake_case` (e.g., `init_telemetry`) +- Types: `snake_case_t` (e.g., `telemetry_event_t`) +- Macros/Constants: `UPPER_SNAKE_CASE` (e.g., `MAX_BUFFER_SIZE`) +- Global variables: `g_` prefix (avoid when possible) +- Static variables: `s_` prefix + +### File Organization +- One .c file per module +- Corresponding .h file for public interface +- Internal functions marked static +- Header guards in all .h files + +```c +// GOOD: header guard +#ifndef TELEMETRY_INTERNAL_H +#define TELEMETRY_INTERNAL_H + +// ... declarations ... + +#endif /* TELEMETRY_INTERNAL_H */ +``` + +## Testing Requirements + +### Unit Tests +- Test all public functions +- Test error paths and edge cases +- Use mocks for external dependencies +- Verify resource cleanup (no leaks) +- Run tests under valgrind + +### Memory Testing +```bash +# Run with memory checking +valgrind --leak-check=full --show-leak-kinds=all \ + --track-origins=yes ./test_binary + +# Static analysis +cppcheck --enable=all --inconclusive source/ +``` + +## Anti-Patterns to Avoid + +```c +// BAD: Magic numbers +if (size > 1024) { ... } + +// GOOD: Named constants +#define MAX_PACKET_SIZE 1024 +if (size > MAX_PACKET_SIZE) { ... } + +// BAD: Unchecked allocation +char* buf = malloc(size); +strcpy(buf, input); + +// GOOD: Checked with cleanup +char* buf = malloc(size); +if (!buf) return ERR_NO_MEMORY; +strncpy(buf, input, size - 1); +buf[size - 1] = '\0'; + +// BAD: Memory leak in error path +FILE* f = fopen(path, "r"); +if (condition) return -1; // Leaked f +fclose(f); + +// GOOD: Cleanup on all paths +FILE* f = fopen(path, "r"); +if (!f) return -1; +if (condition) { + fclose(f); + return -1; +} +fclose(f); +return 0; +``` + +## References + +- Project follows RDK coding standards +- See `uploadstblogs/include/` for uploadSTBLogs API header documentation +- Review existing code in `uploadstblogs/src/` for patterns +- Check `src/unittest/` directory for testing examples diff --git a/.github/instructions/cpp-testing.instructions.md b/.github/instructions/cpp-testing.instructions.md new file mode 100644 index 00000000..28739a25 --- /dev/null +++ b/.github/instructions/cpp-testing.instructions.md @@ -0,0 +1,182 @@ +--- +applyTo: "unittest/**/*.cpp,unittest/**/*.h,uploadstblogs/unittest/**/*.cpp,uploadstblogs/unittest/**/*.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: unittest/dcm_utils_gtest.cpp + +extern "C" { +#include "dcm_utils.h" +#include "dcm_types.h" +} + +#include +#include + +class DcmUtilsTest : public ::testing::Test { +protected: + void SetUp() override { + // Initialize test resources + } + + void TearDown() override { + // Clean up test resources + } +}; + +TEST_F(DcmUtilsTest, ConfigFileReadWriteRoundTrip) { + // Test configuration file parsing + const char* config = "/tmp/test.conf"; + // verify read back value matches written value + ASSERT_EQ(readConfigValue(config, "key"), "value"); +} +``` + +## Testing Patterns + +### Test C Code from C++ +- Wrap C headers in `extern "C"` blocks +- Use RAII in tests for automatic cleanup +- Mock C functions using gmock when needed + +```cpp +extern "C" { +#include "dcm_parseconf.h" +#include "dcm_rbus.h" +} + +#include + +class DcmParseConfTest : public ::testing::Test { +protected: + void SetUp() override { + // Initialize handler stubs + } + + void TearDown() override { + // Clean up + } +}; + +TEST_F(DcmParseConfTest, ParseConfigReturnsExpected) { + DCMDHandle handle = {}; + // Test configuration parsing + int result = dcmParseConfig(&handle, "/etc/dcmresponse.txt"); + // verify handler returns success and populates configuration + ASSERT_EQ(result, 0); +} +``` + +### Memory Leak Testing +- All tests must pass valgrind +- Use RAII wrappers for C resources +- Verify cleanup in TearDown + +```cpp +// GOOD: RAII wrapper for C resource +class FileHandle { + FILE* file_; +public: + explicit FileHandle(const char* path, const char* mode) + : file_(fopen(path, mode)) {} + + ~FileHandle() { + if (file_) fclose(file_); + } + + FILE* get() const { return file_; } + bool valid() const { return file_ != nullptr; } +}; + +TEST(FileTest, ReadConfig) { + FileHandle file("/tmp/config.json", "r"); + ASSERT_TRUE(file.valid()); + // file automatically closed when test exits +} +``` + +### Mocking External Dependencies + +```cpp +// GOOD: Mock for handler dependencies +class MockIniFile { +public: + MOCK_METHOD(std::string, get, (const std::string& key)); + MOCK_METHOD(bool, set, (const std::string& key, const std::string& value)); +}; + +TEST(HandlerTest, GetParamUsesIniFile) { + MockIniFile mock; + + EXPECT_CALL(mock, get("Device.DeviceInfo.Manufacturer")) + .WillOnce(testing::Return("TestVendor")); + + std::string result = mock.get("Device.DeviceInfo.Manufacturer"); + EXPECT_EQ("TestVendor", result); +} +``` + +## Test Quality Standards + +### Coverage Requirements +- All public functions must have tests +- Test both success and failure paths +- Test boundary conditions +- Test error handling + +### Test Naming +```cpp +// Pattern: TEST(ComponentName, BehaviorBeingTested) + +TEST(Vector, CreateReturnsNonNull) { ... } +TEST(Vector, DestroyHandlesNull) { ... } +TEST(Vector, PushIncrementsSize) { ... } +TEST(Utils, ParseConfigInvalidJson) { ... } +``` + +### Assertions +- Use `ASSERT_*` when test can't continue after failure +- Use `EXPECT_*` when subsequent checks are still valuable +- Provide helpful failure messages + +```cpp +// GOOD: Informative assertions +ASSERT_NE(nullptr, ptr) << "Failed to allocate " << size << " bytes"; +EXPECT_EQ(expected, actual) << "Mismatch at index " << i; +EXPECT_TRUE(condition) << "Context: " << debug_info; +``` + +## Running Tests + +### Build Tests +```bash +./configure --enable-gtest +make check +``` + +### Memory Checking +```bash +valgrind --leak-check=full --show-leak-kinds=all \ + ./unittest/dcm_gtest + +valgrind --leak-check=full --show-leak-kinds=all \ + ./uploadstblogs/unittest/uploadstblogs_gtest +``` + +### Test Output +- Use `GTEST_OUTPUT=xml:results.xml` for CI integration +- Check return code: 0 = all passed diff --git a/.github/instructions/shell-scripts.instructions.md b/.github/instructions/shell-scripts.instructions.md new file mode 100644 index 00000000..a25a2c69 --- /dev/null +++ b/.github/instructions/shell-scripts.instructions.md @@ -0,0 +1,179 @@ +--- +applyTo: "**/*.sh" +--- + +# Shell Script Standards for Embedded Systems + +## Platform Independence + +### Use POSIX Shell +- Use `#!/bin/sh` not `#!/bin/bash` +- Avoid bashisms (use shellcheck to verify) +- Test on busybox ash (common in embedded) + +```bash +#!/bin/sh +# GOOD: POSIX compliant + +# BAD: Bash-specific +if [[ $var == "value" ]]; then # Use [ ] instead + array=(1 2 3) # Arrays not in POSIX +fi + +# GOOD: POSIX compliant +if [ "$var" = "value" ]; then + set -- 1 2 3 # Use positional parameters +fi +``` + +## Resource Awareness + +### Minimize Process Spawning +- Use shell builtins when possible +- Avoid pipes when not necessary +- Batch operations to reduce forks + +```bash +# BAD: Multiple processes +cat file | grep pattern | wc -l + +# GOOD: Fewer processes +grep -c pattern file + +# BAD: Loop with external commands +for file in *.txt; do + cat "$file" >> output +done + +# GOOD: Single cat invocation +cat *.txt > output +``` + +### Memory Usage +- Avoid reading entire files into variables +- Process streams line by line +- Clean up temporary files + +```bash +# BAD: Loads entire file into memory +content=$(cat large_file.log) +echo "$content" | grep ERROR + +# GOOD: Stream processing +grep ERROR large_file.log + +# GOOD: Line-by-line processing +while IFS= read -r line; do + process_line "$line" +done < large_file.log +``` + +## Error Handling + +### Always Check Exit Codes +```bash +# GOOD: Check critical operations +if ! mkdir -p /tmp/telemetry; then + logger -t telemetry "ERROR: Failed to create directory" + exit 1 +fi + +# GOOD: Use set -e for fail-fast +set -e # Exit on any error +set -u # Exit on undefined variable +set -o pipefail # Catch errors in pipes + +# GOOD: Trap for cleanup +cleanup() { + rm -f "$TEMP_FILE" +} +trap cleanup EXIT INT TERM + +TEMP_FILE=$(mktemp) +# ... use temp file ... +# cleanup happens automatically +``` + +## Script Quality + +### Defensive Programming +```bash +# GOOD: Quote all variables +rm -f "$file_path" # Not: rm -f $file_path + +# GOOD: Use -- to separate options from arguments +grep -r -- "$pattern" "$directory" + +# GOOD: Check variable is set +: "${CONFIG_FILE:?CONFIG_FILE must be set}" + +# GOOD: Validate inputs +if [ -z "$1" ]; then + echo "Usage: $0 " >&2 + exit 1 +fi +``` + +### Logging +```bash +# Use logger for syslog integration +log_info() { + logger -t telemetry -p user.info "$*" +} + +log_error() { + logger -t telemetry -p user.error "$*" + echo "ERROR: $*" >&2 +} + +# Usage +log_info "Starting telemetry collection" +if ! start_service; then + log_error "Failed to start service" + exit 1 +fi +``` + +## Testing Scripts + +### Use shellcheck +```bash +# Run shellcheck on all scripts +shellcheck script.sh + +# In CI +find . -name "*.sh" -exec shellcheck {} + +``` + +### Test on Target Platform +- Test on actual embedded device or emulator +- Verify with busybox tools +- Check resource usage (memory, CPU) + +## Anti-Patterns + +```bash +# BAD: Unquoted variables +for file in $FILES; do # Word splitting! + +# GOOD: Quoted +for file in "$FILES"; do + +# BAD: Parsing ls output +for file in $(ls *.txt); do + +# GOOD: Use glob +for file in *.txt; do + +# BAD: Useless use of cat +cat file | grep pattern + +# GOOD: grep can read files +grep pattern file + +# BAD: Not checking if file exists +rm /tmp/file # Error if doesn't exist + +# GOOD: Check or use -f +rm -f /tmp/file # Or: [ -f /tmp/file ] && rm /tmp/file +``` diff --git a/.github/skills/memory-safety-analyzer/SKILL.md b/.github/skills/memory-safety-analyzer/SKILL.md new file mode 100644 index 00000000..5d2d9b29 --- /dev/null +++ b/.github/skills/memory-safety-analyzer/SKILL.md @@ -0,0 +1,227 @@ +--- +name: memory-safety-analyzer +description: Analyze C/C++ code for memory safety issues including leaks, use-after-free, buffer overflows, and provide fixes. Use when reviewing memory management, debugging crashes, or improving code safety. +--- + +# Memory Safety Analysis for Embedded C + +## Purpose + +Systematically analyze C/C++ code for memory safety issues that can cause crashes, security vulnerabilities, or resource exhaustion in embedded systems. + +## Usage + +Invoke this skill when: +- Reviewing new code with dynamic memory allocation +- Debugging memory-related crashes +- Analyzing legacy code for safety issues +- Preparing code for production deployment +- Investigating memory leaks or fragmentation + +## Analysis Process + +### Step 1: Identify All Allocations + +Search the code for: +- `malloc`, `calloc`, `realloc` +- `strdup`, `strndup` +- `fopen`, `open` +- `pthread_create`, `pthread_mutex_init` +- Custom allocation functions + +For each allocation, verify: +1. Return value is checked +2. Corresponding free/close exists +3. Error paths also free resources +4. No double-free possible + +### Step 2: Check Pointer Lifetimes + +For each pointer variable: +- When is it assigned? +- When is it freed? +- Can it be used after free? +- Can it outlive the data it points to? +- Is it NULL-initialized? +- Is it NULL-checked before use? + +### Step 3: Analyze Error Paths + +For each error return: +- Are all resources freed? +- Is cleanup done in correct order? +- Are error codes accurate? +- Is logging appropriate? + +### Step 4: Review Buffer Operations + +For string and memory operations: +- `strcpy` → should be `strncpy` with size check +- `sprintf` → should be `snprintf` with size +- `gets` → never use (remove immediately) +- `strcat` → verify buffer size +- `memcpy` → verify no overlap, validate size + +### Step 5: Static Analysis + +Run tools: +```bash +# Cppcheck +cppcheck --enable=all --inconclusive file.c + +# Clang static analyzer +scan-build make + +# Compiler warnings +gcc -Wall -Wextra -Werror file.c +``` + +### Step 6: Dynamic Analysis + +Run valgrind: +```bash +valgrind --leak-check=full \ + --show-leak-kinds=all \ + --track-origins=yes \ + --verbose \ + ./program +``` + +## Common Issues and Fixes + +### Issue: Unchecked malloc + +```c +// PROBLEM +char* buffer = malloc(size); +strcpy(buffer, input); // Crash if malloc failed + +// FIX +char* buffer = malloc(size); +if (!buffer) { + log_error("Failed to allocate %zu bytes", size); + return ERR_NO_MEMORY; +} +strncpy(buffer, input, size - 1); +buffer[size - 1] = '\0'; +``` + +### Issue: Memory leak on error + +```c +// PROBLEM +int process() { + char* buf = malloc(1024); + FILE* f = fopen("file.txt", "r"); + + if (!f) return -1; // Leaked buf + + // ... process ... + + free(buf); + fclose(f); + return 0; +} + +// FIX: Single exit with cleanup +int process() { + int ret = 0; + char* buf = NULL; + FILE* f = NULL; + + buf = malloc(1024); + if (!buf) { + ret = ERR_NO_MEMORY; + goto cleanup; + } + + f = fopen("file.txt", "r"); + if (!f) { + ret = ERR_FILE_OPEN; + goto cleanup; + } + + // ... process ... + +cleanup: + free(buf); + if (f) fclose(f); + return ret; +} +``` + +### Issue: Use after free + +```c +// PROBLEM +free(ptr); +if (ptr->field > 0) { ... } // Use after free! + +// FIX +int value = ptr->field; +free(ptr); +ptr = NULL; +if (value > 0) { ... } +``` + +### Issue: Double free + +```c +// PROBLEM +free(ptr); +// ... later ... +free(ptr); // Double free! + +// FIX: NULL after free +free(ptr); +ptr = NULL; +// ... later ... +free(ptr); // Safe: free(NULL) is a no-op +``` + +### Issue: Buffer overflow + +```c +// PROBLEM +char buffer[100]; +strcpy(buffer, user_input); // Overflow if input > 99 chars + +// FIX +char buffer[100]; +strncpy(buffer, user_input, sizeof(buffer) - 1); +buffer[sizeof(buffer) - 1] = '\0'; +``` + +## Output Format + +Provide findings as: + +``` +## Memory Safety Analysis + +### Critical Issues (must fix) +1. [file.c:123] Unchecked malloc - potential NULL dereference +2. [file.c:456] Memory leak on error path - buffer not freed +3. [file.c:789] Use after free - ptr used after free() + +### Warnings (should fix) +1. [file.c:234] strcpy used - prefer strncpy +2. [file.c:567] Missing NULL check before pointer use + +### Recommendations +1. Add cleanup label for resource management +2. Use RAII wrapper in tests +3. Run valgrind in CI pipeline + +### Suggested Fixes +[Provide specific code changes for each issue] +``` + +## Verification + +After fixes: +1. All static analysis warnings resolved +2. Valgrind shows no leaks +3. All tests pass +4. Code review by human +5. Memory footprint measured and acceptable diff --git a/.github/skills/platform-portability-checker/SKILL.md b/.github/skills/platform-portability-checker/SKILL.md new file mode 100644 index 00000000..aa9c5589 --- /dev/null +++ b/.github/skills/platform-portability-checker/SKILL.md @@ -0,0 +1,318 @@ +--- +name: platform-portability-checker +description: Verify C/C++ code is platform-independent and portable across embedded platforms. Use when reviewing code for cross-platform deployment or preparing for new hardware targets. +--- + +# Platform Portability Checker + +## Purpose + +Ensure C/C++ code is portable across different embedded platforms, architectures, and operating systems without modification. + +## When to Use + +- Reviewing new code before merge +- Porting to new hardware platform +- Preparing release for multiple architectures +- Investigating platform-specific bugs +- Refactoring legacy platform-specific code + +## Portability Checklist + +### 1. Integer Types + +**Check for**: Use of `int`, `long`, `short` without fixed sizes + +```c +// PROBLEM: Size varies by platform +int counter; // 16, 32, or 64 bits? +long timestamp; // 32 or 64 bits? +short flag; // 16 bits on most, but not guaranteed + +// FIX: Use stdint.h types +#include + +uint32_t counter; // Always 32 bits +uint64_t timestamp; // Always 64 bits +uint16_t flag; // Always 16 bits + +// For size_t operations +size_t length; // Pointer-sized unsigned +ssize_t result; // Pointer-sized signed +``` + +### 2. Pointer Assumptions + +**Check for**: Pointer arithmetic, casting, size assumptions + +```c +// PROBLEM: Assumes pointer == long +long ptr_value = (long)ptr; // Fails on 64-bit with 32-bit long + +// FIX: Use uintptr_t +#include +uintptr_t ptr_value = (uintptr_t)ptr; + +// PROBLEM: Pointer used as integer +if (ptr & 0x1) { ... } // What size is ptr? + +// FIX: Be explicit +if ((uintptr_t)ptr & 0x1) { ... } +``` + +### 3. Endianness + +**Check for**: Multi-byte values sent over network or stored to disk + +```c +// PROBLEM: Host byte order assumed +uint32_t value = 0x12345678; +fwrite(&value, 4, 1, file); // Different on LE vs BE + +// FIX: Explicit byte order +#include // For htonl, ntohl + +uint32_t host_value = 0x12345678; +uint32_t network_value = htonl(host_value); +fwrite(&network_value, 4, 1, file); + +// For reading +uint32_t network_value; +fread(&network_value, 4, 1, file); +uint32_t host_value = ntohl(network_value); +``` + +### 4. Structure Packing + +**Check for**: Structures sent over network or saved to disk + +```c +// PROBLEM: Padding varies by platform +struct { + uint8_t type; + uint32_t value; // Padding before this? + uint16_t flags; // Padding before this? +} data; + +// FIX: Explicit packing +struct __attribute__((packed)) { + uint8_t type; + uint32_t value; + uint16_t flags; +} data; + +// Or control padding explicitly +struct { + uint8_t type; + uint8_t padding[3]; // Explicit padding + uint32_t value; + uint16_t flags; + uint16_t padding2; +} data; +``` + +### 5. Boolean Type + +**Check for**: Using int/char for boolean + +```c +// PROBLEM: Non-standard boolean +int flag; // Really 3 states: 0, 1, other +char enabled; // Also used for booleans + +// FIX: Use stdbool.h +#include + +bool flag; +bool enabled; + +if (flag) { ... } // Clear intent +``` + +### 6. Character Sets + +**Check for**: Assumptions about ASCII or character encoding + +```c +// PROBLEM: Assumes ASCII +if (ch >= 'A' && ch <= 'Z') { + ch = ch + 32; // Convert to lowercase? +} + +// FIX: Use standard functions +#include + +if (isupper(ch)) { + ch = tolower(ch); +} +``` + +### 7. File Paths + +**Check for**: Hard-coded path separators + +```c +// PROBLEM: Unix-specific +const char* path = "/tmp/telemetry/data.log"; + +// FIX: Use platform-agnostic approach +#ifdef _WIN32 + #define PATH_SEP "\\" + const char* tmp_dir = getenv("TEMP"); +#else + #define PATH_SEP "/" + const char* tmp_dir = "/tmp"; +#endif + +char path[256]; +snprintf(path, sizeof(path), "%s%stelemetry%sdata.log", + tmp_dir, PATH_SEP, PATH_SEP); +``` + +### 8. System Calls + +**Check for**: Platform-specific syscalls + +```c +// PROBLEM: Linux-specific +#include +int fd = epoll_create(10); + +// FIX: Abstraction layer +// In platform.h +#if defined(__linux__) + #include "platform_linux.h" +#elif defined(__APPLE__) + #include "platform_darwin.h" +#else + #error "Unsupported platform" +#endif + +// Each platform provides same interface +event_loop_t* create_event_loop(void); +``` + +### 9. Compiler Extensions + +**Check for**: GCC/Clang specific features + +```c +// PROBLEM: GCC-specific +typeof(x) y = x; +int array[0]; // Zero-length array + +// FIX: Avoid compiler-specific typeof/__auto_type; use standard types +int y = x; // declare with explicit type + +// Or avoid non-standard features +// Define proper types instead +``` + +### 10. Include Paths + +**Check for**: Platform-specific headers + +```c +// PROBLEM: Assumes Linux headers +#include + +// FIX: Use standard headers or configure check +#ifdef HAVE_LINUX_LIMITS_H + #include +#else + #include +#endif + +// Or use autoconf to detect +// In configure.ac: +// AC_CHECK_HEADERS([linux/limits.h limits.h]) +``` + +## Build System Integration + +### configure.ac checks + +```autoconf +# Check for required features +AC_C_BIGENDIAN +AC_CHECK_SIZEOF([int]) +AC_CHECK_SIZEOF([long]) +AC_CHECK_SIZEOF([void *]) + +# Check for headers +AC_CHECK_HEADERS([stdint.h stdbool.h endian.h]) + +# Check for functions +AC_CHECK_FUNCS([htonl ntohl]) + +# Platform-specific code +case "$host" in + *-linux*) + AC_DEFINE([PLATFORM_LINUX], [1]) + ;; + arm*|*-arm*) + AC_DEFINE([PLATFORM_ARM], [1]) + ;; +esac +``` + +## Testing + +### Cross-Compilation Test + +```bash +# Test building for different architectures +./configure --host=arm-linux-gnueabihf +make clean && make + +./configure --host=x86_64-linux-gnu +make clean && make + +./configure --host=mips-linux-gnu +make clean && make +``` + +### Endianness Test + +```c +// Test endianness handling +uint32_t value = 0x12345678; +uint32_t network = htonl(value); +uint32_t restored = ntohl(network); +assert(value == restored); + +// Verify structure packing +assert(sizeof(packed_struct_t) == EXPECTED_SIZE); +``` + +## Output Format + +``` +## Platform Portability Analysis + +### Critical Issues +1. [file.c:123] Using `long` for timestamp - not fixed width +2. [file.c:456] Writing struct directly to network - endianness issue +3. [file.c:789] Assuming 32-bit pointers + +### Warnings +1. [file.c:234] Using int for boolean - prefer stdbool.h +2. [file.c:567] Hard-coded Unix path separator + +### Recommendations +1. Add configure checks for required headers +2. Create platform abstraction layer +3. Test build on multiple architectures + +### Suggested Fixes +[Specific code changes for each issue] +``` + +## Verification + +- Code compiles on target platforms +- Tests pass on all platforms +- Static analysis clean +- No endianness issues +- No alignment issues +- Structure sizes verified diff --git a/.github/skills/quality-checker/README.md b/.github/skills/quality-checker/README.md new file mode 100644 index 00000000..ae73caab --- /dev/null +++ b/.github/skills/quality-checker/README.md @@ -0,0 +1,72 @@ +# Quality Checker Skill + +Run comprehensive remote debugger quality checks in the standard test container through the chat interface. + +## Quick Start + +Simply ask Copilot to run quality checks in natural language: + +```text +Run quality checks +``` + +```text +Check memory safety +``` + +```text +Run static analysis on src +``` + +## What Gets Checked + +1. **Static Analysis**: cppcheck + shellcheck +2. **Memory Safety**: valgrind leak detection +3. **Thread Safety**: helgrind race/deadlock detection +4. **Build Verification**: strict warnings compilation + +## Environment + +Runs in the same container family used by this repository's CI: + +- Image: `ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest` +- All tools pre-installed +- Consistent with automated tests + +## Example Invocations + +| What to say | What it does | +| ----------- | ------------ | +| "Run quality checks" | Full suite, summary report | +| "Quick static analysis" | cppcheck + shellcheck only | +| "Check for memory leaks" | valgrind on test binaries | +| "Verify build with strict warnings" | Build with -Werror | +| "Run all checks on src" | Full suite, scoped to remote debugger sources | + +## Typical Workflow + +1. **Before committing**: "Run static analysis" +2. **Before push**: "Run quality checks" +3. **Debugging crash**: "Check memory safety" +4. **Reviewing PR**: "Run all checks" + +## Output + +You'll receive: + +- Summary of issues found +- Critical problems highlighted +- Links to detailed reports +- Recommendations for fixes + +## Prerequisites + +- Docker installed and running +- Access to GitHub Container Registry if the image is not already cached locally + +## Tips + +- Start with static analysis (fastest) +- Run memory checks after static analysis passes +- Scope checks to changed files for speed +- Full suite before pushing to develop branch diff --git a/.github/skills/quality-checker/SKILL.md b/.github/skills/quality-checker/SKILL.md new file mode 100644 index 00000000..6da041f6 --- /dev/null +++ b/.github/skills/quality-checker/SKILL.md @@ -0,0 +1,325 @@ +--- +name: quality-checker +description: Run comprehensive quality checks (static analysis, memory safety, thread safety, build verification) in the standard test container. Use when validating code changes or debugging before committing. +--- + +# Container-Based Quality Checker + +## Purpose + +Execute comprehensive quality checks on the remote debugger codebase using the same containerized environment as CI pipelines. Ensures consistency between local development and automated testing. + +## Usage + +Invoke this skill when: +- Validating changes before committing +- Debugging build or test failures +- Running quality checks locally +- Verifying memory safety of new code +- Checking for thread safety issues +- Performing static analysis + +You can run all checks or select specific ones based on your needs. + +## What It Does + +This skill runs quality checks inside the official test container used by this repository (`ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest`), which includes: +- Build tools (gcc, g++, autotools, make) +- Static analysis tools (cppcheck, shellcheck) +- Memory analysis tools (valgrind) +- Thread analysis tools (helgrind) +- Google Test/Mock frameworks + +## Available Checks + +### 1. Static Analysis +- **cppcheck**: Comprehensive C/C++ static code analyzer +- **shellcheck**: Shell script linter +- **Output**: XML report with findings + +### 2. Memory Safety (Valgrind) +- **Memory leak detection**: Finds unreleased allocations +- **Use-after-free detection**: Catches dangling pointer usage +- **Invalid memory access**: Buffer overflows, uninitialized reads +- **Output**: XML and log files per test binary + +### 3. Thread Safety (Helgrind) +- **Race condition detection**: Finds unsynchronized shared memory access +- **Deadlock detection**: Identifies lock ordering issues +- **Lock usage verification**: Validates proper synchronization +- **Output**: XML and log files per test binary + +### 4. Build Verification +- **Strict compilation**: Builds with `-Wall -Wextra -Werror` +- **Test build**: Verifies tests compile +- **Binary analysis**: Reports size and dependencies +- **Output**: Build artifacts and size report + +## Execution Process + +### Step 1: Setup Container Environment + +Pull the latest test container: +```bash +docker pull ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest +``` + +Start container with workspace mounted: +```bash +docker run -d --name native-platform \ + -v /path/to/workspace:/mnt/workspace \ + ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest +``` + +### Step 2: Run Selected Checks + +Execute the requested quality checks inside the container: + +**Static Analysis:** +```bash +docker exec -i native-platform /bin/bash -c " + cd /mnt/workspace && \ + cppcheck --enable=all \ + --inconclusive \ + --suppress=missingIncludeSystem \ + --suppress=unmatchedSuppression \ + --error-exitcode=0 \ + --xml \ + --xml-version=2 \ + . 2> cppcheck-report.xml +" +``` + +**Shell Script Checks:** +```bash +docker exec -i native-platform /bin/bash -c " + cd /mnt/workspace && \ + find . -name '*.sh' -type f -exec shellcheck {} + +" +``` + +**Memory Safety:** +```bash +docker exec -i native-platform /bin/bash -c " + cd /mnt/workspace/src/unittest && \ + automake --add-missing && \ + autoreconf --install && \ + ./configure && \ + make -j\$(nproc) && \ + find . -type f -executable -name '*gtest*' 2>/dev/null | while read test_bin; do + valgrind --leak-check=full \ + --show-leak-kinds=all \ + --track-origins=yes \ + --xml=yes \ + --xml-file=\"valgrind-\$(basename \$test_bin).xml\" \ + \"\$test_bin\" 2>&1 | tee \"valgrind-\$(basename \$test_bin).log\" + done +" +``` + +**Thread Safety:** +```bash +docker exec -i native-platform /bin/bash -c " + cd /mnt/workspace/src/unittest && \ + find . -type f -executable -name '*gtest*' 2>/dev/null | while read test_bin; do + valgrind --tool=helgrind \ + --track-lockorders=yes \ + --xml=yes \ + --xml-file=\"helgrind-\$(basename \$test_bin).xml\" \ + \"\$test_bin\" 2>&1 | tee \"helgrind-\$(basename \$test_bin).log\" + done +" +``` + +**Build Verification:** +```bash +docker exec -i native-platform /bin/bash -c " + cd /mnt/workspace && \ + sh -e cov_build.sh && \ + if [ -f 'src/remotedebugger' ]; then + ls -lh src/remotedebugger + file src/remotedebugger + size src/remotedebugger + fi + if [ -f 'src/unittest/remotedebugger_gtest' ]; then + ls -lh src/unittest/remotedebugger_gtest + file src/unittest/remotedebugger_gtest + size src/unittest/remotedebugger_gtest + fi +" +``` + +### Step 3: Report Results + +Parse and summarize results for the user: +- Number of issues found by category +- Critical issues requiring immediate attention +- Warnings that should be addressed +- Memory leaks with stack traces +- Race conditions or deadlock risks +- Build errors or warnings + +### Step 4: Cleanup + +Stop and remove the container: +```bash +docker stop native-platform +docker rm native-platform +``` + +## Interpreting Results + +### Static Analysis (cppcheck) +- **error**: Critical issues that must be fixed +- **warning**: Potential problems to review +- **style**: Code style improvements +- **performance**: Optimization opportunities + +### Memory Safety (Valgrind) +- **definitely lost**: Memory leaks requiring fixes +- **indirectly lost**: Leaks from lost parent structures +- **possibly lost**: Potential leaks to investigate +- **still reachable**: Memory held at exit (usually OK) +- **Invalid read/write**: Buffer overflow (CRITICAL) +- **Use of uninitialized value**: Must initialize before use + +### Thread Safety (Helgrind) +- **Possible data race**: Unsynchronized access to shared data +- **Lock order violation**: Potential deadlock scenario +- **Unlocking unlocked lock**: Synchronization bug +- **Thread still holds locks**: Resource leak + +### Build Verification +- **Compilation errors**: Must fix before proceeding +- **Warnings**: Review and fix (builds with -Werror) +- **Binary size**: Monitor for embedded constraints + +## User Interaction + +When invoked, ask the user: + +1. **Which checks to run?** + - All checks (comprehensive) + - Static analysis only (fast) + - Memory safety only + - Thread safety only + - Build verification only + - Custom combination + +2. **Scope:** + - Full codebase + - Specific directories + - Recently changed files + +3. **Report detail:** + - Summary only (counts and critical issues) + - Detailed (all findings) + - Full raw output + +## Example Invocations + +**User**: "Run quality checks" +- Default: Run all checks on full codebase, provide summary + +**User**: "Check memory safety" +- Run only valgrind checks, detailed report + +**User**: "Quick static analysis" +- Run cppcheck and shellcheck, summary only + +**User**: "Verify my changes build" +- Run build verification with strict warnings + +**User**: "Full analysis on src" +- Run all checks scoped to the remote debugger sources + +## Best Practices + +1. **Run before committing**: Catch issues early +2. **Start with static analysis**: Fastest feedback +3. **Run memory checks on test binaries**: Most effective +4. **Review thread safety for concurrent code**: Essential for multi-threaded components +5. **Monitor binary size**: Important for embedded targets + +## Integration with Development Workflow + +1. **Pre-commit**: Quick static analysis +2. **Pre-push**: Full quality check suite +3. **Debugging**: Targeted memory/thread analysis +4. **Code review**: Validate reviewer feedback +5. **Refactoring**: Ensure no regressions + +## Advantages Over Manual Testing + +- **Consistency**: Same environment as CI/CD +- **Completeness**: All tools in one command +- **Reproducibility**: Container ensures identical results +- **Efficiency**: No local tool installation needed +- **Confidence**: Pass locally = pass in CI + +## Output Files Generated + +- `cppcheck-report.xml`: Static analysis findings +- `valgrind-.xml`: Memory issues per test +- `valgrind-.log`: Detailed memory logs +- `helgrind-.xml`: Thread safety issues per test +- `helgrind-.log`: Detailed concurrency logs + +These files can be uploaded as artifacts or reviewed locally. + +## Limitations + +- Requires Docker with GitHub Container Registry access +- Container pulls can be slow on first run (cached afterward) +- Full suite can take several minutes depending on codebase size +- Valgrind slows execution significantly (expected) + +## Tips for Faster Execution + +1. Use cached container images (don't pull every time) +2. Run static analysis first (fastest) +3. Scope checks to changed directories +4. Run memory/thread checks only on affected tests +5. Use parallel execution where possible + +## Skill Execution Logic + +When user invokes this skill: + +1. **Check local image first** + - Reuse the existing native-platform image when present + - Pull only when needed and only with user approval outside CI + +2. **Start container** + - Mount workspace at /mnt/workspace + - Use unique container name (quality-checker-) + - Run in detached mode + +3. **Execute requested checks** + - Run checks in sequence + - Capture output + - Continue on errors (collect all findings) + +4. **Collect results** + - Copy result files from container + - Parse XML/log outputs + - Categorize findings + +5. **Report to user** + - Summary count + - Critical issues highlighted + - Link to detailed reports + - Next steps recommendations + +6. **Cleanup** + - Stop container + - Remove container + - Optional: clean up result files + +## Error Handling + +- **Container pull fails**: Report error, suggest manual pull +- **Container start fails**: Check Docker daemon, ports, permissions +- **Build fails**: Report build errors, stop further checks +- **Tools missing**: Verify container version, report missing tools +- **Out of memory**: Suggest increasing Docker memory limit diff --git a/.github/skills/technical-documentation-writer/SKILL.md b/.github/skills/technical-documentation-writer/SKILL.md new file mode 100644 index 00000000..bd9cff1a --- /dev/null +++ b/.github/skills/technical-documentation-writer/SKILL.md @@ -0,0 +1,712 @@ +--- +name: technical-documentation-writer +description: Create and maintain comprehensive technical documentation for embedded systems projects. Use for architecture docs, API references, developer guides, and system documentation following best practices. +--- + +# Technical Documentation Writer for Embedded Systems + +## Purpose + +Create clear, comprehensive, and maintainable technical documentation for embedded C/C++ projects, with focus on architecture, APIs, threading models, memory management, and platform integration. + +## Usage + +Invoke this skill when: +- Documenting new features or components +- Creating system architecture documentation +- Writing API reference documentation +- Documenting threading and synchronization models +- Creating developer onboarding guides +- Documenting debugging procedures +- Writing integration guides for platform vendors + +## Documentation Structure + +### Directory Layout + +``` +project/ +├── README.md # Project overview, quick start +├── docs/ # General documentation +│ ├── README.md # Documentation index +│ ├── architecture/ # System architecture +│ │ ├── overview.md # High-level architecture +│ │ ├── component-diagram.md # Component relationships +│ │ ├── threading-model.md # Threading architecture +│ │ └── data-flow.md # Data flow diagrams +│ ├── api/ # API documentation +│ │ ├── public-api.md # Public API reference +│ │ └── internal-api.md # Internal API reference +│ ├── integration/ # Integration guides +│ │ ├── build-setup.md # Build environment setup +│ │ ├── platform-porting.md # Porting to new platforms +│ │ └── testing.md # Test procedures +│ └── troubleshooting/ # Debug guides +│ ├── memory-issues.md # Memory debugging +│ ├── threading-issues.md # Thread debugging +│ └── common-errors.md # Common error solutions +└── source/ # Source code + └── docs/ # Component-specific docs + ├── bulkdata/ # Mirrors source structure + │ ├── README.md # Component overview + │ └── profile-management.md + ├── protocol/ + │ ├── README.md + │ └── http-architecture.md + └── scheduler/ + ├── README.md + └── scheduling-algorithm.md +``` + +### Document Types + +#### 1. **Architecture Documentation** (`docs/architecture/`) +- System overview and design principles +- Component relationships and dependencies +- Threading and concurrency models +- Data flow and state machines +- Memory management strategies +- Platform abstraction layers + +#### 2. **API Documentation** (`docs/api/`) +- Public API reference with examples +- Internal API documentation +- Function contracts and preconditions +- Thread-safety guarantees +- Memory ownership semantics +- Error handling conventions + +#### 3. **Component Documentation** (`source/docs/`) +- Per-component technical details +- Algorithm explanations +- Implementation notes +- Performance characteristics +- Resource usage (memory, CPU, threads) +- Dependencies and interfaces + +#### 4. **Integration Guides** (`docs/integration/`) +- Build system setup +- Platform porting guides +- Configuration options +- Testing procedures +- Deployment checklists + +#### 5. **Troubleshooting Guides** (`docs/troubleshooting/`) +- Common error scenarios +- Debug techniques +- Log analysis +- Memory profiling +- Thread race detection + +## Documentation Process + +### Step 1: Analyze the Code + +Before writing documentation: + +1. **Read the source code** - Understand implementation +2. **Identify key abstractions** - Classes, structs, modules +3. **Map dependencies** - What calls what, data flow +4. **Find synchronization** - Mutexes, conditions, atomics +5. **Trace resource lifecycle** - Allocations, ownership, cleanup +6. **Review existing docs** - Check for patterns and style + +### Step 2: Create Structure + +For each component: + +```markdown +# Component Name + +## Overview +Brief 2-3 sentence description of purpose and role. + +## Architecture +High-level design with diagrams. + +## Key Components +List main structures, functions, modules. + +## Threading Model +How threads interact, synchronization primitives. + +## Memory Management +Allocation patterns, ownership, lifecycle. + +## API Reference +Public functions with signatures and examples. + +## Usage Examples +Common use cases with code snippets. + +## Error Handling +Error codes, failure modes, recovery. + +## Performance Considerations +Resource usage, bottlenecks, optimization tips. + +## Platform Notes +Platform-specific behavior or requirements. + +## Testing +How to test, test coverage, known issues. + +## See Also +Cross-references to related documentation. +``` + +### Step 3: Add Diagrams + +Use Mermaid for visual documentation: + +#### Component Diagram +```mermaid +graph TB + A[Client] --> B[Connection Pool] + B --> C[CURL Handle 1] + B --> D[CURL Handle 2] + B --> E[CURL Handle N] + C --> F[libcurl] + D --> F + E --> F + F --> G[HTTP Server] +``` + +#### Sequence Diagram +```mermaid +sequenceDiagram + participant Client + participant Pool + participant CURL + participant Server + + Client->>Pool: Request handle + Pool->>Pool: Lock mutex + Pool-->>Client: Return handle + Client->>CURL: Configure request + Client->>CURL: Execute + CURL->>Server: HTTP Request + Server-->>CURL: Response + CURL-->>Client: Result + Client->>Pool: Release handle + Pool->>Pool: Signal condition +``` + +#### State Diagram +```mermaid +stateDiagram-v2 + [*] --> Uninitialized + Uninitialized --> Initialized: init() + Initialized --> Running: start() + Running --> Paused: pause() + Paused --> Running: resume() + Running --> Stopped: stop() + Stopped --> [*] +``` + +#### Data Flow Diagram +```mermaid +flowchart LR + A[Marker Event] --> B{Event Type} + B -->|Component| C[Component Marker] + B -->|Event| D[Event Marker] + C --> E[Profile Matcher] + D --> E + E --> F[Report Generator] + F --> G[HTTP Sender] +``` + +### Step 4: Add Code Examples + +Provide clear, compilable examples: + +#### Good Example Structure +```markdown +### Example: Creating a Profile + +This example shows how to create and configure a telemetry profile. + +**Prerequisites:** +- Telemetry system initialized +- Valid configuration file + +**Code:** +```c +#include "profile.h" +#include + +int main(void) { + profile_t* profile = NULL; + int ret = 0; + + // Create profile with name and interval + ret = profile_create("MyProfile", 60, &profile); + if (ret != 0) { + fprintf(stderr, "Failed to create profile: %d\n", ret); + return -1; + } + + // Add marker to profile + ret = profile_add_marker(profile, "Component.Status", + MARKER_TYPE_COMPONENT); + if (ret != 0) { + fprintf(stderr, "Failed to add marker: %d\n", ret); + profile_destroy(profile); + return -1; + } + + // Activate profile + ret = profile_activate(profile); + if (ret != 0) { + fprintf(stderr, "Failed to activate profile: %d\n", ret); + profile_destroy(profile); + return -1; + } + + printf("Profile created and activated successfully\n"); + + // Cleanup + profile_destroy(profile); + return 0; +} +``` + +**Expected Output:** +``` +Profile created and activated successfully +``` + +**Notes:** +- Always check return values +- Call profile_destroy() even on error paths +- Profile name must be unique + +### Step 5: Document APIs + +For each public function: + +```markdown +### profile_create() + +Creates a new telemetry profile. + +**Signature:** +```c +int profile_create(const char* name, + unsigned int interval_sec, + profile_t** out_profile); +``` + +**Parameters:** +- `name` - Unique profile name (max 63 chars, non-NULL) +- `interval_sec` - Reporting interval in seconds (min: 60, max: 86400) +- `out_profile` - Output pointer to created profile (must be non-NULL) + +**Returns:** +- `0` - Success +- `-EINVAL` - Invalid parameter (NULL name/out_profile, invalid interval) +- `-ENOMEM` - Memory allocation failed +- `-EEXIST` - Profile with same name already exists + +**Thread Safety:** +Thread-safe. Uses internal mutex for profile list management. + +**Memory:** +Allocates memory for profile structure and name copy. Caller must call +`profile_destroy()` to free resources. + +**Example:** +See [Example: Creating a Profile](#example-creating-a-profile) + +**See Also:** +- profile_destroy() +- profile_activate() +- profile_add_marker() +``` + +### Step 6: Document Threading + +For multi-threaded components: + +```markdown +## Threading Model + +### Thread Overview + +| Thread Name | Purpose | Priority | Stack Size | +|------------|---------|----------|------------| +| Main | Initialization, message loop | Normal | Default | +| XConf Fetch | Configuration retrieval | Low | 64KB | +| Report Send | HTTP report transmission | Low | 64KB | +| Event Receiver | Marker event processing | High | 32KB | + +### Synchronization Primitives + +```c +// Global mutexes +static pthread_mutex_t pool_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_mutex_t profile_mutex = PTHREAD_MUTEX_INITIALIZER; + +// Condition variables +static pthread_cond_t pool_cond = PTHREAD_COND_INITIALIZER; +static pthread_cond_t xconf_cond = PTHREAD_COND_INITIALIZER; +``` + +### Lock Ordering + +To prevent deadlocks, always acquire locks in this order: + +1. `profile_mutex` (profile list) +2. `pool_mutex` (connection pool) +3. Individual profile locks + +**Example:** +```c +// CORRECT: Proper lock ordering +pthread_mutex_lock(&profile_mutex); +profile_t* p = find_profile_locked(name); +pthread_mutex_lock(&pool_mutex); +// ... use both resources ... +pthread_mutex_unlock(&pool_mutex); +pthread_mutex_unlock(&profile_mutex); + +// WRONG: Deadlock risk! +pthread_mutex_lock(&pool_mutex); +pthread_mutex_lock(&profile_mutex); // May deadlock! +``` + +### Thread Safety Guarantees + +| Function | Thread Safety | Notes | +|----------|---------------|-------| +| profile_create() | Thread-safe | Uses profile_mutex | +| profile_destroy() | Thread-safe | Uses profile_mutex | +| profile_add_marker() | Not thread-safe | Call before activation only | +| send_report() | Thread-safe | Uses pool_mutex | +``` + +### Step 7: Document Memory Management + +```markdown +## Memory Management + +### Allocation Patterns + +```mermaid +graph TD + A[profile_create] --> B[malloc profile_t] + B --> C[strdup name] + B --> D[malloc markers array] + E[profile_add_marker] --> F[realloc markers] + G[profile_destroy] --> H[free markers] + H --> I[free name] + I --> J[free profile_t] +``` + +### Ownership Rules + +1. **profile_t**: Owned by caller after profile_create() +2. **Marker strings**: Copied; caller retains original ownership +3. **Report data**: Owned by sender; freed after transmission + +### Lifecycle Example + +```c +// Creation phase +profile_t* prof = NULL; +profile_create("test", 60, &prof); // Allocates memory + +// Configuration phase +profile_add_marker(prof, "mark1", TYPE_EVENT); // May realloc +profile_add_marker(prof, "mark2", TYPE_EVENT); // May realloc + +// Active phase - no allocations +profile_activate(prof); + +// Destruction phase +profile_destroy(prof); // Frees all memory +prof = NULL; // Prevent use-after-free +``` + +### Memory Budget + +Typical memory usage per component: + +| Component | Static | Dynamic (per item) | Notes | +|-----------|--------|-------------------|-------| +| Profile | 128 bytes | +32 bytes/marker | Preallocated list | +| Connection Pool | 512 bytes | +256 bytes/handle | Max 5 handles | +| Report Buffer | 0 | 64KB | Temporary, freed after send | + +**Total typical footprint**: ~150KB (5 profiles, 3 connections, 1 report) +``` + +## Best Practices + +### Writing Style + +1. **Be Concise**: Get to the point quickly +2. **Be Specific**: Use exact terms, not vague descriptions +3. **Be Accurate**: Test all code examples +4. **Be Complete**: Don't leave critical details unstated +5. **Be Consistent**: Follow established patterns + +### Code Examples + +- **Always compile-test** examples before documenting +- **Show error handling** - embedded systems need robust code +- **Include cleanup** - demonstrate proper resource management +- **Add context** - explain when/why to use the code +- **Keep focused** - one example, one concept + +### Diagrams + +- **Use Mermaid** for all diagrams (version control friendly) +- **Keep simple** - max 10-12 nodes per diagram +- **Label clearly** - all arrows and nodes need names +- **Show flow** - make direction obvious +- **Add legends** - explain symbols if needed + +### Cross-References + +Link related documentation: + +```markdown +## See Also + +- [Threading Model](../architecture/threading-model.md) - Overall thread architecture +- [Connection Pool API](connection-pool.md) - Pool management functions +- [Error Codes](../api/error-codes.md) - Complete error code reference +- [Build Guide](../integration/build-setup.md) - Compilation instructions +``` + +### Platform-Specific Notes + +Always document platform variations: + +```markdown +## Platform Notes + +### Linux +- Uses pthread for threading +- Requires libcurl 7.65.0+ +- mTLS via OpenSSL 1.1.1+ + +### RDKB Devices +- Integration with RDK logger (rdk_debug.h) +- Uses RBUS for IPC when available +- Memory constraints: limit to 8 profiles max + +### Constraints +- **Memory**: Tested with 64MB minimum +- **CPU**: ARMv7 or better +- **Storage**: 1MB for logs and cache +``` + +## Output Format + +### Component Documentation Template + +```markdown +# [Component Name] + +## Overview + +[2-3 sentence description] + +## Architecture + +[High-level design explanation] + +### Component Diagram +```mermaid +[Component relationship diagram] +``` + +## Key Components + +### [Structure/Type Name] + +[Description] + +```c +typedef struct { + // Fields with comments +} structure_t; +``` + +## Threading Model + +[Thread safety and synchronization] + +## Memory Management + +[Allocation patterns and ownership] + +## API Reference + +### [function_name()] + +[Full API documentation] + +## Usage Examples + +### Example: [Use Case] + +[Complete working example] + +## Error Handling + +[Error codes and recovery] + +## Performance + +[Resource usage and bottlenecks] + +## Testing + +[Test procedures and coverage] + +## See Also + +[Cross-references] +``` + +## Quality Checklist + +Before considering documentation complete: + +- [ ] All public APIs documented with signatures +- [ ] At least one working code example per major function +- [ ] Thread safety explicitly stated +- [ ] Memory ownership clearly documented +- [ ] Error codes and meanings listed +- [ ] Diagrams for complex flows +- [ ] Cross-references to related docs +- [ ] Platform-specific notes included +- [ ] Code examples compile and run +- [ ] Grammar and spelling checked +- [ ] Reviewed by component author + +## Maintenance + +Documentation is code: + +1. **Update with code changes** - docs and code change together +2. **Version documentation** - tag with releases +3. **Review periodically** - ensure accuracy quarterly +4. **Fix broken links** - validate references +5. **Deprecate carefully** - mark old features clearly + +### Deprecation Notice Template + +```markdown +## DEPRECATED: old_function() + +⚠️ **This function is deprecated as of v2.1.0** + +**Reason**: Memory leak risk in error paths + +**Alternative**: Use new_function() instead + +**Migration Example**: +```c +// Old way (deprecated) +old_function(param); + +// New way +new_function(param); +``` + +**Removal**: Scheduled for v3.0.0 (Est. Q2 2026) +``` + +## Tools Integration + +### Generate API Docs from Code + +Use Doxygen-style comments in code: + +```c +/** + * @brief Create a new telemetry profile + * + * Creates and initializes a profile structure. The caller is responsible + * for destroying the profile with profile_destroy() when done. + * + * @param[in] name Unique profile name (max 63 chars) + * @param[in] interval_sec Reporting interval (60-86400 seconds) + * @param[out] out_profile Pointer to receive created profile + * + * @return 0 on success, negative errno on failure + * @retval 0 Success + * @retval -EINVAL Invalid parameter + * @retval -ENOMEM Memory allocation failed + * @retval -EEXIST Profile already exists + * + * @note Thread-safe + * @see profile_destroy(), profile_activate() + * + * @par Example: + * @code + * profile_t* prof = NULL; + * int ret = profile_create("MyProfile", 300, &prof); + * if (ret == 0) { + * // Use profile... + * profile_destroy(prof); + * } + * @endcode + */ +int profile_create(const char* name, + unsigned int interval_sec, + profile_t** out_profile); +``` + +### Diagram Tools + +- **Mermaid Live Editor**: https://mermaid.live +- **VS Code Markdown Preview**: Built-in mermaid support +- **Documentation generators**: Can embed mermaid in output + +## Troubleshooting Common Documentation Issues + +### Issue: Code example doesn't compile + +**Solution**: Always test examples in isolation +```bash +# Extract example to test file +cat > test_example.c << 'EOF' +[paste example code] +EOF + +# Compile with project flags +gcc -Wall -Wextra -I../include test_example.c -o test_example + +# Run to verify +./test_example +``` + +### Issue: Diagram is too complex + +**Solution**: Break into multiple diagrams +- One high-level overview diagram +- Multiple focused detail diagrams +- Link them together in text + +### Issue: Outdated documentation + +**Solution**: Add CI check +```bash +# Check for TODOs in docs +grep -r "TODO\|FIXME\|XXX" docs/ && exit 1 + +# Check for broken links +markdown-link-check docs/**/*.md +``` + +## Example References + +See documentation references for guidance: +- [CURL Architecture](https://curl.se/docs/architecture.html) - Good example of architecture documentation with diagrams +- [Memory Safety Skill](../memory-safety-analyzer/SKILL.md) - Example skill documentation +- [Build Instructions](../../../.github/instructions/build-system.instructions.md) - Integration guide example diff --git a/.github/skills/thread-safety-analyzer/SKILL.md b/.github/skills/thread-safety-analyzer/SKILL.md new file mode 100644 index 00000000..9d413f01 --- /dev/null +++ b/.github/skills/thread-safety-analyzer/SKILL.md @@ -0,0 +1,436 @@ +--- +name: thread-safety-analyzer +description: Analyze C/C++ code for thread safety issues including race conditions, deadlocks, and improper synchronization. Use when reviewing concurrent code or debugging threading issues. +--- + +# Thread Safety Analysis for Embedded C + +## Purpose + +Systematically analyze C/C++ code for thread safety issues that can cause race conditions, deadlocks, or performance degradation in embedded systems. + +## Usage + +Invoke this skill when: +- Reviewing multi-threaded code +- Debugging race conditions or deadlocks +- Optimizing synchronization overhead +- Validating thread creation and cleanup +- Investigating lock contention issues + +## Analysis Process + +### Step 1: Identify Shared Data + +Search for global and static variables: +- Global variables (especially non-const) +- Static variables in functions +- Shared heap allocations +- Reference-counted objects + +For each shared variable, verify: +1. How is it protected (mutex, atomic, etc.)? +2. Is the protection consistent across all accesses? +3. Are reads and writes both protected? +4. Is initialization thread-safe? + +### Step 2: Review Thread Creation + +Check all pthread_create calls: +- Are thread attributes used? +- Is stack size specified? +- Are threads detached or joinable? +- Is cleanup properly handled? + +```c +// CHECK FOR: +pthread_t thread; +pthread_create(&thread, NULL, func, arg); // BAD: No attributes + +// SHOULD BE: +pthread_attr_t attr; +pthread_attr_init(&attr); +pthread_attr_setstacksize(&attr, 64 * 1024); // Explicit size +pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); +pthread_create(&thread, &attr, func, arg); +pthread_attr_destroy(&attr); +``` + +### Step 3: Analyze Lock Usage + +For each mutex/rwlock: +- Is it initialized before use? +- Is it destroyed when done? +- Are lock/unlock pairs balanced? +- What is the lock ordering? +- Are locks held during expensive operations? + +Common patterns to check: +```c +// Pattern 1: Missing unlock on error path +pthread_mutex_lock(&lock); +if (error) return -1; // LEAK! +pthread_mutex_unlock(&lock); + +// Pattern 2: Lock ordering violation +// Thread 1: +pthread_mutex_lock(&a); +pthread_mutex_lock(&b); + +// Thread 2: +pthread_mutex_lock(&b); // Different order! +pthread_mutex_lock(&a); // DEADLOCK RISK! + +// Pattern 3: Heavy lock for simple operation +pthread_rwlock_wrlock(&lock); // Too heavy +counter++; +pthread_rwlock_unlock(&lock); +// Should use atomic_int instead +``` + +### Step 4: Check for Race Conditions + +Look for unprotected accesses to shared data: + +```c +// RACE: Read-modify-write without protection +if (shared_flag == 0) { // Thread 1 reads + shared_flag = 1; // Thread 2 also reads before Thread 1 writes +} + +// FIX: Use atomic or lock +pthread_mutex_lock(&lock); +if (shared_flag == 0) { + shared_flag = 1; +} +pthread_mutex_unlock(&lock); + +// OR: Use atomic compare-and-swap +int expected = 0; +atomic_compare_exchange_strong(&shared_flag, &expected, 1); +``` + +### Step 5: Verify Atomic Usage + +For atomic variables: +- Are they declared with proper type (atomic_int, atomic_bool)? +- Is memory ordering appropriate? +- Are non-atomic operations mixed with atomic ones? + +```c +// CHECK: +atomic_int counter; + +// GOOD: Atomic operations +atomic_fetch_add(&counter, 1); +int value = atomic_load(&counter); + +// BAD: Mixing atomic and non-atomic +counter++; // Non-atomic! Use atomic_fetch_add +``` + +### Step 6: Deadlock Detection + +Check for common deadlock patterns: + +1. **Circular wait**: Lock A → Lock B, Lock B → Lock A +2. **Lock held while waiting**: Mutex held during sleep/wait +3. **Missing timeout**: Indefinite blocking without timeout +4. **Signal under lock**: Condition signal while holding mutex + +```c +// Deadlock Pattern 1: Circular dependency +// Function 1: +lock(mutex_a); +lock(mutex_b); // Order: A, B + +// Function 2: +lock(mutex_b); +lock(mutex_a); // Order: B, A - DEADLOCK! + +// Deadlock Pattern 2: Lock held during expensive operation +lock(mutex); +expensive_network_call(); // Blocks other threads! +unlock(mutex); + +// Deadlock Pattern 3: No timeout +pthread_mutex_lock(&lock); // Waits forever if deadlock +``` + +### Step 7: Check Condition Variables + +For condition variables: +- Is wait always in a loop? +- Is predicate checked before and after wait? +- Is signal/broadcast done correctly? +- Is spurious wakeup handled? + +```c +// GOOD: Proper condition variable usage +pthread_mutex_lock(&mutex); +while (!condition) { // Loop for spurious wakeups + pthread_cond_wait(&cond, &mutex); +} +// ... use protected data ... +pthread_mutex_unlock(&mutex); + +// Signal: +pthread_mutex_lock(&mutex); +condition = true; +pthread_cond_signal(&cond); +pthread_mutex_unlock(&mutex); + +// BAD: Missing loop +pthread_mutex_lock(&mutex); +if (!condition) { // Should be 'while'! + pthread_cond_wait(&cond, &mutex); +} +pthread_mutex_unlock(&mutex); +``` + +## Common Issues and Fixes + +### Issue: Default Thread Stack Size + +```c +// PROBLEM: Wastes memory (8MB per thread) +pthread_t thread; +pthread_create(&thread, NULL, worker, arg); + +// FIX: Specify minimal stack size +pthread_attr_t attr; +pthread_attr_init(&attr); +pthread_attr_setstacksize(&attr, 64 * 1024); // 64KB +pthread_create(&thread, &attr, worker, arg); +pthread_attr_destroy(&attr); +``` + +### Issue: Heavy Synchronization + +```c +// PROBLEM: Reader-writer lock overkill +pthread_rwlock_t lock; +int counter; + +void increment() { + pthread_rwlock_wrlock(&lock); + counter++; + pthread_rwlock_unlock(&lock); +} + +// FIX: Use atomic operations +atomic_int counter; + +void increment() { + atomic_fetch_add(&counter, 1); // No lock needed +} +``` + +### Issue: Lock Ordering Violation + +```c +// PROBLEM: Different lock orders cause deadlock +// Thread 1: +void process_a_then_b() { + lock(&resource_a.lock); + lock(&resource_b.lock); + // ... +} + +// Thread 2: +void process_b_then_a() { + lock(&resource_b.lock); + lock(&resource_a.lock); // DEADLOCK! + // ... +} + +// FIX: Consistent ordering everywhere +void process_a_then_b() { + lock(&resource_a.lock); // Always A first + lock(&resource_b.lock); // Then B + // ... +} + +void process_b_then_a() { + lock(&resource_a.lock); // Always A first + lock(&resource_b.lock); // Then B + // ... +} +``` + +### Issue: Race in Lazy Initialization + +```c +// PROBLEM: Non-thread-safe initialization +static config_t* config = NULL; + +config_t* get_config() { + if (!config) { // Race here! + config = malloc(sizeof(config_t)); + init_config(config); + } + return config; +} + +// FIX: Use pthread_once +static pthread_once_t init_once = PTHREAD_ONCE_INIT; +static config_t* config = NULL; + +static void init_config_once() { + config = malloc(sizeof(config_t)); + init_config(config); +} + +config_t* get_config() { + pthread_once(&init_once, init_config_once); + return config; +} +``` + +### Issue: Missing Lock on Error Path + +```c +// PROBLEM: Lock not released on error +int process_data(data_t* shared) { + pthread_mutex_lock(&shared->lock); + + if (validate(shared) != 0) { + return -1; // BUG: Lock not released! + } + + update(shared); + pthread_mutex_unlock(&shared->lock); + return 0; +} + +// FIX: Unlock on all paths +int process_data(data_t* shared) { + int ret = 0; + + pthread_mutex_lock(&shared->lock); + + if (validate(shared) != 0) { + ret = -1; + goto cleanup; + } + + update(shared); + +cleanup: + pthread_mutex_unlock(&shared->lock); + return ret; +} +``` + +### Issue: Long Critical Section + +```c +// PROBLEM: Expensive operation under lock +pthread_mutex_lock(&lock); +for (int i = 0; i < 1000000; i++) { + compute(); // Blocks other threads! +} +shared_result = final_value; +pthread_mutex_unlock(&lock); + +// FIX: Minimize critical section +int result = 0; +for (int i = 0; i < 1000000; i++) { + result += compute(); // No lock +} + +pthread_mutex_lock(&lock); +shared_result = result; // Lock only for update +pthread_mutex_unlock(&lock); +``` + +## Testing for Thread Safety + +### Compile with Thread Sanitizer + +```bash +# Build with thread sanitizer +gcc -g -fsanitize=thread -O1 source.c -o program -lpthread + +# Run +./program + +# Will report: +# - Data races +# - Lock ordering issues +# - Potential deadlocks +``` + +### Run Helgrind + +```bash +# Check for thread safety issues +valgrind --tool=helgrind \ + --track-lockorders=yes \ + ./program + +# Reports: +# - Race conditions +# - Lock order violations +# - Possible deadlocks +``` + +### Stress Testing + +```c +// Test under high concurrency +#define NUM_THREADS 100 +#define ITERATIONS 10000 + +void stress_test() { + pthread_t threads[NUM_THREADS]; + + for (int i = 0; i < NUM_THREADS; i++) { + pthread_create(&threads[i], NULL, worker, NULL); + } + + for (int i = 0; i < NUM_THREADS; i++) { + pthread_join(threads[i], NULL); + } + + // Verify invariants + assert(shared_counter == NUM_THREADS * ITERATIONS); +} +``` + +## Output Format + +Provide findings as: + +``` +## Thread Safety Analysis + +### Critical Issues (must fix) +1. [file.c:123] Race condition - unprotected access to shared_flag +2. [file.c:456] Deadlock potential - lock order violation (A→B vs B→A) +3. [file.c:789] Lock leak - mutex not released on error path + +### Warnings (should fix) +1. [file.c:234] Default thread stack - wastes 8MB per thread +2. [file.c:567] Heavy lock - use atomic_int instead of mutex +3. [file.c:890] Long critical section - holds lock during I/O + +### Recommendations +1. Establish lock ordering convention (document in header) +2. Use pthread_once for singleton initialization +3. Replace reader-writer locks with atomics for counters +4. Add thread sanitizer to CI pipeline + +### Suggested Fixes +[Provide specific code changes for each issue] +``` + +## Verification + +After fixes: +1. Thread sanitizer shows no errors +2. Helgrind reports clean +3. Stress tests pass consistently +4. Lock contention metrics acceptable +5. No deadlocks under load testing +6. Code review confirms thread safety diff --git a/.github/skills/triage-logs/SKILL.md b/.github/skills/triage-logs/SKILL.md new file mode 100644 index 00000000..affb7117 --- /dev/null +++ b/.github/skills/triage-logs/SKILL.md @@ -0,0 +1,342 @@ +--- +name: triage-logs +description: > + Triage remote debugger behavioral issues on RDK devices by correlating + device log bundles with the remote debugger source tree. Covers daemon + startup failures, profile processing issues, uploadRRDLogs failures, + RBUS communication problems, configuration errors, and archive or cleanup + failures. The user states the issue; this skill guides systematic + root-cause analysis regardless of issue type. +--- + +# Remote Debugger Log Triage Skill + +## Purpose + +Systematically correlate device log bundles with the remote debugger source +code to identify likely root causes, characterize impact, and propose unit +and functional test reproductions for any behavioral anomaly reported by the +user. + +--- + +## Usage + +Invoke this skill when: +- A device log bundle is available under `logs/` or is attached separately +- The user reports a remote debugger problem such as startup failure, missing + report generation, dynamic or static profile issues, upload failure, RBUS + event issues, or archive cleanup problems +- You need to design a reproduction scenario or propose validation coverage + +The user's stated issue drives the investigation. Do not assume a fixed failure +mode. + +--- + +## Step 1: Orient to the Log Bundle + +Typical files to inspect first: + +```text +logs///logs/ + remotedebugger.log.0 <- Primary remote debugger daemon log + messages.txt.0 <- System messages and crashes + top_log.txt.0 <- CPU and memory snapshots + remote_debugger.json <- Static profile configuration if captured + /tmp/rrd/ <- Generated report and working files + /opt/logs/ <- Source logs referenced by uploads +``` + +Additional files may exist depending on platform integration. If the issue +involves shell orchestration, also look for output associated with +`uploadRRDLogs.sh`. + +Note on legacy naming: the remote debugger code still consumes some legacy +configuration artifacts such as `/tmp/DCMSettings.conf` through +`rrd_config_parse_dcm_settings()`. Treat those as compatibility inputs for the +remote debugger rather than evidence of a separate component. + +--- + +## Step 2: Map Startup and Major Components + +Read the startup portion of `remotedebugger.log.0` and identify: + +| What to find | Log pattern | +|---|---| +| Daemon start | `starting remote-debugger` or `RFC is enabled` | +| Daemon stop | `stopping remote-debugger` or `RFC is disabled` | +| RBUS initialization | `rbus_open`, `Subscribe`, `rrdRbusHandle` | +| Profile load | `remote_debugger.json`, static profile parsing | +| Dynamic event handling | issue type, append, dynamic profile logs | +| Upload orchestration | `uploadRRDLogs`, archive generation, HTTP upload | + +Key remote debugger components in this repo: +- Main daemon lifecycle: `src/rrdMain.c` +- RBUS integration and subscriptions: `src/rrdInterface.c`, `src/rrdRbus.h` +- Configuration loading: `src/rrd_config.c` +- Static and dynamic profile handling: `src/rrdJsonParser.c`, `src/rrdDynamic.c`, `src/rrdEventProcess.c` +- Script execution and orchestration: `src/rrdExecuteScript.c`, `src/uploadRRDLogs.c` +- Archive and upload pipeline: `src/rrd_archive.c`, `src/rrd_upload.c` +- Command execution safety: `src/rrdRunCmdThread.c`, `src/rrdCommandSanity.c` + +--- + +## Step 3: Identify the Anomaly Window + +Based on the user's stated issue, search for the relevant evidence pattern. + +### Remote Debugger Daemon Not Starting or Crashing + +```bash +grep -n "remote-debugger\|RFC is enabled\|RFC is disabled\|ERROR\|FATAL\|Segmentation\|core" remotedebugger.log.0 +grep -n "remotedebugger\|crash\|oom\|killed" messages.txt.0 | tail -50 +``` + +Check for: +- RBUS open or subscribe failures +- Invalid or missing static profile JSON +- RFC disabled path unexpectedly stopping the daemon +- Dependency or library failures surfaced in system logs + +### Static or Dynamic Profile Processing Failures + +```bash +grep -n "profile\|dynamic\|static\|issue\|append\|category\|subcategory\|command" remotedebugger.log.0 +``` + +Look for: +- Missing static profile entries in `remote_debugger.json` +- Dynamic profile append failures +- Rejected or harmful commands +- Missing issue type or category mapping + +### Upload and Archive Failures + +```bash +grep -n "uploadRRDLogs\|archive\|tar\|gzip\|HTTP\|curl\|Failed" remotedebugger.log.0 +grep -n "lock\|cleanup\|retry\|upload" remotedebugger.log.0 +``` + +Look for: +- Archive creation failures +- Lock contention in `rrd_upload_check_lock()` or `rrd_upload_wait_for_lock()` +- Upload endpoint, protocol, or HTTP link misconfiguration +- Source directory cleanup failures after upload + +### Configuration Failures + +```bash +grep -n "config\|properties\|RFC\|DCMSettings\|parse" remotedebugger.log.0 +``` + +Verify: +- `/etc/include.properties` and `/etc/device.properties` availability +- RFC lookup results from `rrd_config_query_rfc()` +- Compatibility parsing from `/tmp/DCMSettings.conf` +- Presence of required fields such as log server, protocol, and upload link + +### RBUS Communication Failures + +```bash +grep -n "rbus\|subscribe\|unsubscribe\|RRD_\|REMOTE_DEBUGGER_RBUS_HANDLE_NAME" remotedebugger.log.0 +``` + +Verify: +- `rtrouted` availability +- Event subscription success +- Property get or set failures +- Force-sync and issue-event handling paths + +### Command Execution and Script Issues + +```bash +grep -n "systemd-run\|uploadRRDLogs.sh\|script\|command\|sanity" remotedebugger.log.0 +``` + +Check for: +- Script execution path failures in `rrdExecuteScript.c` +- Background command handling issues +- Command sanitization rejection + +--- + +## Step 4: Correlate with Source Code + +Map the observed evidence to source files: + +| Issue Area | Source Files | +|---|---| +| Daemon initialization and RFC gating | `src/rrdMain.c` | +| RBUS interface and subscriptions | `src/rrdInterface.c`, `src/rrdRbus.h` | +| Configuration loading | `src/rrd_config.c`, `src/rrd_config.h` | +| Static profile parsing | `src/rrdJsonParser.c` | +| Dynamic profile processing | `src/rrdDynamic.c`, `src/rrdEventProcess.c` | +| Command execution | `src/rrdRunCmdThread.c`, `src/rrdExecuteScript.c`, `src/rrdCommandSanity.c` | +| Upload orchestration | `src/uploadRRDLogs.c`, `src/rrd_upload.c` | +| Archive creation | `src/rrd_archive.c` | +| System information collection | `src/rrd_sysinfo.c` | +| Log processing | `src/rrd_logproc.c` | + +Example correlation: + +If logs show repeated upload lock waits or cleanup errors: +1. Check `src/rrd_upload.c` for lock handling and cleanup sequencing +2. Check `src/uploadRRDLogs.c` for configuration and archive naming +3. Verify source directory lifecycle around `rrd_upload_cleanup_source_dir()` + +--- + +## Step 5: Reproduce Locally + +### Configuration Reproduction + +Use the existing unit test binary when possible: + +```bash +sh run_ut.sh +``` + +For targeted config issues, focus on existing tests around `rrd_config_load()` +and upload orchestration in `src/unittest/rrdUnitTestRunner.cpp`, then inspect +`rrd_config_parse_dcm_settings()` directly in `src/rrd_config.c` because there +is not currently a dedicated unit test covering that parser. + +### L2 Reproduction + +Use the repo's functional test harness: + +```bash +sh cov_build.sh +sh run_l2.sh +``` + +Relevant L2 areas include: +- dynamic profile reports +- static profile reports +- harmful command rejection +- start control and single-instance behavior +- debug report upload and C API upload + +### RBUS Reproduction + +```bash +systemctl status rtrouted +rbuscli get Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RemoteDebugger.Enable +``` + +Use the actual property names observed in logs or in `src/rrdRbus.h` when a +platform variant differs. + +--- + +## Step 6: Test Gap Analysis + +Identify code paths that may lack regression coverage. + +### Unit Test Coverage + +Inspect `src/unittest/rrdUnitTestRunner.cpp` for: +- error-path coverage in `rrd_config_load()` +- upload orchestration failures +- archive cleanup behavior +- RBUS failure handling +- harmful command rejection + +### L2 Test Coverage + +Inspect `test/functional-tests/features/` and `test/functional-tests/tests/` for: +- missing scenarios matching the reported bug +- race or timing-sensitive startup cases +- malformed static profile inputs +- upload failure and retry edge cases + +--- + +## Step 7: Propose Fix and Test + +### Fix Template + +```c +// BEFORE: Continue after upload failure +int ret = rrd_upload_execute(server, protocol, link, workdir, archive, source_dir); +// Continued processing here + +// AFTER: Stop and surface the failure +int ret = rrd_upload_execute(server, protocol, link, workdir, archive, source_dir); +if (ret != 0) { + RDK_LOG(RDK_LOG_ERROR, LOG_REMDEBUG, "Upload failed: %d\n", ret); + return ret; +} +``` + +### Test Template + +```cpp +TEST(RemoteDebuggerUploadTest, RejectsUploadFailureAndStopsCleanupSequence) +{ + // Configure mocks to force upload failure, then verify return path. + int ret = rrd_upload_orchestrate("/tmp/rrd-test", "issue_type"); + EXPECT_NE(ret, 0); +} +``` + +--- + +## Output Format + +Present findings in this structure: + +```markdown +## Triage Summary + +**Issue:** +**Evidence:** +**Root Cause:** +**Impact:** + +## Code Location + +**File:** +**Function:** +**Line:** + +## Reproduction + +[bash or test scenario to reproduce] + +## Proposed Fix + +[code diff or description] + +## Test Coverage + +**Existing:** [what tests exist] +**Missing:** [tests needed to prevent regression] + +## Next Steps + +1. [immediate action] +2. [follow-up verification] +``` + +--- + +## Example Triage Flow + +**User:** "remote debugger generates the archive but upload never completes" + +**Step 1:** Located `remotedebugger.log.0`, found repeated lock wait and upload failures. + +**Step 2:** Checked `src/rrd_upload.c` and confirmed the failure path depends on +lock state plus upload return code. + +**Step 3:** Verified configuration inputs in `rrd_config_load()` and confirmed +the upload server or link was missing from the effective configuration. + +**Root Cause:** Effective upload configuration was incomplete, causing the +upload path to fail after archive preparation. + +**Fix Direction:** Add stronger validation before calling +`rrd_upload_execute()` and extend unit coverage for incomplete configuration. diff --git a/README.md b/README.md index 66b492ef..34998f5c 100644 --- a/README.md +++ b/README.md @@ -1,126 +1,506 @@ -# **RDK Remote Debugger (RRD)** -## **Field Diagnostic/Debug Data Collection Tools for X-Class devices** +# RDK Remote Debugger + +Remote Debugger is an embedded diagnostic collection module for RDK platforms. It allows the device to generate issue-specific debug reports without interactive shell access, using RFC and event-driven triggers, static or dynamic profiles, and an upload pipeline that hands off archives to the platform log-upload infrastructure. + +The current implementation is a C-based daemon and support library built around the `remotedebugger` binary, optional IARMBus integration, RBUS event subscriptions, a message-queue-driven execution loop, JSON profile parsing, command safety filtering, local report generation, and archive upload orchestration. + +## Table of Contents + +- [Overview](#overview) +- [Current Behavior](#current-behavior) +- [Architecture](#architecture) + - [Daemon Startup Flow](#daemon-startup-flow) +- [Module Layout](#module-layout) +- [Configuration Sources](#configuration-sources) +- [Trigger and Processing Flow](#trigger-and-processing-flow) + - [RBUS Subscription Sequence](#rbus-subscription-sequence) + - [Event Dispatch Sequence](#event-dispatch-sequence) +- [Profiles](#profiles) + - [Profile Resolution Flow](#profile-resolution-flow) +- [Upload Flow](#upload-flow) + - [Upload Pipeline Sequence](#upload-pipeline-sequence) +- [Build and Test](#build-and-test) +- [Runtime Artifacts](#runtime-artifacts) +- [RFC and Event Interfaces](#rfc-and-event-interfaces) +- [Versioning](#versioning) +- [Changelog](#changelog) -## Table of Contents -[Requirement](#requirement)
-[Approach](#approach)
-[Prerequisites](#prerequisites)
-[Usecase](#usecase)
-[Implementation](#implementation)
-[Architecture Overview](#architecture-overview)
-[Sequence Diagram](#sequence-diagram)
-[RFC Parameter](#rfc-parameters)
-[Versioning](#versioning)
-[CHANGELOG](#changelog)
+## Overview + +The module is intended for field triage and remote diagnostics. A controller enables the feature, sets an issue type, and the device collects matching diagnostics into a report archive for upload. -### **Requirement** +Compared with the older script-heavy design, the current codebase has migrated key responsibilities into C modules: -As a Field Triage Engineer and Developer responsible for debugging field issues, having the ability to collect diagnostic/ debug data from the device without requiring SSH access would be beneficial. The Triage Engineer can use WebPA communication to initiate the data reporting process and retrieve the report from the S3 log service. +- Daemon lifecycle and event loop in `src/rrdMain.c` +- RBUS and optional IARM subscription logic in `src/rrdInterface.c` +- Static and dynamic profile handling in `src/rrdJsonParser.c`, `src/rrdDynamic.c`, and `src/rrdEventProcess.c` +- Upload orchestration in `src/uploadRRDLogs.c` +- Upload execution and lock coordination in `src/rrd_upload.c` +- Configuration loading and compatibility fallbacks in `src/rrd_config.c` -- A Trigger based Data Reporting - - A WebPA based trigger - - Ability to generate a profile-based report - - Profile comprises of single/ multiple commands +## Current Behavior -As a Product Owner, the goal is for the device to process cloud requests internally upon receiving an external trigger and generate a report based on the issue type. The final report will be uploaded to the same log server for analysis and debugging. An additional RDK module, the RDK Remote Debugger, has been added to handle cloud request processing and report generation. +At runtime, the daemon follows this high-level flow: -The Debugger process will run with a static profile. This profile will be created based on triage inputs, with issue types and the corresponding debug commands. +1. Initialize logging, device information, and internal cache. +2. Check whether Remote Debugger is enabled through RFC or syscfg-backed state. +3. Create the internal message queue. +4. Subscribe to event sources through RBUS and, when enabled, IARMBus. +5. Wait for issue, webconfig, or deep-sleep-related events. +6. Resolve the issue type against the static profile or dynamic profile path. +7. Execute allowed commands and store output under the report working directory. +8. Archive the collected output. +9. Upload the archive through the existing platform log-upload implementation. +10. Clean up generated artifacts after success or failure paths. -The following steps outline the process to generate the report: +The daemon exits early when the feature is disabled. -1. **Initiate WebPA:** Initiate the WebPA Trigger to start Report generation from Cloud Server endpoint. +## Architecture -2. **Enable Remote Debugger:** Enable Remote Debugger on the device by setting below tr181 data model +### Control Plane - `DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.Enable` +The control plane is event-driven: -3. **Set Issue Type:** The WebPA command will set the issue types for report generation. Using the new tr181 data model. +- RFC or syscfg state gates whether the daemon should run. +- RBUS subscriptions receive Remote Debugger issue events and webconfig updates. +- Optional IARMBus support extends integration for platforms that use those interfaces. +- Events are converted into internal messages and processed on the daemon thread through a System V message queue. - `DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType` +### Data Plane -4. **RRD Daemon Startup:** The RRD daemon will start on Boot up and will subscribe for WebPA event for handling the Cloud requests. +The data plane turns an issue trigger into an uploaded archive: -5. **Event Handling:** Upon receiving an event trigger, the handler will look up the issue type in static profile and will collect the corresponding debug commands for execution. +- Static profile data comes from `remote_debugger.json`. +- Dynamic profile processing is handled through the dynamic profile and download-manager integration paths. +- Commands are sanitized before execution. +- Command output is collected into a working directory. +- The working directory is archived under `/tmp/rrd/`. +- The archive is uploaded using the `uploadstblogs_run()` API path when IARMBus support is enabled, or by the shell fallback path otherwise. -6. **Dynamic Profile Handling**: If the specific issue type is not found in the Static profile, the RRD will trigger a download for a Dynamic profile via the RDK Download Manager. The following outcomes are possible: - - If the download succeeds and the profile contains the issue type, the corresponding debug command will be executed. - - If the Dynamic profile is not available or if the profile is available but the download fails, it will be logged. +### Visual References -### **Approach:** +Existing design diagrams are still available: -The RDK Remote Debugger is an application that collects debug and diagnostic information from RDK platforms without the need for SSH access to the device. It operates as a daemon with an IARM Event handler, which listens for events from the TR181 RFC parameters via IARM Bus communication. These events are received from the WebPA and processed by the Remote Debugger Main Thread, using a message queue to manage different types of messages. Based on the issue type, the corresponding debug commands are extracted from a JSON file and executed through system calls. The resulting output is generated as a report, which is then uploaded to an S3 server. +- `images/rrd_usecase.png` +- `images/rrd_architecture.png` +- `images/rrd_daemon_logic.png` +- `images/rrd_sequence.png` +- `images/rrd_sequence_flow.png` -### **Prerequisites:** +### Daemon Startup Flow -The RDK Remote Debugger Daemon relies on the RFC and WebPA mechanisms to communicate with the requester for data collection. +```mermaid +flowchart TD + A([remotedebugger start]) --> B[rdk_logger_init] + B --> C[RRDStoreDeviceInfo\ninitCache] + C --> D{isRRDEnabled?} + D -->|IARM path:\ngetRFCParameter| E{RFC value\n== false?} + D -->|Non-IARM path:\nsyscfg_get| F{RemoteDebuggerEnabled\n== true?} + E -->|Yes| X([Exit 0 — disabled]) + E -->|No| G[msgget\ncreate System V message queue] + F -->|No| X + F -->|Yes| G + G --> H{Queue created?} + H -->|No| Y([Exit 1 — queue failed]) + H -->|Yes| I[RRD_subscribe\nRBUS open + event subscriptions] + I --> J[pthread_create\nRRDEventThreadFunc] + J --> K[pthread_join\nblock until event loop exits] + K --> L[RRD_unsubscribe\nrbus_close] + L --> M([Return 0]) +``` -- The RFC parameter is used to send events through WebPA. -- The Remote Debugger runs with a default profile. -- The profile includes issue categories and a set of corresponding commands. -- Based on the value of the RFC parameter, the relevant commands are read and executed. -- If the issue type is not found in the default profile, the system will attempt to download a dynamic profile via the Download Manager. +## Module Layout -#### **RRD Features with Static Profile** +### Main executable -- Collect diagnostic and debug data from field devices without the need for SSH access. -- Use existing communication channels, such as WebPA, to trigger report generation based on the static profile. -- Start the Debugger with the default profile, which is packaged during build time. +- `src/rrdMain.c`: daemon startup, RFC gate check, message queue creation, event thread lifecycle -#### **RRD Features with Dynamic Profile** +### Event and interface layer -- Collect diagnostic and debug data from field devices without requiring SSH access. -- Trigger report generation via existing communication channels, such as WebPA, based on the dynamic profile. -- If the issue type is not found in the static profile, automatically download the dynamic profile via the Download Manager. -- Execute commands from the dynamic profile if available, with the ability to handle failures in downloading or missing issue types. +- `src/rrdInterface.c`: RBUS open, event subscription, webconfig integration, message delivery +- `src/rrdIarmEvents.c`: optional IARMBus event support +- `src/rrdRbus.h`: RBUS-facing definitions and subscription state -### **Usecase** -![USECASE](./images/rrd_usecase.png) +### Profile and event processing -### **Implementation:** +- `src/rrdJsonParser.c`: static profile parsing +- `src/rrdDynamic.c`: dynamic profile and download-manager related handling +- `src/rrdEventProcess.c`: issue event processing +- `src/rrdMsgPackDecoder.c`: webconfig payload decoding support -The Remote Debugger reads the IARM event and the value from the TR69 parameter, passing the message to the thread via a message queue. The daemon receives the message from IARM and reads the [remote_debugger.json](./remote_debugger.json) file to capture the commands associated with the issue type. System calls are used to execute these commands. +### Command execution and safety -- WebPA is used from the remote side: - - Triggers the start and initiation of command execution via RRD. - - Initiates the RDM process for dynamic profile updates. -- RRD processes the request based on the issue type: - - Uses the TR181 data model for each issue type/category. - - The handler looks up the profile database for the corresponding debug commands. - - Generates the final report. - - Uploads the final report tar file to the S3 log server. +- `src/rrdRunCmdThread.c`: command execution flow +- `src/rrdCommandSanity.c`: command allow and block checks +- `src/rrdExecuteScript.c`: upload invocation path and script fallback handling -### **Architecture Overview** +### Upload and report generation -![Architecture Diagram](./images/rrd_architecture.png) +- `src/uploadRRDLogs.c`: top-level archive and upload orchestration +- `src/rrd_upload.c`: upload execution, lock waiting, cleanup behavior +- `src/rrd_archive.c`: archive creation and cleanup +- `src/rrd_logproc.c`: log validation, normalization, and preprocessing +- `src/rrd_sysinfo.c`: MAC address and timestamp generation -![Daemon Logic](./images/rrd_daemon_logic.png) +### Configuration -### **Sequence Diagram** +- `src/rrd_config.c`: property parsing, RFC query, legacy compatibility parsing, fallback loading +- `remote_debugger.json`: packaged static profile example -![RRD Sequence Diagram](./images/rrd_sequence.png) +### Unit and functional tests -![RRD Flow](./images/rrd_sequence_flow.png) +- `src/unittest/rrdUnitTestRunner.cpp`: unit coverage for core modules +- `test/functional-tests/`: L2 test features and Python test cases +- `run_ut.sh`: unit-test entry point +- `run_l2.sh`: L2 functional test entry point -### **RFC Parameters:** +## Configuration Sources -Implemented the following RFC parameters as part of the Remote Debugger feature. These parameters enable or disable the RRD feature and trigger events for the required issue types. All relevant information is updated in the tr69hostif source code (datamodel.xml) and the Device_DeviceInfo source code to create the set functions that broadcast the event using IARM calls. +The current implementation reads configuration from multiple places and uses fallback logic in `src/rrd_config.c`. -- `DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.Enable` - This parameter is a Boolean value (True/False) used to control the activation or deactivation of the RRD feature. -- `DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType` - This parameter takes a string value that sets the data through RFC to trigger the event for the Daemon, based on the specified issue type. +Primary sources: -### Versioning -Given a version number MAJOR.MINOR.PATCH, increment the: -- **MAJOR:** version when you make incompatible API changes that break backwards compatibility. This could be removing existing APIs, changes to API Signature or major changes to API behavior that breaks API contract, -- **MINOR:** version when you add backward compatible new features like adding new APIs, adding new parameters to existing APIs, -- **PATCH:** version when you make backwards compatible bug fixes. +- `/etc/include.properties` +- `/etc/device.properties` +- RFC values queried through `tr181` -### CHANGELOG -The CHANGELOG file that contains all changes done so far. When version is updated, add a entry in the CHANGELOG.md at the top with user friendly information on what was changed with the new version. Refer to [Changelog](https://github.com/olivierlacan/keep-a-changelog/blob/main/CHANGELOG.md) as an example and [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) for more details. +Compatibility and fallback sources: -Please Add entry in the CHANGELOG for each version change and indicate the type of change with these labels: -- **Added** for new features. -- **Changed** for changes in existing functionality. -- **Deprecated** for soon-to-be removed features. -- **Removed** for now removed features. -- **Fixed** for any bug fixes. -- **Security** in case of vulnerabilities. +- `/tmp/DCMSettings.conf` +- `/opt/dcm.properties` +- `/etc/dcm.properties` + +Important notes: + +- The legacy DCM-named files are still compatibility inputs for the Remote Debugger upload configuration path. +- RFC values are preferred when available. +- If required upload values are still empty, the code falls back to `dcm.properties` locations. +- The default upload protocol is initialized to `HTTP`. + +## Trigger and Processing Flow + +### Enablement + +The daemon checks whether Remote Debugger is enabled before entering the event loop. + +Depending on build configuration and platform support, this comes from: + +- RFC lookup via `getRFCParameter()` when IARMBus support is enabled +- `syscfg_get("RemoteDebuggerEnabled", ...)` when running in the non-IARM path + +### Event subscriptions + +The module subscribes to these event classes in the interface layer: + +- Issue-type events for report generation +- Webconfig-related events +- Download-manager events for dynamic profile handling + +### Internal processing + +Once an event arrives: + +1. The event handler converts it into an internal `data_buf` message. +2. The daemon thread receives it from the message queue. +3. The message type determines whether the request is a standard issue event, webconfig event, or deep-sleep event. +4. The processing layer resolves the issue type and executes the appropriate collection flow. + +### RBUS Subscription Sequence + +```mermaid +sequenceDiagram + participant Main as rrdMain.c + participant Iface as rrdInterface.c + participant IARM as IARMBus + participant RBUS as RBUS daemon + participant WCfg as webconfigFramework + + Main->>Iface: RRD_subscribe() + opt IARMBUS_SUPPORT enabled + Iface->>IARM: RRD_IARM_subscribe() + IARM-->>Iface: 0 (success) + end + Iface->>RBUS: rbus_open("rdkRrdRbus") + RBUS-->>Iface: rrdRbusHandle + Iface->>RBUS: rbusEvent_SubscribeEx(RRD_SET_ISSUE_EVENT) + Iface->>RBUS: rbusEvent_SubscribeEx(RRD_WEBCFG_ISSUE_EVENT) + Iface->>RBUS: rbusEvent_SubscribeEx(RDM_DOWNLOAD_EVENT) + Iface->>WCfg: webconfigFrameworkInit() + Iface-->>Main: 0 (success) +``` + +### Event Dispatch Sequence + +```mermaid +sequenceDiagram + participant RBUS as RBUS / IARMBus + participant Handler as Event Handler
(rrdInterface.c) + participant Queue as System V Message Queue + participant Thread as RRDEventThreadFunc
(rrdMain.c) + participant Proc as rrdEventProcess.c + + RBUS->>Handler: _remoteDebuggerEventHandler(event) + Handler->>Handler: Allocate data_buf
set mtype + mdata + Handler->>Queue: msgsnd(msqid, data_buf) + Note over Thread: Blocked on msgrcv() + Queue-->>Thread: msgHdr (data_buf*) + Thread->>Thread: switch(rbuf->mtype) + alt EVENT_MSG + Thread->>Proc: processIssueTypeEvent(rbuf) + else EVENT_WEBCFG_MSG + Thread->>Proc: processWebCfgTypeEvent(rbuf) + else DEEPSLEEP_EVENT_MSG + Thread->>Proc: RRDProcessDeepSleepAwakeEvents(rbuf) + end +``` + +## Profiles + +The Remote Debugger supports both static and dynamic profiles. + +### Static profile + +The static profile is packaged as `remote_debugger.json` and contains hierarchical issue categories, commands, and timeouts. + +Examples in the current sample profile include: + +- Device information and uptime collection +- Process and service status collection +- Deep-sleep-specific audio, video, and process diagnostics +- Sanity rules for dangerous commands + +### Dynamic profile + +If an issue type is not found in the static profile, the daemon can follow the dynamic profile path. This is tied to the remote download and event framework used by the platform. + +### Command safety + +Command execution is gated by sanity checks. The shipped sample profile includes blocked patterns such as: + +- `rm -rf` +- `kill` +- `pkill` +- `iptables` +- `ip6tables` + +### Profile Resolution Flow + +```mermaid +flowchart TD + A([processIssueTypeEvent]) --> B[issueTypeSplitter\nsplit on comma] + B --> C[For each issue type:\nallocate data_buf] + C --> D[processIssueType\ngetIssueInfo: extract Node & subNode] + D --> E{appendMode?} + + E -->|Yes: merge dynamic + static| G[processIssueTypeInDynamicProfileappend\nread dynamic profile JSON] + G --> H{Dynamic profile\nparsed?} + H -->|No| I[RRDRdmManagerDownloadRequest\ntrigger RDM package download] + H -->|Yes| J[processIssueTypeInStaticProfileappend\nread static profile JSON] + J --> K{Static profile\nfound?} + K -->|No| L([skip — log error]) + K -->|Yes| M[Merge commands\ncheckIssueNodeInfo] + M --> Z[executeRRDCmds\nuploadDebugoutput] + + E -->|No: normal path| N{rbuf->inDynamic?} + N -->|Yes| O[processIssueTypeInDynamicProfile\nread JSON at jsonPath] + O --> P{Issue found\nin dynamic JSON?} + P -->|No| Q([discard — log error]) + P -->|Yes| R[checkIssueNodeInfo\nsanity check + build command list] + N -->|No| S[processIssueTypeInStaticProfile\nread /etc/rrd/remote_debugger.json] + S --> T{Issue found\nin static JSON?} + T -->|No| U[processIssueTypeInInstalledPackage\ncheck RDM-installed package JSON] + U --> V{Found?} + V -->|No| W([not found — log error]) + V -->|Yes| R + T -->|Yes| R + R --> Z +``` + +## Upload Flow + +The code path for upload is now explicit and modular. + +1. `rrd_upload_orchestrate()` validates inputs. +2. `rrd_config_load()` resolves upload configuration. +3. `rrd_sysinfo_get_mac_address()` and `rrd_sysinfo_get_timestamp()` build archive naming inputs. +4. `rrd_logproc_*()` validates and prepares the report contents. +5. `rrd_archive_generate_filename()` generates the archive name. +6. `rrd_archive_create()` writes the archive under `/tmp/rrd/`. +7. `rrd_upload_execute()` checks for upload lock conflicts and invokes the platform upload implementation. +8. Cleanup runs for both the archive and source directory. + +Important implementation details: + +- Upload lock coordination uses `/tmp/.log-upload.lock`. +- The upload implementation calls `uploadstblogs_run()` with `rrd_flag = true` in the IARM-enabled path. +- `LOGUPLOAD_ENABLE` receives special live-log handling before archiving. +- When IARMBus support is not enabled, `rrdExecuteScript.c` falls back to `/lib/rdk/uploadRRDLogs.sh`. + +### Upload Pipeline Sequence + +```mermaid +sequenceDiagram + participant Script as rrdExecuteScript.c + participant Orch as uploadRRDLogs.c + participant Cfg as rrd_config.c + participant Sys as rrd_sysinfo.c + participant Log as rrd_logproc.c + participant Arc as rrd_archive.c + participant Up as rrd_upload.c + participant STB as uploadstblogs_run() + + Script->>Orch: rrd_upload_orchestrate(upload_dir, issue_type) + Orch->>Cfg: rrd_config_load(&config) + Note over Cfg: /etc/include.properties
/etc/device.properties
RFC via tr181
/tmp/DCMSettings.conf fallback + Cfg-->>Orch: log_server, protocol, http_link + Orch->>Sys: rrd_sysinfo_get_mac_address() + Orch->>Sys: rrd_sysinfo_get_timestamp() + Sys-->>Orch: mac_addr, timestamp + Orch->>Log: rrd_logproc_validate_source(upload_dir) + Orch->>Log: rrd_logproc_prepare_logs(upload_dir, issue_type) + Orch->>Log: rrd_logproc_convert_issue_type(issue_type, sanitized) + opt issue_type == LOGUPLOAD_ENABLE + Orch->>Log: rrd_logproc_handle_live_logs(upload_dir) + end + Orch->>Arc: rrd_archive_generate_filename(mac, issue, ts) + Arc-->>Orch: archive_filename + Orch->>Arc: rrd_archive_create(upload_dir, /tmp/rrd/, archive_filename) + Arc-->>Orch: 0 (success) + Orch->>Up: rrd_upload_execute(server, protocol, http_link,\n/tmp/rrd/, archive_filename, upload_dir) + Up->>Up: rrd_upload_check_lock()
wait on /tmp/.log-upload.lock + Up->>STB: uploadstblogs_run(params, rrd_flag=true) + STB-->>Up: 0 (success) + Up-->>Orch: 0 (success) + Orch->>Arc: rrd_archive_cleanup(archive_fullpath) + Orch->>Up: rrd_upload_cleanup_source_dir(upload_dir) +``` + +## Build and Test + +### Build output + +The top-level autotools build generates: + +- Binary: `remotedebugger` +- Installed public headers under `$(includedir)/rrd` + +### Main source set + +The base build includes: + +- `rrdMain.c` +- `rrdEventProcess.c` +- `rrdJsonParser.c` +- `rrdRunCmdThread.c` +- `rrdCommandSanity.c` +- `rrdDynamic.c` +- `rrdExecuteScript.c` +- `rrdMsgPackDecoder.c` +- `rrdInterface.c` + +When `--enable-iarmbusSupport=yes` is enabled, the build also includes: + +- `rrdIarmEvents.c` +- `uploadRRDLogs.c` +- `rrd_config.c` +- `rrd_sysinfo.c` +- `rrd_logproc.c` +- `rrd_archive.c` +- `rrd_upload.c` + +### Configure options + +The repo currently exposes these relevant options in `configure.ac`: + +- `--enable-iarmbusSupport=yes|no` +- `--enable-gtestapp=yes|no` +- `--enable-L2support=yes|no` + +### Standard build helper + +The current helper script is: + +```sh +sh cov_build.sh +``` + +This script prepares dependent repositories and builds the module with IARMBus support. It also builds and installs dependencies used by the Remote Debugger test and upload paths, including the external upload implementation dependency. + +### Unit tests + +Run unit tests with: + +```sh +sh run_ut.sh +``` + +This script: + +- enters `src/unittest/` +- regenerates autotools files +- builds `remotedebugger_gtest` +- executes the unit-test binary +- optionally captures coverage data when run with `--enable-cov` + +### L2 functional tests + +Run functional tests with: + +```sh +sh run_l2.sh +``` + +The current L2 harness: + +- prepares `/etc/rrd`, `/tmp/rrd`, and `/lib/rdk` +- copies `remote_debugger.json` and `uploadRRDLogs.sh` +- installs test shims for `systemd-run`, `systemctl`, and `journalctl` +- runs pytest-based functional scenarios for static profile, dynamic profile, single-instance, start control, upload, deep-sleep, harmful command, and C API upload flows +- writes JSON reports under `/tmp/l2_test_report` + +## Runtime Artifacts + +Common runtime paths used by the module include: + +- Static profile location: `/etc/rrd/remote_debugger.json` +- Working report directory: `/tmp/rrd/` +- Upload helper script: `/lib/rdk/uploadRRDLogs.sh` +- Upload lock file: `/tmp/.log-upload.lock` +- Example source log path: `/opt/logs` + +## RFC and Event Interfaces + +The current README-level contract for the feature is: + +- Enable Remote Debugger +- Set an issue type for collection +- Allow the daemon to resolve commands and generate an archive +- Upload the archive through the log-upload stack + +Historically documented RFC parameters remain part of the feature model: + +- `DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.Enable` +- `DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.RDKRemoteDebugger.IssueType` + +The implementation also uses RBUS event subscriptions and webconfig integration to receive and process runtime updates. + +## Versioning + +Given a version number `MAJOR.MINOR.PATCH`, increment the: + +- `MAJOR` version when you make incompatible API changes that break backwards compatibility. +- `MINOR` version when you add backward compatible functionality. +- `PATCH` version when you make backwards compatible bug fixes. + +## Changelog + +Update `CHANGELOG.md` whenever the version changes and place the newest entry at the top. + +Use the following change labels: + +- `Added` +- `Changed` +- `Deprecated` +- `Removed` +- `Fixed` +- `Security` From 8afd127f242c2826714602cce29971fb09f3a087 Mon Sep 17 00:00:00 2001 From: Hanasi Date: Thu, 9 Apr 2026 15:12:37 -0400 Subject: [PATCH 2/2] Updated the instructions --- .../instructions/build-system.instructions.md | 32 +++++++++-------- .../instructions/c-embedded.instructions.md | 4 +-- .../instructions/cpp-testing.instructions.md | 35 +++++++++---------- 3 files changed, 35 insertions(+), 36 deletions(-) diff --git a/.github/instructions/build-system.instructions.md b/.github/instructions/build-system.instructions.md index 4efca56a..9b1b630e 100644 --- a/.github/instructions/build-system.instructions.md +++ b/.github/instructions/build-system.instructions.md @@ -37,16 +37,19 @@ AM_CONDITIONAL([WITH_GTEST_SUPPORT], [test "x$enable_gtest" = "xyes"]) ```makefile # GOOD: Minimal linking -bin_PROGRAMS = dcmd uploadstblogs uploadlogsnow - -dcmd_SOURCES = dcm.c dcm_utils.c dcm_parseconf.c dcm_cronparse.c dcm_schedjob.c dcm_rbus.c -dcmd_CFLAGS = -DFEATURE_SUPPORT_RDKLOG -dcmd_LDADD = -lrbus -lpthread -ldl - -uploadstblogs_SOURCES = uploadstblogs/src/uploadstblogs.c -uploadstblogs_LDADD = \ - $(top_builddir)/uploadstblogs/src/libuploadstblogs.la \ - -lcurl -lssl -lcrypto -lrbus +bin_PROGRAMS = remotedebugger + +remotedebugger_SOURCES = rrdMain.c rrdEventProcess.c rrdJsonParser.c rrdRunCmdThread.c \ + rrdCommandSanity.c rrdDynamic.c rrdExecuteScript.c rrdMsgPackDecoder.c rrdInterface.c +remotedebugger_CFLAGS = -DFEATURE_SUPPORT_RDKLOG +remotedebugger_LDADD = -lrbus -lpthread -ldl + +# Optional upload support (enabled with IARMBUS_ENABLE) +if IARMBUS_ENABLE +remotedebugger_SOURCES += rrdIarmEvents.c uploadRRDLogs.c rrd_config.c rrd_sysinfo.c \ + rrd_logproc.c rrd_archive.c rrd_upload.c +remotedebugger_LDADD += -luploadstblogs -lfwutils -lz +endif # GOOD: Conditional compilation if WITH_GTEST_SUPPORT @@ -97,11 +100,10 @@ AC_SUBST([DBUS_LIBS]) ### Header Organization ```makefile # Include paths -AM_CPPFLAGS = -I$(top_srcdir)/include \ - -I$(top_srcdir)/uploadstblogs/include \ - -I$(top_srcdir)/backup_logs/include \ - -I$(top_srcdir)/usbLogUpload/include \ - $(DBUS_CFLAGS) +AM_CPPFLAGS = -I$(top_srcdir)/src \ + -I$(PKG_CONFIG_SYSROOT_DIR)$(includedir)/rbus \ + -I$(PKG_CONFIG_SYSROOT_DIR)$(includedir) \ + $(CJSON_CFLAGS) ``` ## Build Performance diff --git a/.github/instructions/c-embedded.instructions.md b/.github/instructions/c-embedded.instructions.md index 236cb44f..01fa3f6a 100644 --- a/.github/instructions/c-embedded.instructions.md +++ b/.github/instructions/c-embedded.instructions.md @@ -688,6 +688,6 @@ return 0; ## References - Project follows RDK coding standards -- See `uploadstblogs/include/` for uploadSTBLogs API header documentation -- Review existing code in `uploadstblogs/src/` for patterns +- See `src/rrd_config.h`, `src/rrdInterface.h` for remote debugger API header documentation +- Review existing code in `src/` for patterns - Check `src/unittest/` directory for testing examples diff --git a/.github/instructions/cpp-testing.instructions.md b/.github/instructions/cpp-testing.instructions.md index 28739a25..24718211 100644 --- a/.github/instructions/cpp-testing.instructions.md +++ b/.github/instructions/cpp-testing.instructions.md @@ -1,5 +1,5 @@ --- -applyTo: "unittest/**/*.cpp,unittest/**/*.h,uploadstblogs/unittest/**/*.cpp,uploadstblogs/unittest/**/*.h" +applyTo: "src/unittest/**/*.cpp,src/unittest/**/*.h" --- # C++ Testing Standards (Google Test) @@ -17,17 +17,17 @@ Use Google Test (gtest) and Google Mock (gmock) for all C++ test code. ```cpp // GOOD: Test file structure -// filepath: unittest/dcm_utils_gtest.cpp +// filepath: src/unittest/rrd_config_gtest.cpp extern "C" { -#include "dcm_utils.h" -#include "dcm_types.h" +#include "rrd_config.h" +#include "rrdCommon.h" } #include #include -class DcmUtilsTest : public ::testing::Test { +class RrdConfigTest : public ::testing::Test { protected: void SetUp() override { // Initialize test resources @@ -38,9 +38,9 @@ protected: } }; -TEST_F(DcmUtilsTest, ConfigFileReadWriteRoundTrip) { +TEST_F(RrdConfigTest, ConfigFileReadWriteRoundTrip) { // Test configuration file parsing - const char* config = "/tmp/test.conf"; + const char* config = "/tmp/rrd_test_profile.json"; // verify read back value matches written value ASSERT_EQ(readConfigValue(config, "key"), "value"); } @@ -55,13 +55,13 @@ TEST_F(DcmUtilsTest, ConfigFileReadWriteRoundTrip) { ```cpp extern "C" { -#include "dcm_parseconf.h" -#include "dcm_rbus.h" +#include "rrdJsonParser.h" +#include "rrdRbus.h" } #include -class DcmParseConfTest : public ::testing::Test { +class RrdJsonParserTest : public ::testing::Test { protected: void SetUp() override { // Initialize handler stubs @@ -72,11 +72,11 @@ protected: } }; -TEST_F(DcmParseConfTest, ParseConfigReturnsExpected) { - DCMDHandle handle = {}; - // Test configuration parsing - int result = dcmParseConfig(&handle, "/etc/dcmresponse.txt"); - // verify handler returns success and populates configuration +TEST_F(RrdJsonParserTest, ParseProfileReturnsExpected) { + rrdProfile_t profile = {}; + // Test JSON profile parsing + int result = rrdParseProfile(&profile, "/etc/rrd_profile.json"); + // verify handler returns success and populates profile ASSERT_EQ(result, 0); } ``` @@ -171,10 +171,7 @@ make check ### Memory Checking ```bash valgrind --leak-check=full --show-leak-kinds=all \ - ./unittest/dcm_gtest - -valgrind --leak-check=full --show-leak-kinds=all \ - ./uploadstblogs/unittest/uploadstblogs_gtest + ./src/unittest/rrdUnitTestRunner ``` ### Test Output