add led #2
add led #2
Conversation
Summary of ChangesHello @stc1988, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive support for addressable LEDs within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for controlling LEDs, which is a great new feature. The implementation includes a new Led class to manage effects like on, off, blink, and rainbow, along with integration into the main Robot class. My review focuses on improving the robustness and correctness of the new Led class by ensuring that different effects don't conflict with each other. I've also pointed out a couple of areas where type safety and documentation can be improved.
| on(r: number, g: number, b: number, duration?: number, index?: number, count?: number) { | ||
| const _index = index ?? 0 | ||
| const _count = count ?? this.length - _index | ||
| this._fill(this.makeRGB(r, g, b), _index, _count) | ||
|
|
||
| this.update() | ||
| if (duration) { | ||
| Timer.set(() => { | ||
| this._fill(this.makeRGB(0, 0, 0), _index, _count) | ||
| this.update() | ||
| }, duration) | ||
| } | ||
| } |
There was a problem hiding this comment.
The on() method does not stop currently running effects like blink() or rainbow(). This can lead to unexpected behavior where the LED color is immediately overridden by the active effect's timer. To ensure on() sets a static color, you should stop all other effects at the beginning of this method. Consider extracting this cleanup logic into a private helper method to be reused across on, blink, and rainbow to avoid code duplication.
on(r: number, g: number, b: number, duration?: number, index?: number, count?: number) {
this._blinking = false
if (this._blinkTimer !== undefined) {
Timer.clear(this._blinkTimer)
this._blinkTimer = undefined
}
if (this._effect) {
this.stop()
this._effect = undefined
}
const _index = index ?? 0
const _count = count ?? this.length - _index
this._fill(this.makeRGB(r, g, b), _index, _count)
this.update()
if (duration) {
Timer.set(() => {
this._fill(this.makeRGB(0, 0, 0), _index, _count)
this.update()
}, duration)
}
}
firmware/stackchan/led/led.ts
Outdated
| blink(r: number, g: number, b: number, interval: number, index?: number, count?: number) { | ||
| const _index = index ?? 0 | ||
| const _count = count ?? this.length - _index | ||
| if (this._blinking) return | ||
| this._blinking = true | ||
| this._blinkState = false | ||
|
|
||
| const step = () => { | ||
| if (!this._blinking) return | ||
| this._blinkState = !this._blinkState | ||
| if (this._blinkState) { | ||
| this._fill(this.makeRGB(r, g, b), _index, _count) | ||
| } else { | ||
| this._fill(this.makeRGB(0, 0, 0), _index, _count) | ||
| } | ||
| this.update() | ||
| } | ||
| this._blinkTimer = Timer.repeat(step, interval) | ||
| } |
There was a problem hiding this comment.
The blink() method should cancel any other running effects (like rainbow) when called. Additionally, the if (this._blinking) return prevents updating an existing blink with new parameters. It's better to stop the current blink and any other effects, then start a new one. This ensures predictable behavior and allows for dynamic changes to the blink effect.
blink(r: number, g: number, b: number, interval: number, index?: number, count?: number) {
this._blinking = false
if (this._blinkTimer !== undefined) {
Timer.clear(this._blinkTimer)
this._blinkTimer = undefined
}
if (this._effect) {
this.stop()
this._effect = undefined
}
const _index = index ?? 0
const _count = count ?? this.length - _index
this._blinking = true
this._blinkState = false
const step = () => {
if (!this._blinking) return
this._blinkState = !this._blinkState
if (this._blinkState) {
this._fill(this.makeRGB(r, g, b), _index, _count)
} else {
this._fill(this.makeRGB(0, 0, 0), _index, _count)
}
this.update()
}
this._blinkTimer = Timer.repeat(step, interval)
}| rainbow(index?: number, count?: number) { | ||
| const _index = index ?? 0 | ||
| const _count = count ?? this.length - _index | ||
| if (this._effect) return | ||
|
|
||
| this._effect = new NeoStrand.HueSpan({ strand: this, start: _index, end: _count }) | ||
| this.setScheme([this._effect]) | ||
| this.start(50) | ||
| } |
There was a problem hiding this comment.
Similar to the other effect methods, rainbow() should cancel any previously running effects like blink(). The if (this._effect) return also prevents restarting the effect. It's better to always clear previous state before starting a new effect.
rainbow(index?: number, count?: number) {
this._blinking = false
if (this._blinkTimer !== undefined) {
Timer.clear(this._blinkTimer)
this._blinkTimer = undefined
}
if (this._effect) {
this.stop()
this._effect = undefined
}
const _index = index ?? 0
const _count = count ?? this.length - _index
this._effect = new NeoStrand.HueSpan({ strand: this, start: _index, end: _count })
this.setScheme([this._effect])
this.start(50)
}| const led = Object.fromEntries( | ||
| Object.entries(configLed).map(([key, config]) => [ | ||
| key, | ||
| new Led(config as { pin: number; length?: number; order?: string }), |
There was a problem hiding this comment.
Using a type assertion with as bypasses type-checking and can hide potential bugs if the config object from preferences doesn't match the expected shape. This could lead to runtime errors. For better type safety, consider adding a type guard function to validate the config object's structure before creating a new Led instance. This would make your code more robust against invalid configurations.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…into feature/light
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for controlling LEDs, including effects like blinking and rainbow. The implementation is well-structured within a new led module and integrated into the existing Robot class. I've identified a critical bug in the rainbow effect, a high-severity performance issue in the blink effect, and a minor typing issue in a stub file. Addressing these points will significantly improve the correctness and performance of this new feature.
| const newColor = value < half ? this.rgbOn : this.rgbOff | ||
| const currentColor = this.strand.getPixel(this.start) | ||
| if (newColor !== currentColor) { | ||
| for (let i = this.start; i < this.end; i++) this.strand.set(i, newColor, this.start, this.end) |
There was a problem hiding this comment.
The current implementation for setting pixel colors within the Blink effect is inefficient and contains a bug. The for loop calls this.strand.set() on each iteration, which in neostrand triggers an update of the LED strand. This leads to multiple, unnecessary updates within a single animation frame. Furthermore, the last argument to set() is this.end (an index), where a count is expected.
A more efficient and correct approach is to use the fill() method to update the pixel buffer for the entire segment at once. The animation timeline will then handle calling update() once per frame.
| for (let i = this.start; i < this.end; i++) this.strand.set(i, newColor, this.start, this.end) | |
| this.strand.fill(newColor, this.start, this.size) |
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>
No description provided.