Fix persistent sending #34
Conversation
WalkthroughThe pull request introduces a modification to the Changes
Sequence DiagramsequenceDiagram
participant Thread
participant StupidArtnet
StupidArtnet->>Thread: start()
alt Running State
Thread-->>Thread: Sleep
Thread-->>Thread: Initialize Thread
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
stupidArtnet/StupidArtnet.py (2)
195-199: Consider improving thread safety and adding documentation.While the fix works, there are some improvements to consider:
- Add documentation explaining the thread management logic
- Consider adding thread safety mechanisms to prevent multiple thread spawns
Here's a suggested implementation with better thread safety and documentation:
def start(self): - """Starts thread clock.""" + """Starts a persistent thread that sends DMX data at the specified FPS. + + The method ensures continuous sending by: + 1. Sending initial DMX data + 2. Initializing the running flag if not present + 3. Spawning a new thread if running + """ self.show() if not hasattr(self, "running"): + self._thread = None self.running = True if self.running: + if self._thread is None: sleep((1000.0 / self.fps) / 1000.0) - _thread.start_new_thread(self.start, ()) + self._thread = _thread.start_new_thread(self.start, ())
Line range hint
204-206: Update the stop method to handle thread cleanup.The
stopmethod should be updated to properly cleanup the thread reference.def stop(self): """Set flag so thread will exit.""" self.running = False + self._thread = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stupidArtnet/StupidArtnet.py(1 hunks)
🔇 Additional comments (1)
stupidArtnet/StupidArtnet.py (1)
197-199: LGTM! The change fixes the persistent sending issue.The modification from
eliftoifensures continuous sending by allowing the thread to persist after initialization. This appears to be the root cause fix for issue #31.
| @@ -194,7 +194,7 @@ def start(self): | |||
| self.show() | |||
| if not hasattr(self, "running"): | |||
| self.running = True | |||
| elif self.running: | |||
| if self.running: | |||
There was a problem hiding this comment.
question: would this code not always evaluate to true since it was set the line before?
There was a problem hiding this comment.
If self.running exists and is False (with stop function), it will not be set to True in the previous line.
So I think the condition is necessary to stop the thread.
See #31
Summary by CodeRabbit