Skip to content
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.

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

feat: add ArtNzs opcode #106

Merged
merged 2 commits into from
Jun 19, 2024
Merged

feat: add ArtNzs opcode #106

merged 2 commits into from
Jun 19, 2024

Conversation

japhsc
Copy link
Contributor

@japhsc japhsc commented Jun 16, 2024

Hey! Your library is amazing! I am using it for a small ESP32 project on mine. It uses ArtNzs to send frames to the memory of my micro controller. I've added support to send and receive ArtNzs packages to your library.

I intentionally skipped the sequence incrementing part (I use Sequence and StartCode to encode a uint16_t timestamp).

Let me know if you like something to be changed :)

@hideakitai
Copy link
Owner

Hi, I don't know what ArtNzs is. Can you explain it to me?

@hideakitai
Copy link
Owner

Ah, opcode? I will check it later

@hideakitai
Copy link
Owner

@japhsc Thank you for your contribution! It looks almost good to me, but I couldn't understand why you removed the sequence incrementing part. Could you explain in more detail?

I think it should be incremented because almost everyone who uses this library expects that sequence will be incremented.

@japhsc
Copy link
Contributor Author

japhsc commented Jun 18, 2024

Hey @hideakitai!

Sorry for my late reply, I was held up by my day job here in Berlin.

The background of my project: I was looking for a way to transfer DMX packages to the memory of my ESP32, including a 16bit timestamp. My plan was to later replay this sequence from memory when sending an ArtTrigger packet with KeyShow and memory index as SubKey.

When reading through the documentation for the ArtNet 4 protocol, I found the ArtNzs package (on page 66). It is a DMX512 data packet with non-zero start code (thus Nzs). Compared to ArtDmx it has a field called StartCode with 8 bit, instead of Physical. And on top, the packet is also specific to a universe.

So I decided to "reuse" this packet type to fit my needs and also stay as close as possible to the ArtNet standard. After all, tt is a non-zero start of the packet :D
Btw: the memory index (or better slot) where I am writing to is selected via a prior ArtTrigger with Key>4.

In my special case I am bunching the Sequence and StartCode field together to form my 16bit timestamp. That's why I didn't bother to implement the auto-increment of the Sequence filed. But it is no problem to do it. If you like, I would add two sequences -map attributes to the class instead of one: sequences_dmx & sequences_nzs and increment them accordingly.

If you think it is a good idea, I am planning on adding additional ArtNet packet callbacks, namely ArtIpProg and ArtAddress for configuration of the IP address / DHCP and the universe.

Let me know what you think :)

@hideakitai
Copy link
Owner

hideakitai commented Jun 18, 2024

Thank you! I could understand why you haven't incremented the sequence. But I think it should be increased for general users (your implementation is "hackey" for your purpose). Please increment the sequence.

If you like, I would add two sequences -map attributes to the class instead of one: sequences_dmx & sequences_nzs and increment them accordingly.

Yes, it looks good to me.

If you think it is a good idea, I am planning on adding additional ArtNet packet callbacks, namely ArtIpProg and ArtAddress for configuration of the IP address / DHCP and the universe.

It sounds great :) If you can contribute, please open another two PRs for each opcode.

@japhsc
Copy link
Contributor Author

japhsc commented Jun 19, 2024

New changes are pushed and nzs_sequences & dmx_sequences are added :)

Copy link
Owner

@hideakitai hideakitai left a comment

Choose a reason for hiding this comment

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

LGTM

Artnet/Sender.h Outdated Show resolved Hide resolved
@hideakitai
Copy link
Owner

Ah, sorry, I missed SequenceMap, which was not declared in the art_nzs namespace. Anyway, I will merge this PR and fix it in my next PR. Thank you for your contribution, and I am looking forward to the PR for ArtIpProg and ArtAddress :)

@hideakitai hideakitai merged commit bbca52d into hideakitai:main Jun 19, 2024
0 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants