Skip to content

demo:button demo #30

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
luoliwoshang wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −0
Open

demo:button demo #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions _demo/button/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package main
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot Dec 1, 2025

Choose a reason for hiding this comment

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

Documentation: Missing package documentation

Add documentation explaining:

  • Target hardware (ESP32)
  • What the demo demonstrates
  • Hardware setup requirements
  • Why watchdog timers are disabled

Suggestion:

// This demo demonstrates reading button input on ESP32-based boards.
// It continuously polls GPIO34 for button state and prints the result.
//
// Hardware requirements:
//   - ESP32 board (tested on M5Stack)
//   - Button connected to GPIO34 (or use built-in button)
//
// Note: This demo disables watchdog timers to prevent resets during execution.
//
//go:build esp32

package main


import (
"unsafe"

"github.com/goplus/emb/machine"
)

//go:linkname StoreUint32 llgo.atomicStore
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot Dec 1, 2025

Choose a reason for hiding this comment

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

Documentation: Undocumented go:linkname directives

These advanced features need explanation for demo/educational purposes:

// StoreUint32 performs an atomic store to a memory address.
// Linked to llgo's atomic store for bare-metal operations.
//
//go:linkname StoreUint32 llgo.atomicStore
func StoreUint32(addr *uint32, val uint32)

// sleep pauses execution for n seconds using llgo runtime.
//
//go:linkname sleep sleep
func sleep(n int)

Note: The StoreUint32 functionality already exists in runtime/volatile/volatile.go and could be imported instead.

func StoreUint32(addr *uint32, val uint32)

//go:linkname sleep sleep
func sleep(n int)

func main() {
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff480A4))), 0x50D83AA1)
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot Dec 1, 2025

Choose a reason for hiding this comment

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

Critical: Undocumented magic numbers and hardware addresses

These hard-coded register addresses and magic values make the code unmaintainable and potentially unsafe:

  • 0x3ff480A4 = RTC_CNTL.WDTWPROTECT register
  • 0x50D83AA1 = Watchdog unlock key
  • 0x3ff4808C = RTC_CNTL.WDTCONFIG0 register
  • 0x3ff5f048 = TIMG0.WDTCONFIG0 register

Issues:

  1. No documentation explaining these disable watchdog timers
  2. Users can't understand or safely modify this code
  3. The project already has proper abstractions in device/esp/esp32.go that should be used

Recommendation:

// Disable ESP32 watchdog timers to prevent automatic resets
const (
    RTC_CNTL_WDTWPROTECT_REG = 0x3ff480A4
    RTC_CNTL_WDT_WKEY = 0x50D83AA1  // Unlock key
    RTC_CNTL_WDTCONFIG0_REG = 0x3ff4808C
    TIMG0_WDTCONFIG0_REG = 0x3ff5f048
)

// Unlock and disable RTC watchdog
StoreUint32((*uint32)(unsafe.Pointer(uintptr(RTC_CNTL_WDTWPROTECT_REG))), RTC_CNTL_WDT_WKEY)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(RTC_CNTL_WDTCONFIG0_REG))), 0)

// Disable Timer Group 0 watchdog
StoreUint32((*uint32)(unsafe.Pointer(uintptr(TIMG0_WDTCONFIG0_REG))), 0)

Or better yet, use the existing device package abstractions.

StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff4808C))), 0)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff5f048))), 0)
Comment on lines +16 to +18
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Dec 1, 2025

Choose a reason for hiding this comment

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

high

The code uses magic numbers for memory addresses and values to disable watchdog timers. This makes the code hard to read, understand, and maintain. These magic numbers should be replaced with named constants that explain their purpose. This will greatly improve code clarity and make it easier to verify the register manipulation logic.

Please define these constants at the package level, for example, before the main function:

const (
	// RTC Watchdog Timer write protection
	wdtRTCProtectReg = 0x3ff4808C

	// RTC Watchdog Timer configuration
	wdtRTCConfig0Reg = 0x3ff480A4
	wdtRTCKeyValue   = 0x50D83AA1 // Key to unlock RTC WDT registers

	// Timer Group 0 Watchdog Timer write protection
	wdtT0ProtectReg = 0x3ff5f048
)
Suggested change
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff480A4))), 0x50D83AA1)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff4808C))), 0)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(0x3ff5f048))), 0)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(wdtRTCConfig0Reg))), wdtRTCKeyValue)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(wdtRTCProtectReg))), 0)
StoreUint32((*uint32)(unsafe.Pointer(uintptr(wdtT0ProtectReg))), 0)

buttonPin := machine.GPIO34
buttonPin.Configure(machine.PinConfig{Mode: machine.PinInput})

for {
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot Dec 1, 2025

Choose a reason for hiding this comment

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

Performance: Excessive I/O operations

The code prints to UART on every loop iteration (every second), regardless of whether the button state changed. This wastes CPU cycles and power.

Recommendation: Only print on state changes:

var lastState bool
for {
    currentState := buttonPin.Get()
    if currentState != lastState {
        if currentState {
            println("Button: HIGH")
        } else {
            println("Button: LOW")
        }
        lastState = currentState
    }
    sleep(1)
}

This reduces unnecessary I/O operations by ~99% when the button is in a stable state.

if buttonPin.Get() {
println("yes")
} else {
println("no")
}
sleep(1)
}
}