Re: [PATCH 1/1] sound: rawmidi: Add framing mode

[Jaroslav Kysela <perex@perex.cz>]

Dne 24. 03. 21 v 17:17 David Henningsson napsal(a):
> 
> On 2021-03-24 17:06, Jaroslav Kysela wrote:
>> Dne 24. 03. 21 v 6:31 David Henningsson napsal(a):
>>> This commit adds a new framing mode that frames all MIDI data into
>>> 16-byte frames with a timestamp from the monotonic_raw clock.
>> I would add support for monotonic timestamps, too. The NTP drifts are usually
>> small, so it may make sense to support those timestamps, too. It may be handy
>> for the synchronization among multiple machines (timing sources).
>>
>> The timestamp mode should be selected separately than the framing mode.
> Okay, noted for v3.
>>
>>> +#define SND_RAWMIDI_FRAMING_DATA_LENGTH 7
>>> +
>>> +struct snd_rawmidi_framing_tstamp {
>>> +	unsigned int tv_sec;	/* seconds */
>>> +	unsigned int tv_nsec;	/* nanoseconds */
>>> +	unsigned char length;
>>> +	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
>>> +};
>> Perhaps, we should consider to have a fixed header and variable data length
>> here. For MIDI, the standard messages have only few bytes usually. It would be
>> better to use this space for the seconds field:
>>
>> header {
>> 	unsigned long long tv_sec;
>> 	unsigned int tv_nsec;
>> 	unsigned int len;
>> 	unsigned char data[0];
>> };
> 
> I considered that, but it has problems with alignment. If you have a 
> normal midi message of 3 bytes, now your second tv_sec will end up 
> starting on an odd byte, unless you add padding, and then that padding 
> needs to be specified and so on. In addition, half of the header could 
> end up in the end of the ring buffer and the other half in the 
> beginning. So I found the 16 byte fixed version to be simpler and easier 
> to implement correctly.

I see. I agree that the fixed frame is easier to handle.

> However if you like we could change the tv_sec to 64 bit and end up with:
> 
> #define SND_RAWMIDI_FRAMING_DATA_LENGTH 3
> 
> struct snd_rawmidi_framing_tstamp {
> 	unsigned long long tv_sec;	/* seconds */
> 	unsigned int tv_nsec;	/* nanoseconds */
> 	unsigned char length;
> 	unsigned char data[SND_RAWMIDI_FRAMING_DATA_LENGTH];
> };
> 
> We'll then have only three bytes for the actual data, but since that is what most midi messages are anyway, it would be okay, I assume.

We can use the free bits in tv_nsec. It may be possible to carry 4 midi bytes
with the 64-bit tv_sec field, too.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.