More tests and validated Port-Address type

Firionus commented 4 years ago

Sorry, I couldn't control myself to put these things in different branches. Not sure if you want me to un-tangle or just merge it together. Don't have that much experience with Git(Hub).

I changed subnet to port_address with the PortAddress datatype. PortAddress is validated upon creation to be in the valid range of 0 to 32_767. It can be created with the std traits From, TryFrom, Into and TryInto. I also implemented Convertable.
I added the simple regression tests create_single_dmx_value_art_dmx_packet and create_512_dmx_values_art_dmx_packet. They test Output packet creation.
I added two failing tests (parse_invalid_lengths and data_longer_than_length). They show bugs in the parsing with strange length properties.

I started wondering why we would save length at all. When someone creates a packet, the length field should be automatically created upon serialization. When someone receives a packet, the length field is redundant with data.len() and does not need to be known after parsing.

VictorKoenders commented 4 years ago

I started wondering why we would save length at all

The reason I added the length field is because I wanted the data structures to be a 1-on-1 match with the documentation. I have no issue with removing the field if we can replace it with a name that clearly indicates that the 2 fields in the documentation (length and data) match the 1 field in the code.

Firionus commented 4 years ago

The failing tests mostly relate to #8. I tried to see whether your rewrite works and as you can see there are still issues with length in parsing. I refactored the tests so you can see more clearly from their names what's going wrong. Parsing can currently happen with any length value, when in fact the length value should be validated upon parsing and determine how many bytes of data are read in. Also, ProtVer should be validated during parsing to be below 15 as well (and during serialization should not be set by the user when this packet only supports v14). These issues result in the failing tests.

Not sure how to proceed - your crate, your design. I think an Art-Net crate should do all validation for me and only allow me to serialize standard-conform packets. It should basically abstract away all protocol related things. This requires even more changes. And isn't there an accepted framework for serialization/de-serialization with serde? What are your thoughts?

VictorKoenders commented 4 years ago

I don't think serde is suitable for this, for one it takes a lot of lines of code to implement, and I think it would be overkill for just serializing some structs.

I think a crate like this can go one of two ways

Split it into 2 crates, one that is the raw protocol, and one that is a safe abstraction layer on top of it. This way people can use the raw crate if they want/have to
Keep a single crate and make a safe abstraction, but lose the direct connection between the protocol and what the programmer uses.

I haven't used this crate in almost two years, I think people that use this crate should decide on what they want this to be

VictorKoenders commented 4 years ago

Looking at the parsing failures, I agree with you that they are hard to solve without completely rewriting the crate. I'm not interested in putting in too much effort in this crate right now as I'm not using it myself.

If someone runs into parsing issues and wants to make a pull request to add in proper validation they are more than welcome to, but for now I'm just going to merge this, take out the failing parsing validation, and merge this into the master branch.

VictorKoenders commented 4 years ago

Opened #12 for the parsing command issue

Firionus commented 4 years ago

I think a crate like this can go one of two ways
* Split it into 2 crates, one that is the raw protocol, and one that is a safe abstraction layer on top of it. This way people can use the raw crate if they want/have to

* Keep a single crate and make a safe abstraction, but lose the direct connection between the protocol and what the programmer uses.
I haven't used this crate in almost two years, I think people that use this crate should decide on what they want this to be

Thank you for your work and thoughts 👍

For my project I'll continue with my own crate and come back here once I need to implement ArtPoll/ArtPollReply. Serializing ArtDmx packets is barely 20 lines of code and I'll avoid the dependency for now.

I'll also say I favor the single crate, safe abstraction approach. An attractive crate would probably abstract even higher and include support for sACN (which is compatible with Art-Net 4). Of course, being a competitor to OLA isn't ideal, but Rust hast its charms...

Trangar / artnet_protocol

More tests and validated Port-Address type #10