Added the ability to send ArtSync message in the Art-Net protocol implementation. #27
Conversation
WalkthroughThe Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- stupidArtnet/StupidArtnet.py (13 hunks)
Additional comments: 4
stupidArtnet/StupidArtnet.py (4)
- 47-47: Initialization of
artsync_headeris correct but ensure its usage aligns with the intended ArtSync functionality.- 65-67: Calling
make_artdmx_headerandmake_artsync_headerin the constructor ensures both headers are prepared upon object instantiation. Verify that this aligns with the intended use cases for ArtSync functionality.- 134-150: The
make_artsync_headermethod correctly initializes the ArtSync header according to the Art-Net protocol specifications. Ensure that the OpCode0x52is the correct value for ArtSync packets.- 387-389: Sending an ArtSync packet using
send_artsyncat the end of the main block is a practical demonstration of the new functionality. Ensure that this is intended for example purposes and not part of the production code.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/run tests.yml (1 hunks)
- stupidArtnet/StupidArtnet.py (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- stupidArtnet/StupidArtnet.py
Additional comments: 1
.github/workflows/run tests.yml (1)
- 10-13: Verify the project's dependencies for compatibility with Python 3.11.8 and ensure
actions/setup-python@v1supports this Python version.
There was a problem hiding this comment.
Hi @kacper516 , thank you for this.
In your proposal, the user would have to imperatively emit the ArtSync packets while the ArtDmx packet sending is automated by the clock thread.
I wonder whether this process should be managed by the library rather than by the user. But I dont have experience in this matter.
Few suggestions:
Would it make sense that the internal automation sends the ArtSync packets?
Would we like to have an opt-in process for ArtSync, perhaps an option in the constructor?
From what I understand, artDmx packets are buffered until a sync command is shown, so we would need to keep the ArtSync at the desired refresh rate. Is this correct?
|
In case we want to utilize ArtSync, we send it after each buffer transmission (sometimes we want to send a lot of buffers before artsync, what i tell about below). For the ArtNet data transmission, we could consider adding an option to send ArtSync directly in the constructor. This way, we determine whether we will send synchronization data or not. When setting up multiple ArtNet servers, for example, a list consisting of 20 ArtNet servers, each sending data to different universes but sequentially, I would like to synchronize the data only after iterating through all 20 elements. At that point, it constitutes one frame of animation for me. Then, we send the synchronization signal. For this purpose, I utilize a separate instance responsible for sending the buffer to a specific address. Based on this, I'm introducing a commit that enables sending the synchronization signal after each packet transmission (by default, this option is set to false in the constructor). Additionally, I'm allowing direct invocation of the function via the class instance, as this might be useful in certain scenarios (at least in my case). |
|
This all sounds sensible to me, thank you for your work. I will prepare a release, meanwhile, do you think I could ask you for some feedback on this unresolved PR? |
Changes:
Purpose:
Introducing the capability to send ArtSync messages enables synchronization of Art-Net receiving devices, which is essential for ensuring synchronized lighting effects on stage or in other environments.
Summary by CodeRabbit