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

add helper methods to pack and unpack midi data bytes into number types #333

Open
soundanalogous opened this issue Nov 24, 2016 · 7 comments

Comments

@soundanalogous
Copy link
Member

examples:

packUint8 / unpackUint8
packInt8 / unpackInt8
packUint16 / unpackUint16
packInt16 / unpackInt16
packUint32 / unpackUint32
packInt32 / unpackInt32

Not sure if we need a set for 64 bits.

Also open to a naming convention here.

@soundanalogous
Copy link
Member Author

pack methods could accept a number of the specified type and maybe an endian value and either return an array, or write to the stream directly (TBD).

unpack methods could accept a reference to an array and a starting index and maybe an endian value and would return the parsed value in the specified type

@dtex
Copy link

dtex commented Nov 24, 2016

For packing, if we return to an array instead of writing directly to the stream we have the option of leveraging unused bits before writing into the stream.

@soundanalogous
Copy link
Member Author

Also when unpacking, an optional length param could be used in the case that less bytes than needed for the specific type are returned.

@soundanalogous
Copy link
Member Author

@zfields curious to hear your thoughts on this proposal. I assume this would belong in FirmataMarshaller.

@soundanalogous
Copy link
Member Author

@finson did something similar in his Luni (arduino device driver) library, but for translating larger numbers to and from an array of bytes: https://github.com/finson/Luni/blob/b0.10-i5-globals/src/Device/ByteOrder.h. This is what initially inspired my idea to do something similar but for 7-bit midi data bytes, which is not 2 midi bytes for each 8 bit byte, it's based on the number of bits (7 per midi data byte so 3 midi data bytes to create a 16 bit integer and 5 to create a 32 bit integer, etc).

@zfields
Copy link
Contributor

zfields commented Nov 26, 2016

@soundanalogous I think they would belong as private members to FirmataMarshaller.

void FirmataMarshaller::sendAnalog(uint8_t pin, uint16_t value)

Here, you would need the ability to pack the value parameter efficiently, and having a packUint16 method sounds like a great idea.

void FirmataMarshaller::sendString(const char *string)

Here, you don't need anything extra, because basic ASCII has a range between [0, 127]. However, extended ASCII would require more sophisticated packNBytes method.

The point being, as long as the pack___ methods support the protocol they make sense. However, I would be skeptical about the need for them to have public access. Allowing a user to form Firmata compatible byte strings seems like it would be handy at first blush, but it also seems to defy the intent of FirmataClass. Adding efficiency to the FirmataMarshaller is great, but adding complexity to the public API (especially when not directly related to the protocol) seems like a mistake.

@soundanalogous
Copy link
Member Author

soundanalogous commented Jan 3, 2017

could also consider supporting floats if necessary, however there is a huge advantage to using integers over floats whenever possible on microcontrollers without native floating point processors (most MCUs supported by Firmata). Something like a 40x more efficient using integers.

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