Feature/light#355
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…into feature/light
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds LED strip support: new Led class and NeoPixel stub, integrates LEDs into Robot with control methods, a light mod wiring buttons to LED actions, manifests/tsconfig updates, and a test demonstrating LED effects. Changes
Sequence DiagramsequenceDiagram
participant Button as Button Input
participant Mod as Light Mod
participant Robot as Robot
participant Led as Led Instance
participant NeoPixel as NeoPixel Driver
Button->>Mod: press button (a/b/c)
Mod->>Robot: call lightOn/Off/Blink/Rainbow(ledName, ...)
Robot->>Led: delegate method (on/off/blink/rainbow,...)
Led->>Led: stop existing effects (if any)
Led->>NeoPixel: fill/set pixels & update()
loop effect ticks
Led->>NeoPixel: update frame
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Moddable 7.0.0 で型定義が入り、ビルドが通るようになったので、ドラフトを外します |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@firmware/stackchan/led/led.ts`:
- Around line 110-121: The blink method is passing an incorrect count to the
Blink effect (it sets count: _index + _count which expands the range); update
the Blink instantiation in blink(r,g,b,duration,index?,count?) to pass count:
_count (keeping index: _index) so Blink receives the intended size, and ensure
the default calculation const _count = count ?? this.length - _index remains
unchanged.
- Around line 88-99: The auto-off Timer created in on() can leave a stale handle
that later calls off() unexpectedly; modify on() to save the Timer.set() return
value into a field (e.g., this._autoOffTimer) and ensure _stopEffect() clears
that timer (Timer.clear(this._autoOffTimer) or equivalent) before scheduling a
new Timer; when scheduling in on() assign the handle to this._autoOffTimer so
subsequent calls cancel the previous callback before creating a new one and
avoid unexpected shutdowns.
In `@firmware/stackchan/robot.ts`:
- Line 8: The code uses InstanceType<typeof Led> with a type-only import, which
breaks TS because typeof needs a runtime value; replace all occurrences of
InstanceType<typeof Led> in robot.ts with a plain Led type or Record<string,
Led> (e.g., use Record<string, Led> for maps of LEDs and Led for single
instances) and keep the import as a type-only import (import type Led) so the
type resolves correctly; ensure every place referencing InstanceType<typeof Led>
(the import area and the other two usages) is updated consistently.
🧹 Nitpick comments (3)
firmware/stackchan/led/manifest_led.json (1)
11-25: Consider consolidating duplicate platform configurations.The
mac,lin, andwinplatform sections are identical. If the Moddable manifest format supports platform grouping or wildcards, this could reduce duplication.That said, the current explicit approach is clear and works correctly.
firmware/tests/led/main.ts (1)
18-19: Minor timing observation: LED auto-off before wait completes.The
led.on(255, 255, 255, 1000)will automatically turn off after 1 second, butasyncWait(2000)waits 2 seconds. This means the LED will be off for the last second of the wait period. If this is intentional for visual demonstration purposes, consider adding a brief comment for clarity.firmware/mods/light/mod.js (1)
12-30: Consider adding null checks for buttons.Looking at
firmware/stackchan/default-mods/on-robot-created.ts, button handlers are guarded with null checks likeif (robot.button.a != null). This prevents errors on devices without all three buttons.♻️ Suggested defensive pattern
- robot.button.a.onChanged = function () { + if (robot.button.a != null) { + robot.button.a.onChanged = function () { if (this.read()) { const firstColor = colors.shift() colors.push(firstColor) robot.lightOn('a', firstColor.r, firstColor.g, firstColor.b) } } + }Apply similarly to buttons
bandc.
| on(r: number, g: number, b: number, duration?: number, index?: number, count?: number) { | ||
| const _index = index ?? 0 | ||
| const _count = count ?? this.length - _index | ||
| this._stopEffect() | ||
| this._fill(this.makeRGB(r, g, b), _index, _count) | ||
|
|
||
| this.update() | ||
| if (duration) { | ||
| Timer.set(() => { | ||
| this.off(_index, _count) | ||
| }, duration) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the LED file
find . -path "*/firmware/stackchan/led/led.ts" -type fRepository: stack-chan/stack-chan
Length of output: 96
🏁 Script executed:
# Also try to locate it with fd
fd -t f "led.ts" firmware/Repository: stack-chan/stack-chan
Length of output: 94
🏁 Script executed:
# Let's check the repository structure
git ls-files | grep -E "firmware.*led" | head -20Repository: stack-chan/stack-chan
Length of output: 235
🏁 Script executed:
# Read the LED file to see the full context
cat -n ./firmware/stackchan/led/led.tsRepository: stack-chan/stack-chan
Length of output: 4661
🏁 Script executed:
# Check for Timer.clear usage in the codebase
rg "Timer\.clear" --type ts --type jsRepository: stack-chan/stack-chan
Length of output: 878
🏁 Script executed:
# Check for Timer imports and usage patterns
rg "Timer\." --type ts --type js -A 2 | head -50Repository: stack-chan/stack-chan
Length of output: 2446
🏁 Script executed:
# Look for type definitions or d.ts files related to Timer
find . -name "*.d.ts" -o -name "*timer*" -type f | grep -v node_modules | head -20Repository: stack-chan/stack-chan
Length of output: 387
🏁 Script executed:
# Check one of the examples of Timer.clear usage to understand the pattern
cat -n firmware/stackchan/drivers/dynamixel-driver.ts | grep -A 3 -B 3 "Timer.clear"Repository: stack-chan/stack-chan
Length of output: 269
🏁 Script executed:
# Check the preference-server example
cat -n firmware/stackchan/services/preference-server.ts | grep -A 3 -B 3 "Timer.clear"Repository: stack-chan/stack-chan
Length of output: 273
🏁 Script executed:
# Check if Timer type definitions exist
find . -path "*/typings*" -name "*.d.ts" -exec grep -l "Timer" {} \;Repository: stack-chan/stack-chan
Length of output: 47
🏁 Script executed:
# Check how _stopEffect is called across the class
rg "_stopEffect" firmware/stackchan/led/led.ts -B 2 -A 2Repository: stack-chan/stack-chan
Length of output: 813
🏁 Script executed:
# Verify the pattern - check if there are other similar timer clearing patterns
rg "#.*Timer.*=" firmware/stackchan/drivers/dynamixel-driver.ts -B 1 -A 1Repository: stack-chan/stack-chan
Length of output: 47
🏁 Script executed:
# Look at how other files store timer handles
rg "private.*Timer" firmware/stackchan/drivers/ -A 1 | head -30Repository: stack-chan/stack-chan
Length of output: 47
🏁 Script executed:
# Check a complete example of timer handle management
cat -n firmware/stackchan/drivers/sg90-driver.ts | grep -A 5 -B 5 "_driveHandler"Repository: stack-chan/stack-chan
Length of output: 1412
Cancel stale auto-off timers to prevent unexpected shutdowns.
When on() is called with a duration before a previous timer fires, the old callback still executes and turns off the LEDs unexpectedly. Store the timer handle and clear it in _stopEffect() before scheduling a new one.
🛠️ Proposed fix
export default class Led extends NeoStrand {
private _effect?: NeoStrandEffect
+ private _offTimer?: ReturnType<typeof Timer.set>
@@
private _stopEffect() {
if (this._effect) {
this.stop()
this._effect = undefined
}
+ if (this._offTimer) {
+ Timer.clear(this._offTimer)
+ this._offTimer = undefined
+ }
}
@@
- if (duration) {
- Timer.set(() => {
- this.off(_index, _count)
- }, duration)
- }
+ if (duration) {
+ this._offTimer = Timer.set(() => {
+ this.off(_index, _count)
+ }, duration)
+ }🤖 Prompt for AI Agents
In `@firmware/stackchan/led/led.ts` around lines 88 - 99, The auto-off Timer
created in on() can leave a stale handle that later calls off() unexpectedly;
modify on() to save the Timer.set() return value into a field (e.g.,
this._autoOffTimer) and ensure _stopEffect() clears that timer
(Timer.clear(this._autoOffTimer) or equivalent) before scheduling a new Timer;
when scheduling in on() assign the handle to this._autoOffTimer so subsequent
calls cancel the previous callback before creating a new one and avoid
unexpected shutdowns.
meganetaaan
left a comment
meganetaaan
left a comment
There was a problem hiding this comment.
Nekomimi LEDを使って動作確認しました。LGTMです 🙆
Feature/light
2 participants
概要
M5GO bottom付属LEDやNekomimi LEDといったNeoPixel互換LEDに対応します
ModdableSDK#1549 での型定義が必要となるので、一旦ドラフトになります。
使用方法
manifestファイルのの
configセクションに以下のようにLEDの定義を追加することで、robot.ledでLedクラスのインスタンスにアクセスできます。また、robot自身にlightOnなどの関数を追加しています。使用例は、
mods/lightやtests/ledが参考になります。Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.