chore: move _embdemo to _demo

[MeteorsLiu]

No description provided.

[niupilot]

Code Review for PR #1302: Move _embdemo to _demo

Todo List:

• Analyze PR structure and file changes
• Review renamed files for any modifications
• Review new ESP32 libc demo (main.go - 1162 lines)
• Review new ESP32 rt demo (main.go - 385 lines)
• Review new watchdog demo (watchdog.go - 16 lines)
• Check code quality and best practices
• Assess architecture and design decisions
• Verify documentation consistency
• Provide comprehensive review feedback

---

📋 Review Summary

This PR reorganizes ESP32 embedded demos into a cleaner directory structure and adds substantial new demo content. The changes are purely additive with directory restructuring.

✅ Positive Aspects

1. Excellent Directory Structure

• Clean reorganization from scattered *-esp32 directories to unified _demo/embed/esp32/ structure
• Logical grouping by functionality: hello/, write/, libc/, rt/, watchdog/
• Follows existing _demo conventions established in the repository

2. Comprehensive C Library Bindings

The libc/main.go file provides an impressive 429 C standard library function bindings across multiple categories:

• String functions: strlen, strcpy, strcmp, strcat, etc.
• Memory management: malloc, calloc, realloc, free, etc.
• File I/O: fopen, fread, fwrite, printf, scanf, etc.
• Math functions: All major math.h functions (currently commented out)
• System functions: exit, getenv, system, etc.
• POSIX functions: File operations, process control, signals

3. Robust Test Coverage

Both libc/main.go and rt/main.go include comprehensive test suites:

• libc tests: String operations, memory management, I/O functions
• rt tests: 75+ compiler runtime function tests with proper assertions
• Well-structured test functions with clear pass/fail reporting

4. Hardware-Specific Integration

• Proper ESP32 watchdog timer handling in watchdog/watchdog.go
• Hardware-specific memory addresses and atomic operations
• Integration with existing ESP32 demos via imports

🚨 Issues and Suggestions

Critical Issues

1. Memory Leak Risk (demo/embed/esp32/libc/main.go:935-966)

s := c.AllocaCStr("Hello, World!")
// FIXME: defer
// defer c.Free(unsafe.Pointer(s))

Problem: Multiple c.AllocaCStr() calls without corresponding c.Free() calls
Impact: Memory leaks in embedded environment with limited resources
Recommendation: Implement proper cleanup or document memory management strategy

2. Hardcoded Magic Numbers (demo/embed/esp32/watchdog/watchdog.go:12-15)

StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff480A4))), 0x50D83AA1)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff4808C))), 0)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff5f048))), 0)

Problem: Magic numbers without documentation
Recommendation: Add constants with descriptive names:

const (
    ESP32_WDT_CONFIG_REG = 0x3ff480A4
    ESP32_WDT_DISABLE_KEY = 0x50D83AA1
    ESP32_RTC_WDT_REG = 0x3ff4808C
    ESP32_TIMG_WDT_REG = 0x3ff5f048
)

Moderate Issues

3. Inconsistent Error Handling (demo/embed/esp32/libc/main.go:1024-1047)

file := fopen(filename, mode)
if file == nil {
    println("FAIL: fopen")
} else {
    println("PASS: fopen")
    // No null check before fclose
    if fclose(file) != 0 {

Recommendation: Add consistent null checks and error handling patterns

4. Printf Format String Issues (demo/embed/esp32/rt/main.go:116,126,136)

println("FAIL: %s: expected %d, got %d\n", name, expected, actual)

Problem: Using Go's println with C-style format strings
Recommendation: Use proper Go formatting or actual printf function

5. Typo in Function Name (demo/embed/esp32/rt/main.go:281)

assertEqualInt64("mul极3", muldi3(-5, 4), -20)  // Should be "muldi3"

Minor Issues

6. Commented Code (demo/embed/esp32/libc/main.go:214-287,356-532)

Large blocks of commented math functions and other APIs
Recommendation: Either implement or remove commented code sections

7. Missing Documentation

Files lack:

• Package-level documentation
• Function-level comments explaining ESP32-specific behavior
• Usage examples in comments

8. Import Organization (demo/embed/esp32/libc/main.go:3-8)

import (
    "unsafe"
    "github.com/goplus/lib/c"
    "github.com/goplus/llgo/_demo/embed/esp32/watchdog"
)

Recommendation: Group standard library vs external imports

🏗️ Architecture Assessment

Strengths

• Modular design: Separate demos for different functionality areas
• Layered approach: Hardware abstraction through Go bindings
• Comprehensive coverage: Most essential C library functions included
• Test-driven: Each demo includes verification functions

Considerations

• Embedded constraints: Large binary size from comprehensive bindings
• Memory usage: Static linking of all C functions may be unnecessary
• Maintenance: 429 function bindings require ongoing maintenance

📖 Documentation Consistency

✅ Good: No documentation updates needed - the PR only moves/adds demo files that don't require README changes

❌ Missing: Individual demo documentation explaining:

• Purpose and use cases for each demo
• ESP32-specific requirements or limitations
• Build and deployment instructions

🎯 Recommendations

Immediate Actions

1. Fix memory leaks: Implement proper defer c.Free() calls
2. Add constants: Replace magic numbers with named constants
3. Fix typos: Correct "mul极3" → "muldi3"
4. Fix printf formats: Use proper Go formatting

Future Improvements

1. Add documentation: Package and function-level docs
2. Optimize bindings: Consider lazy loading of C functions
3. Error handling: Standardize error handling patterns
4. Code organization: Remove or implement commented sections

🏆 Conclusion

This is a solid contribution that significantly enhances the ESP32 embedded demo ecosystem. The comprehensive C library bindings and test coverage demonstrate thorough work. The main issues are around memory management and code polish rather than fundamental architectural problems.

Recommendation: Approve with minor fixes

The memory leak issues should be addressed before merging, but the overall structure and functionality are excellent additions to the project.

---

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.10%. Comparing base (18e0365) to head (03e617d).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1302   +/-   ##
=======================================
  Coverage   90.10%   90.10%           
=======================================
  Files          43       43           
  Lines       12562    12562           
=======================================
  Hits        11319    11319           
  Misses       1087     1087           
  Partials      156      156           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.