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

Difficulty writing descriptors of unknown size #45

Open
electroCutie opened this issue Oct 17, 2020 · 6 comments
Open

Difficulty writing descriptors of unknown size #45

electroCutie opened this issue Oct 17, 2020 · 6 comments

Comments

@electroCutie
Copy link

I'm writing a MIDI descriptor builder for use in another project of mine and realize that it was fairly hard to write the descriptors because they need to know the length of all the MIDI specific headers to put into a Class Specific descriptors

The issue is that the sizes aren't fixed and depend on things such as the number of MIDI entities, the number of virtual "pins" in the topology, and the number of connections in the topology. Counting the number of bytes needed is error prone and making one's own buffer to build on before submitting it to this package is wasteful since there is already a backing buffer in place.

I propose that we make use of the is buffer and expose a lazy form of the writer that will allow descriptors to be written and, at the end, have lengths or other data filled in.

I also have a pull request that implements this idea as a proof of concept that I added to develop my MIDI USB Descriptor builder, so at the very least I can say the idea is sound and I hope it can serve as the basis for an implementation

@electroCutie
Copy link
Author

Here is an example of how I use the proposed interface in #46

writer.writer(|w| {
    let start_b6 = w.position();
    w.delayed_write(
        1, // outer writes the header size
        |w| {
            w.arr(&[
                constants::DescriptorType::ClassSpecificInterface as u8,
                constants::EndpointSubtype::Header as u8,
                // Revision, 1.0 BCD in Little-endian
                0x00,
                0x01,
            ]);
            //header is the length of that plus the two bytes that describe the total length
            let header_length_b6 = (w.position() - start_b6) + 2;

            // writes the rest and then the length of that into the header
            w.delayed_write(
                2,
                |w| {
                    for e in self.entities {
                        e.write_descriptor(w);
                    }

                    // this length includes the header, we reuse the start from above
                    (w.position() - start_b6) as u16
                },
                |slice, size| slice.copy_from_slice(&size.to_le_bytes()),
            );

            //the outer wrapper writes the header length
            header_length_b6
        },
        |slice, size| slice[0] = size as u8,
    );
})?;

@electroCutie
Copy link
Author

electroCutie commented Dec 2, 2020

Figured I'd ping this

Any thoughts or feelings on it or the PR?
@mvirkkunen

@agausmann
Copy link

agausmann commented Aug 6, 2021

usb-device already does something like this internally to write the number of endpoints in an interface. Because it doesn't know how many endpoints there are when the interface descriptor is written, It writes out a placeholder zero and remembers the position, and then every time DescriptorWriter.endpoint is called, it increments the byte at that position.

The API you propose seems complex and engineered for this specific usecase, but it's also solvable with a simpler and more generic interface: a way to retroactively write bytes in your descriptors.

Of course, if you just let the user overwrite any byte at any position, that's a recipe for trouble. But there's a more foolproof way to do it - add a method to DescriptorWriter that returns a typed handle to a position (or an offset from the current position of the writer). Then, later you can pass that hande back to the DescriptorWriter to write a byte there.

The benefit to the handle approach is that it is easily validated. For example, it could require that the handle points to a position greater than the writer's position when it is created, and also require that the writer's position has gone past the handle's position before the handle can be used to write. That would verify that the byte you are writing is within the bounds of the descriptor(s) you wrote between creating the handle and using it.

@electroCutie
Copy link
Author

I don't disagree that it was a bit complex. I'd want to take another crack at it these days. But there is one salient feature that it has that was valuable to me: it counted bytes for me. The idea was that it could write sizes.

I like the internal version of that, where it increments the byte every time a write takes place. I've got a notion... let me dust this off and see if I can do something that still preserves the byte-counting but in a way that acts more like the handles you proposed

@agausmann
Copy link

agausmann commented Aug 6, 2021

This is what I imagine it'd look like, for reference:

struct DeferredWrite(usize);

trait DescriptorWriter {
    /* ... */
	
    fn defer_ahead(&self, offset: usize) -> DeferredWrite {
        DeferredWrite(self.position() + offset)
    }

    fn get_deferred_mut(&mut self, handle: &DeferredWrite) -> Result<&mut u8> {
        if self.position() <= handle.0 {
            Err(/* ... */)
        } else {
            Ok(&mut self.buf[handle.0])
        }
    }
}

This could also be extended to make handles to ranges and return &mut [u8] slices, so you can write multi-byte values without managing multiple handles. Perhaps by making DeferredWrite generic over the index type, and using a subtrait of SliceIndex that adds a method for checking whether a given position is past the index.


But there is one salient feature that it has that was valuable to me: it counted bytes for me. The idea was that it could write sizes.

I'm not really interested in it counting bytes for me, it is easy enough to take the position at either end of the list of descriptors with writer.position() and write out the difference.

@stappersg
Copy link
Contributor

@electroCutie do know that you can close this issue you opened.

Please also consider if #46 should remain open.

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

No branches or pull requests

3 participants