Embedded Code Review Expert

https://github.com/ylongw/embedded-review
Admin
An expert code review skill for embedded/firmware projects that detects memory safety issues,...
Tokens:16,545
Snippets:172
Trust Score:5.9
License:MIT
Update:5 months ago
Show doc for...
Tokens:
Raw
### Structured Markdown Report for Code Review

Source: https://context7.com/ylongw/embedded-review/llms.txt

Presents an example of a structured markdown report generated after a dual-model code review, summarizing findings, severity, and consensus.

```markdown
## Embedded Code Review Summary

**Target**: STM32H743 | FreeRTOS 10.4 | arm-none-eabi-gcc 12.2
**Branch**: feat/nfc-improvements
**Files reviewed**: 3 files, 213 lines changed
**Review mode**: Dual-model (Claude Code + Codex)
**Overall assessment**: REQUEST_CHANGES

---

## Findings

### 🔴 P0 - Critical (must block)
1. **[nci_handler.c:88]** Buffer overflow on NFC frame receive [🤝 consensus]
   - Description: `memcpy(local_buf, rx_data, rx_len)` — `rx_len` from RF field is not validated
   - Risk: Stack corruption; arbitrary code execution via malformed NFC frame
   - Fix: `if (rx_len > sizeof(local_buf)) return NCI_ERR_OVERFLOW;` before memcpy

### 🟠 P1 - High (fix before merge)
1. **[nci_handler.c:134]** Missing `portYIELD_FROM_ISR` after queue send [🤝 consensus]
   - Description: `xQueueSendFromISR` called but `portYIELD_FROM_ISR(woken)` omitted
   - Risk: Higher-priority task not scheduled immediately; NFC response latency violation
   - Fix: Add `portYIELD_FROM_ISR(xHigherPriorityTaskWoken);` at end of ISR

2. **[rf_protocol.c:55]** FWT (Frame Waiting Time) timer not reset on retry [🔵 Claude-only]
   - Description: On NACK, code retries command without resetting the FWT countdown
   - Risk: Timeout fires during retry → spurious session abort
   - Fix: Reset FWT timer before each retry attempt

### 🟡 P2 - Medium (fix or follow-up)
1. **[nci.h:23]** NCI response buffer size not a compile-time constant [🟢 Codex-only]
   - Description: `#define NCI_MAX_PAYLOAD 255` used as VLA size in one call site
   - Risk: Stack allocation of unknown size at compile time; rejected by -Wvla
   - Fix: Use fixed-size array or static buffer
```

--------------------------------

### Check-then-Act Race Condition Example

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Demonstrates a non-atomic check-then-act pattern where an ISR might overwrite data after a check but before processing. Fix by copying data within a critical section.

```c
// 1. Check-then-act (non-atomic)
if (uart_rx_ready) {           // ISR sets this
    process(uart_rx_buffer);   // ISR may overwrite buffer HERE
    uart_rx_ready = false;
}
// Fix: copy buffer in critical section, then process outside
```

--------------------------------

### Read-Modify-Write Race Condition Example

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Illustrates a race condition when multiple ISRs modify the same hardware register, potentially corrupting values. Using atomic set/reset registers like BSRR or critical sections can prevent this.

```c
// 2. Read-modify-write on hardware register
GPIOA->ODR |= GPIO_PIN_5;     // ISR modifying other GPIOA pins can corrupt
// Fix: use BSRR register (atomic set/reset) or critical section
```

--------------------------------

### Multi-byte Update Torn Read Example

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Shows a torn read issue where a 32-bit timestamp is updated by an ISR, but a main loop read might capture the high byte from a new update and the low byte from an old one. Protect multi-byte accesses with critical sections or sequence counters.

```c
// 3. Multi-byte update torn read
void ISR(void) {
    timestamp_high++;  // ISR updates high word
    timestamp_low = 0; // then low word
}
void main(void) {
    uint32_t h = timestamp_high;  // could read new high + old low
    uint32_t l = timestamp_low;
}
// Fix: read in critical section, or use sequence counter pattern
```

--------------------------------

### Shared Peripheral Access Race Condition Example

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Highlights a race condition where concurrent tasks accessing a shared peripheral (like SPI) with different configurations can lead to data corruption if one task is preempted mid-operation. Use a mutex per peripheral bus to serialize access.

```c
// 4. Shared peripheral access
// Task A configures SPI for device 1 (CPOL=0)
// Task B configures SPI for device 2 (CPOL=1) — preempts A mid-transfer
// Fix: mutex per peripheral bus
```

--------------------------------

### Cache Cleaning Before DMA Write

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Clean the CPU data cache by address before starting a DMA write operation. This ensures that data written by the CPU is flushed from the cache to main memory before the DMA controller reads it.

```c
SCB_CleanDCache_by_Addr(tx_buf, len);  // before starting DMA TX
```

--------------------------------

### Severity Levels for Findings

Source: https://context7.com/ylongw/embedded-review/llms.txt

This defines the four-level severity scale used for classifying findings, from Critical (P0) to Low (P3). Examples are provided for each level.

```plaintext
P0 — Critical   Must block merge
                Memory corruption, interrupt safety violation, security vulnerability, brick risk
                Examples: unbounded memcpy from wire input, ISR calling blocking FreeRTOS API

P1 — High       Should fix before merge
                Race condition, resource leak, undefined behavior, RTOS misuse
                Examples: missing volatile on ISR-shared flag, torn 64-bit read, missing portYIELD_FROM_ISR

P2 — Medium     Fix or create follow-up ticket
                Code smell, portability issue, missing error handling, suboptimal pattern
                Examples: infinite busy-wait without timeout, VLA from external size, missing NULL check on malloc

P3 — Low        Optional improvement
                Style, naming, documentation, minor suggestion
                Examples: magic number literals, inconsistent naming convention, missing comment on ISR
```

--------------------------------

### Peripheral Initialization Order

Source: https://github.com/ylongw/embedded-review/blob/main/references/hardware-interface.md

Follow this sequence for correct peripheral initialization: enable clock, configure GPIO, configure registers, enable peripheral, and enable NVIC interrupt. Incorrect order can lead to ignored writes or spurious interrupts.

```c
1. Enable clock to peripheral        (RCC->APBxENR |= ...)
2. Configure GPIO pins              (alternate function, pull-up/down, speed)
3. Configure peripheral registers   (baud rate, mode, DMA, interrupts)
4. Enable peripheral                (USARTx->CR1 |= USART_CR1_UE)
5. Enable interrupt in NVIC         (NVIC_EnableIRQ)
```

--------------------------------

### Prepare Review Payloads for Dual-Model Review

Source: https://context7.com/ylongw/embedded-review/llms.txt

Illustrates the preparation of task prompts for two AI models (Claude Code and Codex) for independent code review, including context and desired output format.

```shell
# Step 1: Prepare independent review payloads from the same REVIEW_CONTEXT

Claude Code task prompt:
  "You are a senior embedded systems engineer reviewing firmware code changes.
   [REVIEW_CONTEXT: branch, MCU, RTOS, compiler, full diff, focus areas]
   Review against: memory-safety, interrupt-safety, hardware-interfaces, c-pitfalls, architecture, security.
   Format: [P0/P1/P2/P3] [file:line] Title / Description / Risk / Suggested fix
   Mark uncertain items with [?]."

Codex task prompt:
  "You are an independent firmware code reviewer.
   [REVIEW_CONTEXT: same as above]
   Focus: memory corruption, concurrency bugs, hardware errors, logic errors, security.
   Format: [SEVERITY: critical/high/medium/low] [file:line] Title / What's wrong / Impact / Fix
   Report everything — do not skip low-severity items."
```

--------------------------------

### Spawn Parallel AI Review Sessions

Source: https://context7.com/ylongw/embedded-review/llms.txt

Shows how to spawn parallel review sessions using AI agents (Claude Code and Codex) with specified runtimes and tasks. The system waits for both sessions to complete.

```python
# Step 2: Spawn parallel sessions
sessions_spawn(runtime="acp", agentId="claude-code", task=claude_task)
sessions_spawn(runtime="acp", agentId="codex",       task=codex_task)
# Both run simultaneously. Skill waits for both to complete.
```

--------------------------------

### FreeRTOS API Usage in ISR

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Demonstrates the correct usage of FreeRTOS queue functions from an ISR context. Uses `xQueueSendFromISR` and `portYIELD_FROM_ISR` to safely send data and potentially yield the processor.

```c
// WRONG:
void UART_IRQHandler(void) {
    xQueueSend(queue, &data, portMAX_DELAY);  // WRONG: task version, may block
}

// RIGHT:
void UART_IRQHandler(void) {
    BaseType_t xHigherPriorityTaskWoken = pdFALSE;
    xQueueSendFromISR(queue, &data, &xHigherPriorityTaskWoken);
    portYIELD_FROM_ISR(xHigherPriorityTaskWoken);  // Context switch if needed
}
```

--------------------------------

### REVIEW_CONTEXT Package Structure

Source: https://context7.com/ylongw/embedded-review/llms.txt

The `REVIEW_CONTEXT` package is assembled after running `prepare-diff.sh` and is used by both single- and dual-model review phases. It contains repository information, the full git diff, relevant reference documents, and detected focus areas.

```json
REVIEW_CONTEXT = {
  repo_info:   { branch: "feat/nfc-improvements", mcu: "STM32", rtos: "FreeRTOS", compiler: "arm-none-eabi" },
  diff:        "<full git diff text>",
  references:  [
    "references/memory-safety.md",      // loaded when diff touches buffers, DMA, heap
    "references/interrupt-safety.md",   // loaded when diff touches ISR, tasks, shared vars
    "references/hardware-interface.md", // loaded when diff touches peripherals, registers
    "references/c-pitfalls.md"          // loaded for all C/C++ changes
  ],
  focus_areas: ["NFC", "ISR/DMA"]       // auto-detected or user-specified
}

# Decision logic:
# - No changes        → inform user, offer staged or commit range
# - SMALL (≤100 loc)  → single-model review
# - LARGE (>500 loc)  → summarize by file/module, review in batches
# - Critical path     → always recommend dual-model

```

--------------------------------

### Sign Extension Surprise

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

When a signed integer is converted to a larger unsigned integer type, sign extension can occur, preserving the sign bit. To get the expected bit pattern, cast through an unsigned type of the same size first.

```c
int8_t signed_byte = 0xFF;  // = -1
uint32_t extended = signed_byte;  // = 0xFFFFFFFF, not 0xFF!
// Fix: cast through unsigned first
uint32_t correct = (uint8_t)signed_byte;  // = 0xFF
```

--------------------------------

### Prepare Diff Script

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

This script prepares the diff for review, gathering repository information, target identification, and diff statistics. It helps determine the review mode and identify critical paths.

```bash
scripts/prepare-diff.sh <repo_path> [diff_range]
```

--------------------------------

### Uninitialized Local Variables (UB)

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Using uninitialized local variables results in undefined behavior as their value is indeterminate. Always initialize variables before use.

```c
int x;
if (x > 0) { ... }  // UB — x could be anything
```

--------------------------------

### Handle Signed Integer Overflow in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Illustrates a signed integer overflow vulnerability and provides a safe fix by checking for potential overflow before addition.

```c
// [P1] src/parser/frame.c:67  Signed integer overflow — UB, corrupted length calculation
int32_t len = base + offset;       // ❌ if base=INT32_MAX, +1 is UB; compiler may optimize assuming no overflow
// Fix:
if (offset > 0 && base > INT32_MAX - offset) return ERR_OVERFLOW;
int32_t len = base + offset;
```

--------------------------------

### C Conditional Compilation: Include Guards

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Demonstrates the standard include guard pattern using `#ifndef`, `#define`, and `#endif` to prevent multiple inclusions of header files. Use unique names to avoid collisions.

```c
// Use unique names: PROJECT_MODULE_FILE_H
#ifndef FIRMWARE_PRO2_NFC_HANDLER_H
#define FIRMWARE_PRO2_NFC_HANDLER_H
...
#endif
```

--------------------------------

### prepare-diff.sh Script Usage

Source: https://context7.com/ylongw/embedded-review/llms.txt

Extracts git diff, identifies the target MCU/RTOS/compiler from build files, classifies diff size, and detects critical code paths. This script is called as Phase 0 of every review.

```bash
# Usage: prepare-diff.sh [repo_path] [diff_range]

# Example output for a medium NFC-related change:
$ bash scripts/prepare-diff.sh ~/firmware/firmware-pro2 HEAD~2..HEAD

=== REPO INFO ===
Path: /home/user/firmware/firmware-pro2
Branch: feat/nfc-improvements
Last commit: a3f91bc Fix NFC frame waiting time handling

=== TARGET IDENTIFICATION ===
CMakeLists.txt  (matched: STM32)
CMakeLists.txt  (matched: FreeRTOS)
CMakeLists.txt  (matched: arm-none-eabi)

=== DIFF STAT ===
 src/nfc/nci_handler.c   | 48 +++++++++++++++++++++------
 src/nfc/rf_protocol.c   | 22 ++++++-------
 include/nfc/nci.h       |  5 +++
 3 files changed, 52 insertions(+), 23 deletions(-)

=== DIFF SIZE ===
Lines: 213
Assessment: MEDIUM
Critical paths detected: NFC
  → Dual-model cross-review recommended

=== DIFF ===
diff --git a/src/nfc/nci_handler.c b/src/nfc/nci_handler.c
...
```

--------------------------------

### Review Context Package Structure

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

Defines the structure of the review context package, which includes repository information, the full git diff, relevant checklist references, and identified focus areas.

```json
{
  repo_info: (branch, MCU, RTOS, compiler),
  diff: (full git diff text),
  references: (relevant checklist sections from references/),
  focus_areas: (user-specified or auto-detected critical paths)
}
```

--------------------------------

### C Portability: Endianness Conversion Built-ins

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Shows how to use GCC built-in functions like `__builtin_bswap16` and `__builtin_bswap32` for efficient byte swapping to convert between host and network/protocol byte orders.

```c
// Converting between host and network/protocol byte order:
uint16_t __builtin_bswap16(uint16_t);
uint32_t __builtin_bswap32(uint32_t);
```

--------------------------------

### Safe String Copying with snprintf

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Use `snprintf` with the buffer size to prevent buffer overflows when formatting strings. This is a safer alternative to `sprintf`.

```c
snprintf(buf, sizeof(buf), ...)
```

--------------------------------

### Peripheral Clock Enable Before Configuration in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Ensure the clock for a peripheral is enabled before attempting to configure its registers. Accessing registers before the clock is active can lead to undefined behavior.

```c
// --- FINDINGS EXAMPLE ---

// [P1] src/spi/flash.c:12  Peripheral configured before clock enabled
void SPI_Init(void) {
    SPI1->CR1 = SPI_CR1_MSTR | SPI_CR1_SPE;  // ❌ writes to dead register — RCC clock not enabled yet
    __HAL_RCC_SPI1_CLK_ENABLE();
    // Fix — enable clock first:
    __HAL_RCC_SPI1_CLK_ENABLE();
    SPI1->CR1 = SPI_CR1_MSTR | SPI_CR1_SPE;
}

```

--------------------------------

### Division by Zero (UB)

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Division or modulo by zero is undefined behavior, even for unsigned integers. Always check the divisor before performing the operation.

```c
// Division by zero is UB (even for unsigned)
uint32_t ratio = total / count;  // Must check count != 0

// Integer division truncates toward zero (C99+)
int x = -7 / 2;  // = -3, not -4
int y = -7 % 2;  // = -1 (sign follows dividend in C99+)
```

--------------------------------

### Unbounded Copy from External Input

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

This pattern shows an unbounded copy from external data (`data`) into a fixed-size buffer (`local_buf`). If `len` exceeds the buffer size, a buffer overflow will occur.

```c
// 1. Unbounded copy from external input
void handle_packet(uint8_t *data, uint16_t len) {
    uint8_t local_buf[64];
    memcpy(local_buf, data, len);  // P0: len could be > 64
}
```

--------------------------------

### C Linker Issue: Section Placement

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Demonstrates using `__attribute__((section(...)))` to place data into specific memory sections, such as `.dtcm_data` for fast access, which is crucial for performance-critical embedded applications.

```c
__attribute__((section(".dtcm_data"))) uint8_t fast_buffer[1024];
```

--------------------------------

### Next Steps Markdown

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

A markdown template for outlining the next steps after an embedded code review, including the number of issues found and options for proceeding with fixes.

```markdown
---
## Next Steps

Found X issues (P0: _, P1: _, P2: _, P3: _).

**How would you like to proceed?**
1. **Fix all** — implement all suggested fixes
2. **Fix P0/P1 only** — address critical and high priority
3. **Fix specific items** — tell me which issues to fix
4. **Re-review with dual-model** — run cross-review (if single-model was used)
5. **No changes** — review complete
```

--------------------------------

### Reference Checklists Directory Structure

Source: https://context7.com/ylongw/embedded-review/llms.txt

This structure shows the organization of reference checklists used by the expert. Each checklist covers specific areas of embedded C/C++ development.

```bash
references/
├── memory-safety.md       # Stack overflow, buffer overrun, alignment, DMA cache, MMIO volatile, heap
├── interrupt-safety.md    # volatile rules, atomicity table, critical sections, ISR best practices,
│                          # FreeRTOS FromISR API, priority inversion, deadlock, race condition patterns
├── hardware-interface.md  # Peripheral init order, register access (RMW, W1C, write-only), timeouts,
│                          # I2C/SPI/UART/NFC protocols, GPIO config, clock tree, DMA setup
└── c-pitfalls.md          # Undefined behavior, integer promotion, strict aliasing, preprocessor macros,
                           # portability, type safety, linker issues

# Loading logic (applied during Phase 1 / Phase 2 payload construction):
if diff touches buffers, DMA, malloc/free → load memory-safety.md
if diff touches ISR, task, semaphore      → load interrupt-safety.md
if diff touches peripheral, register      → load hardware-interface.md
always                                    → load c-pitfalls.md
```

--------------------------------

### C Portability: Fixed-Width Integer Types

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Recommends using fixed-width integer types from `<stdint.h>` (e.g., `uint8_t`, `int32_t`) in embedded systems to ensure consistent data sizes across different platforms, unlike standard types like `int` or `long`.

```c
// ALWAYS use <stdint.h> types in embedded:
uint8_t, int8_t, uint16_t, int16_t, uint32_t, int32_t
// NOT: char, short, int, long (sizes vary by platform)

// For "at least N bits" (allows compiler to pick efficient size):
uint_fast8_t, uint_least8_t

// For pointer-sized integers:
uintptr_t, intptr_t
```

--------------------------------

### Fix Strict Aliasing Violation in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Demonstrates a strict aliasing violation and its fix using `memcpy`. Ensure type-punning is done safely to avoid undefined behavior.

```c
// --- FINDINGS EXAMPLE --- 

// [P1] src/math/crc.c:14  Strict aliasing violation — UB, may produce wrong CRC
float f = sensor_reading;
uint32_t bits = *(uint32_t*)&f;    // ❌ UB: strict aliasing violation
// Fix — use memcpy (compiler optimizes to single register move):
uint32_t bits;
memcpy(&bits, &f, sizeof(bits));   // ✅ defined behavior
```

--------------------------------

### C Conditional Compilation: #if vs #ifdef

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Highlights the difference between `#ifdef` and `#if` in C conditional compilation. `#ifdef` checks for macro definition existence, while `#if` evaluates the macro's value.

```c
#define FEATURE 0
#ifdef FEATURE   // TRUE — FEATURE is defined (even though it's 0)
#if FEATURE      // FALSE — FEATURE is 0
```

--------------------------------

### Safe Memory Copying with memcpy

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

When using `memcpy`, always validate that the size to be copied does not exceed the destination buffer's capacity to prevent overflows.

```c
memcpy(dst, src, len)
```

--------------------------------

### C++ Linker Issue: Missing extern "C"

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Shows how to use `extern "C"` to ensure C linkage for functions, preventing C++ name mangling issues when linking C code with C++ projects, especially for interrupt handlers.

```c
#ifdef __cplusplus
extern "C" {
#endif
void ISR_Handler(void);  // Must use C linkage for vector table
#ifdef __cplusplus
}
#endif
```

--------------------------------

### Optimization Trap: Reordering

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Compilers may reorder instructions for optimization. If strict hardware ordering is required, use memory barrier intrinsics (e.g., `__DMB()` on ARM) to enforce execution order.

```c
// 3. Reordering
x = 1;
y = 2;
// Compiler may swap these. If hardware ordering matters:
x = 1;
__DMB();  // Data Memory Barrier
y = 2;
```

--------------------------------

### Safe Bit Setting with Shift Operations

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

When setting bits using left shifts, ensure the literal `1` is unsigned (`1U`) to avoid potential signed shift issues. Always validate the bit index to prevent undefined behavior from shifts outside the valid range.

```c
void set_bit(uint32_t *reg, int bit) {
    if (bit >= 0 && bit < 32)  // Guard!
        *reg |= (1U << bit);   // Note: 1U, not 1 (avoids signed shift)
}
```

--------------------------------

### FreeRTOS Critical Section (Task Context)

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Enters a critical section from a task context in FreeRTOS by disabling interrupts up to the system call priority level. Must be exited using taskEXIT_CRITICAL().

```c
taskENTER_CRITICAL();    // Disables interrupts up to configMAX_SYSCALL_INTERRUPT_PRIORITY
// ... critical section ...
taskEXIT_CRITICAL();
```

--------------------------------

### Comparison of Signed and Unsigned Integers

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Comparing signed and unsigned integers can lead to surprising results. The signed integer is typically converted to unsigned, potentially resulting in a very large positive value, making the comparison false.

```c
int x = -1;
unsigned int y = 0;
if (x < y) { ... }  // FALSE! -1 is converted to UINT_MAX
```

--------------------------------

### Busy-Wait vs. Timeout Implementation

Source: https://github.com/ylongw/embedded-review/blob/main/references/hardware-interface.md

Implement busy-wait loops with timeouts to prevent infinite hangs, especially when waiting for peripheral status flags. Avoid loop counters that the compiler might optimize away; use volatile variables or hardware timers for reliable timeouts.

```c
// DANGEROUS: infinite wait
while (!(SPI1->SR & SPI_SR_TXE));  // Hangs if SPI is misconfigured

// SAFE: with timeout
uint32_t timeout = HAL_GetTick() + 100;
while (!(SPI1->SR & SPI_SR_TXE)) {
    if (HAL_GetTick() > timeout) {
        return ERROR_TIMEOUT;
    }
}

// ALSO DANGEROUS: timeout using loop counter
for (int i = 0; i < 100000; i++) {  // Compiler may optimize away!
    if (SPI1->SR & SPI_SR_TXE) break;
}
// Fix: make loop variable volatile, or use hardware timer
```

--------------------------------

### Correct FreeRTOS API in ISR Context

Source: https://context7.com/ylongw/embedded-review/llms.txt

Use FreeRTOS `FromISR` variants of API calls (e.g., `xQueueSendFromISR`) when operating within an interrupt service routine. Using task-level APIs can lead to blocking and crashes.

```c
// [P0] src/comm/uart.c:78  Wrong FreeRTOS API in ISR context
void UART1_IRQHandler(void) {
    xQueueSend(rx_queue, &byte, portMAX_DELAY);  // ❌ task API — may block, causes crash
    // Fix:
    BaseType_t woken = pdFALSE;
    xQueueSendFromISR(rx_queue, &byte, &woken);  // ✅ ISR-safe variant
    portYIELD_FROM_ISR(woken);                   // ✅ context switch if higher-priority task unblocked
}

```

--------------------------------

### Correct Macro Precedence with Parentheses in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Shows a common macro pitfall where missing parentheses lead to incorrect operator precedence. Always fully parenthesize macro definitions.

```c
// [P2] src/util/bits.c:9  Macro without parentheses — wrong operator precedence
#define BIT(n)  1 << n             // ❌ BIT(3) | BIT(4)  →  1 << (3 | 4)  →  wrong!
// Fix:
#define BIT(n)  (1UL << (n))       // ✅ fully parenthesized, unsigned
```

--------------------------------

### C Macro Pitfalls: Parentheses and Evaluation

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Demonstrates common issues with C macros like missing parentheses leading to incorrect order of operations and double argument evaluation. Use parentheses around arguments and the entire macro body for safety. For double evaluation, consider inline functions or GCC statement expressions.

```c
#define SQUARE(x) x * x
int y = SQUARE(1 + 2);  // = 1 + 2 * 1 + 2 = 5, not 9
// Fix:
#define SQUARE(x) ((x) * (x))
```

```c
#define MAX(a, b) ((a) > (b) ? (a) : (b))
int z = MAX(i++, j);  // i++ evaluated twice if i > j!
// Fix: use inline function or GCC statement expression
#define MAX(a, b) ({ typeof(a) _a = (a); typeof(b) _b = (b); _a > _b ? _a : _b; })
```

--------------------------------

### C Macro Pitfalls: Semicolon and do-while(0)

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Shows how a semicolon in a macro definition can create an empty statement, causing issues with `if-else` constructs. Use `do { ... } while(0)` to ensure the macro behaves like a single statement.

```c
#define LOG(msg) printf("%s\n", msg);
if (debug)
    LOG("test");  // Semicolon creates empty statement, else attaches wrong
else
    ...
// Fix: use do { ... } while(0)
#define LOG(msg) do { printf("%s\n", msg); } while(0)
```

--------------------------------

### C Macro Pitfalls: Stringification and Token Pasting

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Illustrates C preprocessor directives for stringification (`#`) and token pasting (`##`). Nested macros require an indirection layer to expand correctly.

```c
#define STR(x) #x      // STR(foo) → "foo"
#define CONCAT(a,b) a##b  // CONCAT(foo,bar) → foobar
// Nested macros need indirection:
#define XSTR(x) STR(x)  // XSTR(VERSION) → "3" (expands VERSION first)
```

--------------------------------

### Optimization Trap: Dead Store Elimination

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Compilers may remove seemingly dead stores (e.g., `memset` to a buffer that isn't subsequently read) if they assume the memory is not used. Use `volatile` or specific functions like `explicit_bzero` to prevent this.

```c
// 1. Dead store elimination
void clear_secret(uint8_t *buf, size_t len) {
    memset(buf, 0, len);  // Compiler may remove this if buf isn't used after
}
// Fix: use volatile or explicit_bzero/SecureZeroMemory
```

--------------------------------

### FreeRTOS Critical Section (ISR Context)

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Enters a critical section from an ISR context in FreeRTOS. Returns a saved state that must be used to exit the critical section correctly using taskEXIT_CRITICAL_FROM_ISR().

```c
UBaseType_t saved = taskENTER_CRITICAL_FROM_ISR();
// ... critical section ...
taskEXIT_CRITICAL_FROM_ISR(saved);
```

--------------------------------

### Write-1-to-Clear (W1C) Register Handling

Source: https://github.com/ylongw/embedded-review/blob/main/references/hardware-interface.md

When dealing with Write-1-to-Clear registers, correctly clear specific flags by writing '1' only to the intended flag bit. Avoid writing '0' to all bits, which can unintentionally clear other flags. Always consult the reference manual for register behavior (W1C vs. W0C).

```c
// DANGEROUS: clearing all flags unintentionally
USART1->SR = 0;  // May clear flags you didn't intend to clear

// CORRECT: write 1 only to the flag you want to clear
USART1->SR = ~USART_SR_TC;  // Clear TC, preserve others (on W1C registers)
// BUT: read the reference manual — some registers are W1C, others are W0C!
```

--------------------------------

### DMA Data Width Alignment in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Ensure that the DMA peripheral data alignment matches the peripheral's data register width. Mismatches can lead to data corruption, where each transfer overwrites adjacent memory.

```c
// [P2] src/dma/rx.c:45  DMA data width mismatch
// Peripheral data register is 8-bit (UART DR), but DMA configured for 16-bit word transfer
// → every byte received overwrites adjacent memory
hdma_usart1_rx.Init.PeriphDataAlignment = DMA_PDATAALIGN_BYTE;   // ✅ must match peripheral
hdma_usart1_rx.Init.MemDataAlignment    = DMA_MDATAALIGN_BYTE;   // ✅

```

--------------------------------

### Embedded Code Review Summary Markdown

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

A markdown template for summarizing the results of an embedded code review, including target details, review mode, findings, and cross-review analysis.

```markdown
## Embedded Code Review Summary

**Target**: [MCU/Board] | [RTOS/Bare-metal] | [Compiler]
**Branch**: [branch name]
**Files reviewed**: X files, Y lines changed
**Review mode**: [Single-model / Dual-model (Claude Code + Codex)]
**Overall assessment**: [APPROVE / REQUEST_CHANGES / COMMENT]

---

## Findings

### 🔴 P0 - Critical (must block)
(none or list)

### 🟠 P1 - High (fix before merge)
1. **[file:line]** Brief title [🤝 consensus / 🔵 Claude-only / 🟢 Codex-only]
   - Description of issue
   - Risk: what can go wrong
   - Suggested fix

### 🟡 P2 - Medium (fix or follow-up)
...

### ⚪ P3 - Low (optional)
...

---

## Cross-Review Analysis (dual-model only)

| Metric | Count |
|--------|-------|
| 🤝 Consensus (both found) | X |
| 🔵 Claude-only | Y |
| 🟢 Codex-only | Z |
| ⚠️ Contradictions | W |

### Notable disagreements
(list any contradictions with both perspectives)

---

## Hardware/Timing Concerns
(register access, peripheral init, timing-sensitive code)

## Architecture Notes
(layering, testability, portability observations)
```

--------------------------------

### C Conditional Compilation: Macro Expansion in Includes

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Warns against defining macros that could unintentionally expand identifiers within included header files, potentially breaking their functionality.

```c
#define status 0  // Now every use of "status" in included headers is broken
```

--------------------------------

### Spawn Parallel ACP Sessions

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

Initiates two parallel ACP sessions, one for the Claude Code agent and one for the Codex agent, each with their respective defined tasks.

```python
sessions_spawn(runtime="acp", agentId="claude-code", task=claude_task)
sessions_spawn(runtime="acp", agentId="codex", task=codex_task)
```

--------------------------------

### Safe Input Reading with fgets

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Use `fgets` to read input safely into a buffer, specifying the maximum number of characters to read. This prevents buffer overflows.

```c
fgets(buf, size, stdin)
```

--------------------------------

### Safe String Copying with strncpy

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Use `strncpy` for safer string copying, ensuring the destination buffer is not overflowed. Be mindful of null termination.

```c
strncpy
```

--------------------------------

### Shift Operations: UB and Implementation-Defined Behavior

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Shifting by a number of bits greater than or equal to the width of the promoted type is undefined behavior. Left-shifting negative values is also UB. Right-shifting negative values is implementation-defined (arithmetic vs. logical shift). Always ensure shift amounts are within valid ranges and use unsigned types for predictable bit manipulation.

```c
// Shifting by >= type width is UB
uint32_t x = 1;
uint32_t y = x << 32;  // UB

// Shifting negative values is UB (left shift) or implementation-defined (right shift)
int x = -1;
int y = x << 1;   // UB
int z = x >> 1;   // Implementation-defined (arithmetic vs. logical shift)
```

--------------------------------

### Atomic GPIO Register Access in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Use atomic register access methods like BSRR for GPIO pin manipulation to avoid race conditions with ISRs. This ensures that operations on one pin do not interfere with others.

```c
// [P1] src/gpio/led.c:19  GPIO read-modify-write race with ISR
GPIOA->ODR |= (1 << 5);        // ❌ RMW: ISR modifying another pin corrupts this write
// Fix — use atomic BSRR:
GPIOA->BSRR = (1 << 5);        // ✅ atomic set bit 5
GPIOA->BSRR = (1 << (5 + 16)); // ✅ atomic reset bit 5

```

--------------------------------

### Safe String Concatenation with strncat

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Use `strncat` for safer string concatenation, specifying the maximum number of characters to append. Ensure remaining size is tracked.

```c
strncat
```

--------------------------------

### Invoke Embedded Review Skill

Source: https://context7.com/ylongw/embedded-review/llms.txt

Trigger the embedded review skill by passing a repository path and optional git diff range. The skill auto-selects single- or dual-model mode based on diff size and critical path detection.

```bash
# Review uncommitted changes in a firmware repo
/embedded-review ~/firmware/firmware-pro2
```

```bash
# Review the last 5 commits
/embedded-review ~/firmware/firmware-pro2 HEAD~5..HEAD
```

```bash
# Review a feature branch vs main
/embedded-review ~/firmware/firmware-pro2 main..feat/nfc
```

```bash
# Review a GitHub PR (URL passed as argument)
/embedded-review https://github.com/org/firmware-pro2/pull/42
```

```bash
# Force dual-model cross-review explicitly
"用双模型 review ~/firmware/firmware-pro2"
```

```bash
# Force quick single-model review
"quick review ~/firmware/firmware-pro2 HEAD~1..HEAD"
```

--------------------------------

### Unsigned to Signed Narrowing Conversion

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Converting a large unsigned integer to a smaller signed integer type results in implementation-defined behavior. The value may be truncated or wrap around, often leading to unexpected results.

```c
uint32_t big = 0xFFFFFFFF;
int16_t small = big;  // Implementation-defined, likely -1 or truncated
```

--------------------------------

### C Portability: Incorrect Endianness and Alignment

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Illustrates a dangerous practice of directly casting a pointer to read a `uint16_t` from a packet, which can lead to incorrect results due to wrong endianness and potential unaligned access violations.

```c
// Don't do this:
uint16_t val = *(uint16_t*)packet;  // Wrong endian + possibly unaligned
```

--------------------------------

### Bare-metal Critical Section: Global Interrupt Disable

Source: https://github.com/ylongw/embedded-review/blob/main/references/interrupt-safety.md

Disables all interrupts globally to create a critical section. Always restore the previous interrupt mask state to avoid unintended side effects.

```c
uint32_t primask = __get_PRIMASK();
__disable_irq();
// ... critical section ...
__set_PRIMASK(primask);  // Restore previous state, don't blindly enable
```

--------------------------------

### NULL Check After Malloc in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Always check the return value of `malloc` before using the allocated memory. Failure to do so can lead to crashes if `malloc` returns `NULL` due to memory exhaustion.

```c
// [P0] src/util/string.c:201  Missing NULL check after malloc
char *buf = malloc(128);
memset(buf, 0, 128);  // ❌ crashes if malloc returns NULL on heap exhaustion
// Fix:
if (!buf) return ERR_NOMEM;
memset(buf, 0, 128);

```

--------------------------------

### Codex Task Definition

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

Defines the persona, context, focus areas, and output format for the Codex model in an independent embedded/firmware code review.

```text
You are an independent code reviewer for embedded/firmware projects.
Your job is to find bugs, security issues, and correctness problems.

[REVIEW_CONTEXT: repo info, diff, focus areas]

Focus on:
1. Memory corruption risks (buffer overflow, use-after-free, stack overflow)
2. Concurrency bugs (race conditions, missing volatile, ISR safety)
3. Hardware interface errors (timing, register access, peripheral init)
4. Logic errors and edge cases
5. Security vulnerabilities

Output: List every issue found as:
[SEVERITY: critical/high/medium/low] [file:line] Issue title
- What's wrong
- What could happen
- How to fix

Do NOT skip low-severity items. Report everything.
```

--------------------------------

### I2C Timeout for Busy-Wait in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Implement timeouts for busy-wait loops, especially in I2C communication. This prevents the system from hanging indefinitely if the I2C bus is stuck or misconfigured.

```c
// [P1] src/i2c/sensor.c:88  Infinite busy-wait — hangs on misconfigured I2C
void i2c_write_byte(uint8_t data) {
    while (!(I2C1->SR1 & I2C_SR1_TXE));  // ❌ hangs forever if I2C bus is stuck
    // Fix — add timeout:
    uint32_t deadline = HAL_GetTick() + 10;  // 10ms timeout
    while (!(I2C1->SR1 & I2C_SR1_TXE)) {
        if (HAL_GetTick() > deadline) { i2c_recover_bus(); return; }
    }
    I2C1->DR = data;
}

```

--------------------------------

### Claude Code Task Definition

Source: https://github.com/ylongw/embedded-review/blob/main/SKILL.md

Defines the persona, context, review checklist, and output format for the Claude Code model in an embedded systems firmware review.

```text
You are a senior embedded systems engineer reviewing firmware code changes.

[REVIEW_CONTEXT: repo info, diff, focus areas]

Review checklist (apply all that are relevant):
- Memory safety (references/memory-safety.md)
- Interrupt & concurrency (references/interrupt-safety.md)
- Hardware interfaces (references/hardware-interface.md)
- C/C++ pitfalls (references/c-pitfalls.md)
- Architecture & security

Output format: For each finding, provide:
[P0/P1/P2/P3] [file:line] Title
- Description
- Risk
- Suggested fix

Be thorough. Flag everything you find, even if uncertain — mark uncertain items with [?].
```

--------------------------------

### Aligned Buffer Declaration for DMA

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Declare buffers with specific alignment attributes (e.g., `__attribute__((aligned(32)))`) when required by peripherals or DMA controllers to ensure proper data transfer.

```c
__attribute__((aligned(32))) uint8_t dma_buf[256];
```

--------------------------------

### Safe Register Access Patterns

Source: https://github.com/ylongw/embedded-review/blob/main/references/hardware-interface.md

Avoid read-modify-write hazards on ISR-accessible registers by using atomic set/reset registers. For bit-fields, use explicit mask-and-shift operations to prevent race conditions.

```c
// DANGEROUS in ISR-accessible registers:
GPIOA->ODR |= (1 << 5);  // RMW: read ODR, OR bit, write ODR
// If ISR modifies another bit between read and write → that bit is lost

// SAFE: use atomic set/reset registers
GPIOA->BSRR = (1 << 5);       // Atomic set bit 5
GPIOA->BSRR = (1 << (5+16));  // Atomic reset bit 5
```

```c
// DANGEROUS: compiler may generate RMW for bit-field access
typedef struct {
    uint32_t MODE : 2;
    uint32_t SPEED : 2;
    uint32_t PULL : 2;
} GPIO_Bits;
// If ISR accesses different fields in same word → race condition

// SAFE: use explicit mask-and-shift
reg = (reg & ~MASK) | (value << SHIFT);
```

--------------------------------

### C Portability: Endianness Conversion Portable Approach

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Provides a portable C function `read_be16` for reading big-endian 16-bit values from a byte array, avoiding direct pointer casts which can be incorrect due to endianness and alignment issues.

```c
// Or portable approach:
uint16_t read_be16(const uint8_t *p) {
    return ((uint16_t)p[0] << 8) | p[1];
}
```

--------------------------------

### Missing NULL Check After Malloc

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

This pattern highlights a potential issue where the return value of `malloc` is not checked for `NULL`. If `malloc` fails, subsequent operations on the pointer `p` will lead to a crash.

```c
// 4. Missing NULL check
char *p = malloc(128);
memset(p, 0, 128);  // P0: p could be NULL
```

--------------------------------

### Integer Promotion in Expressions

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Integer types smaller than `int` (like `uint8_t`, `int8_t`, `uint16_t`, `int16_t`) are promoted to `int` when used in expressions. This can lead to unexpected truncation if the result exceeds the range of the original smaller type.

```c
uint8_t a = 200, b = 100;
uint8_t c = a + b;  // Promoted to int, result 300, truncated to 44
// No warning from most compilers!
```

--------------------------------

### Prevent Implicit Sign Extension in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Explains the surprise of implicit sign extension when assigning a signed char to an unsigned int. Cast through an unsigned type first to ensure correct behavior.

```c
// [P2] src/config/nvm.c:33  Implicit sign extension surprise
int8_t  raw  = read_register();    // raw = 0xFF = -1 as signed
uint32_t val = raw;                // ❌ sign-extended to 0xFFFFFFFF, not 0x000000FF
// Fix:
uint32_t val = (uint8_t)raw;       // ✅ cast through unsigned first
```

--------------------------------

### Fix Unbounded memcpy in C

Source: https://context7.com/ylongw/embedded-review/llms.txt

Prevent buffer overflows by checking the length against the destination buffer size before copying data. This is crucial when copying data from untrusted sources.

```c
// --- FINDINGS EXAMPLE ---

// [P0] src/comm/packet.c:34  Unbounded memcpy from wire input
void handle_packet(uint8_t *data, uint16_t len) {
    uint8_t local_buf[64];
    memcpy(local_buf, data, len);  // ❌ len is attacker-controlled; no bounds check
    // Fix:
    if (len > sizeof(local_buf)) return ERR_OVERFLOW;
    memcpy(local_buf, data, len);
}
```

--------------------------------

### Unaligned Memory Access

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

This code snippet demonstrates an unaligned memory access by casting a byte pointer to a word pointer. This can cause a fault on architectures with strict alignment requirements.

```c
// 3. Unaligned access
uint16_t get_word(uint8_t *p) {
    return *(uint16_t*)p;  // P1: unaligned on M0
}
```

--------------------------------

### Safe Unaligned Memory Access

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Avoid direct casting of pointers for unaligned access on strict-alignment targets. Use `memcpy` to safely copy data to a correctly aligned variable.

```c
// DANGEROUS on strict-align targets:
uint32_t val = *(uint32_t*)&rx_buf[3];

// SAFE:
uint32_t val;
memcpy(&val, &rx_buf[3], sizeof(val));
```

--------------------------------

### Array Out-of-Bounds Access (UB)

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Accessing an array beyond its defined bounds is undefined behavior and can silently corrupt adjacent memory, such as other stack variables.

```c
int arr[10];
arr[10] = 42;  // UB — may corrupt adjacent stack variables silently
```

--------------------------------

### Safe scanf Usage with Width Specifier

Source: https://github.com/ylongw/embedded-review/blob/main/references/memory-safety.md

Use a width specifier with `scanf` for string inputs to limit the number of characters read, preventing buffer overflows.

```c
scanf("%255s", buf)
```

--------------------------------

### Volatile Keyword Usage

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

The `volatile` keyword is essential for hardware registers (MMIO), variables shared with ISRs, and variables shared between RTOS tasks (in addition to synchronization). It prevents compiler optimizations that assume a variable's value doesn't change unexpectedly. It does NOT guarantee atomicity, memory barriers, or mutual exclusion.

```c
// volatile tells compiler: "this value may change without your knowledge"
// REQUIRED for:
// - Hardware registers (MMIO)
// - Variables shared with ISR
// - Variables shared between RTOS tasks (in addition to synchronization)

// NOT a substitute for:
// - Atomic operations (volatile doesn't guarantee atomicity)
// - Memory barriers (volatile doesn't prevent CPU reordering on multi-core)
// - Mutexes/critical sections (volatile doesn't provide mutual exclusion)

// WRONG: volatile pointer vs pointer to volatile
volatile uint32_t *reg = (volatile uint32_t*)0x40001000;  // pointer to volatile (CORRECT)
uint32_t *volatile ptr;  // volatile pointer to non-volatile (rarely what you want)
```

--------------------------------

### Compiler-Specific Optimization Flags

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Optimization levels like `-O2` or `-Os` can break code relying on undefined behavior, particularly integer overflow. Flags like `-fno-strict-aliasing` are common in embedded development. Be aware of differing compiler handling of packed structs and the non-standard nature of `#pragma once`.

```c
// -O2 or -Os may break code that depends on UB (especially integer overflow checks)
// -fno-strict-aliasing is common in embedded (Linux kernel uses it)
// Different compilers handle packed structs differently (padding, alignment)
// #pragma once vs include guards: #pragma once not guaranteed by standard (but widely supported)
```

--------------------------------

### Null Pointer Dereference (UB)

Source: https://github.com/ylongw/embedded-review/blob/main/references/c-pitfalls.md

Dereferencing a null pointer is undefined behavior. This can lead to a crash or, on some microcontrollers, silently read from address 0, which might be valid (e.g., the vector table).

```c
int *p = NULL;
int x = *p;  // UB — may crash, may silently read address 0 (which is valid on some MCUs!)
```
Tokens:
Raw