Skip to content

fix(esp32): resolve task watchdog timeout in edge_dsp task#242

Closed
chandusurisetty wants to merge 1 commit intoruvnet:mainfrom
chandusurisetty:main
Closed

fix(esp32): resolve task watchdog timeout in edge_dsp task#242
chandusurisetty wants to merge 1 commit intoruvnet:mainfrom
chandusurisetty:main

Conversation

@chandusurisetty
Copy link
Copy Markdown

The 0ms delay causing the ESP32-S3 edge_dsp task to trigger the watchdog timer

Copy link
Copy Markdown
Author

@chandusurisetty chandusurisetty left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Copy Markdown
Owner

@ruvnet ruvnet left a comment

Choose a reason for hiding this comment

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

Code Review — PR #242

Recommendation: REJECT

Summary

This PR from @chandusurisetty claims to fix a task watchdog timeout in the edge_dsp task. The PR description says "The 0ms delay causing the ESP32-S3 edge_dsp task to trigger the watchdog timer."

Actual Changes Analysis

The diff is 786 lines but contains zero functional changes. Every single modification is a whitespace/formatting reformatting:

  1. Brace style: if (x) { changed to if (x)\n{ (Allman style)
  2. Alignment removal: Aligned assignments like bq->b0 = alpha changed to bq->b0 = alpha
  3. Single-line to multi-line: if (x) return; changed to if (x)\n return;
  4. String continuation alignment: Log format strings re-indented

The One Functional Change (Line 833)

// Before:
vTaskDelay(pdMS_TO_TICKS(1));

// After:
vTaskDelay(1);

This is the only non-cosmetic change — and it is incorrect:

  • pdMS_TO_TICKS(1) converts 1 millisecond to the correct number of FreeRTOS ticks based on configTICK_RATE_HZ. At the default 1000 Hz tick rate, pdMS_TO_TICKS(1) == 1, so the behavior happens to be the same.
  • However, removing the macro makes the code non-portable — if configTICK_RATE_HZ is changed (e.g., to 100 Hz for power savings), vTaskDelay(1) would delay 10ms instead of the intended 1ms.
  • This change does not fix the watchdog timeout. The watchdog issue was already fixed in commit 024d2583f ("fix(firmware): edge_dsp task watchdog starvation on Core 1").

Security Assessment

  • No security implications (purely cosmetic changes).

Conflict Assessment (ADR-069 through ADR-078)

  • Would conflict heavily. The PR reformats the entire edge_processing.c file, which has been modified substantially by ADR-069 and prior commits. Every reformatted line would produce a merge conflict.
  • The PR was branched from main but is based on a stale version of edge_processing.c.

Code Quality Concerns

  • 216 additions, 131 deletions with no functional improvement.
  • The reformatting is inconsistent with the project's existing coding style (K&R braces are used throughout the firmware codebase).
  • Mass reformatting without a .clang-format config or team consensus creates unnecessary churn and makes git blame useless for the entire file.

Verdict: REJECT — No functional fix, introduces a subtle portability regression, conflicts with existing work, and reformats the entire file against project style conventions.

@ruvnet
Copy link
Copy Markdown
Owner

ruvnet commented Apr 3, 2026

Thank you for the contribution. The watchdog timeout was already fixed in commit 024d258 (v0.5.3). This PR contains primarily whitespace reformatting which would conflict with recent firmware changes (ADR-069 feature vectors, ADR-073 channel hopping). The pdMS_TO_TICKS(1) → vTaskDelay(1) change also reduces portability across tick rates. Closing — but thank you for looking into the watchdog issue!

@ruvnet ruvnet closed this Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants