Feedback to v2: Settings, Participants, Invite, parseControlPackets

[hugbug]

I use the library in an (open source) desktop app. Excellent library! Moreover it's the only one open source library for RTP/Apple-MIDI out there. That's why I use it in a non-arduino project.

I started in spring 2018. That version worked very well but it had an issue with very long connecting time. I've recently updated to v2 and am reporting small issues I've encountered.

Using own settings

I want to change default settings. I also need to use my own UdpClass which communicates via standard sockets:

class MyRtpMidi : public appleMidi::AppleMIDISession<MyMidiSocket,MyRtpMidiSettings>
{
...
};
typedef midi::MidiInterface<MyRtpMidi,MyMidiSettings,MyMidiPlatform> MyMidi;
MyRtpMidi rtpMidi("MyApp", port);
MyMidi midi(rtpMidi);

This code fails to compile when MidiInterface-class attempts to call methods beginTransmission, write and others from my AppleMIDISession-subclass. That's because the friend declaration for MidiInterface instantiates template without MidiSettings:

Arduino-AppleMIDI-Library/src/AppleMIDI.h

Line 43 in 5ecc825

friend class MIDI_NAMESPACE::MidiInterface<AppleMIDISession<UdpClass>>;

As a workaround I've just declared the necessary methods as public but you'll sure find a better way.

Participants

If there are more than one participants only the first one get MIDI-messages. Others get empty packets.

Arduino-AppleMIDI-Library/src/AppleMIDI.hpp

Lines 461 to 468 in 5ecc825

	void AppleMIDISession<UdpClass, Settings, Platform>::writeRtpMidiToAllParticipants()
	{
	for (size_t i = 0; i < participants.size(); i++)
	{
	auto participant = &participants[i];
	writeRtpMidiBuffer(participant);
	}
	}

Because writeRtpMidiBuffer clears the buffer:

Arduino-AppleMIDI-Library/src/AppleMIDI.hpp

Lines 533 to 539 in 5ecc825

	while (!outMidiBuffer.empty())
	{
	auto byte = outMidiBuffer.front();
	outMidiBuffer.pop_front();
	dataPort.write(byte);
	}

Restore connection

I'm connecting to (digital) piano by sending invite. If connections breaks I want it to be reestablished. I can catch OnDisconnected event and then send another invite.

However if the invitation (after all attempts) failed I don't get a notification and don't know when to send invite again. I could increase the number of invite attempts to a ridiculously high number but there other code places where participants are removed and no notifications are sent.

Curently I'm doing the following. In my run loop I check if the participants is empty and then send an invite. For that I had to make participants protected (that was enough to access it from my subclass).

Is there a better way not requiring modifying of the library code?

parseControlPackets

If _appleMIDIParser.parse returns NotSureGiveMeMoreData or NotEnoughData we stay in an endless loop.

Arduino-AppleMIDI-Library/src/AppleMIDI.hpp

Lines 30 to 40 in eddc2a6

	while (controlBuffer.size() > 0)
	{
	auto retVal = _appleMIDIParser.parse(controlBuffer, amPortType::Control);
	if (retVal == parserReturn::UnexpectedData)
	{
	if (NULL != _errorCallback)
	_errorCallback(ssrc, -2);
	controlBuffer.pop_front();
	}
	}

That happend to me before #82 but not after the fix. Anyway it's better to be prepared for all parse results, for example:

    if (retVal == parserReturn::UnexpectedData)
    {
        if (NULL != _errorCallback)
            _errorCallback(ssrc, -2);
        controlBuffer.pop_front();
    }
    else if (retVal != parserReturn::Processed)
        return;

That's it. Thanks again for the library.

[lathoub]

Thanks @hugbug for bringing these issues to my attention - I'll look at them.
(Very impressed by your Conpianist app!)

To start, I'll spin off a v2.1 branch, so that we can test without interfering with the master branch.

[lathoub]

Branch v2.1.0 already addresses issues Participants, Restore Connection (Exception handling). Parse control already addressed in v.2.0.5. (Can you already test @hugbug ?)

Looking into the Settings issue

[hugbug]

v2.1.0 works good.

Found another issue. The invitation rejected-responses do not contain peer name - see packet screenshots:

Invitation rejected by Mac:

Invitation rejected by Yamaha piano:

The parser however attempts to read or skip peer name:

Arduino-AppleMIDI-Library/src/AppleMIDI_Parser.h

Lines 333 to 347 in c834468

	#ifdef KEEP_SESSION_NAME
	uint16_t bi = 0;
	while ((i < buffer.size()) && (buffer[i] != 0x00))
	{
	if (bi <= DefaultSettings::MaxSessionNameLen)
	invitationRejected.sessionName[bi++] = buffer[i];
	i++;
	}
	invitationRejected.sessionName[bi++] = '\0';
	#else
	while ((i < buffer.size()) && (buffer[i] != 0x00))
	i++;
	#endif
	if (i == buffer.size() || buffer[i++] != 0x00)
	return parserReturn::NotEnoughData;

If the buffer contains further packets the parser starts to parse them as the name.

Apple's documentation says the name-part maybe omitted. That's it. There is no header indicating if the name is presented or not.

Knowing the size of the UDP-packet could help. However the design of v2 was changed from "parse udp-packets" to "save all packets into a stream, then parse". It would be helpful for parser to know packet boundaries in the stream though. Especially if a parse error occurs the parser could skip the data up to the next packet, then continue parsing. With current design the parser attempts to parse next data, fails, tries to parse again, fails again and so on. Until it accidentally starts to parse at a position where packet starts.

Before #82 was fixed the parser sometime gone crazy and I received a lot of NoteOff-events (because 0x80 is an often byte in RTP-MIDI). That may took a lot (several minutes) until the program started to work correctly again, either by reconnecting or if parser found the right place in the stream.

So where to save the info about packet boundaries?

First of all you need to know where the packets end when you read them. That is currently problematic because the data is read only in 24-bytes chunks. When using the library in my program I had to change the read data loop. The issue was that with standard sockets there is no function which would tell how many bytes there is to read (available on Arduino).

Furthermore reading in small chunks (24 bytes) is also problematic. Unlike TCP-sockets the remaining data of the UDP-packet is discarded if it wasn't read. The next call to read from UDP-socket will read the next packet and not the data remained from the previous partial read. For that reason I changed the loop to read as much data as possible. And I of course also increased all buffers to much larger sizes (don't have memory constraints on desktop pc).

With my read loop version each read returns one UDP-packet (so I hope) and it is possible to save the information about packet boundaries somewhere. I think the easiest would be to put the length of the packet in the data stream before the packet itself. Then we could easily scroll to the next packet if the parsing of current packet fails.

I don't know how to do that properly on Arduino with small chunks (24 bytes).

Back to invitation reject problem

Anyway if you don't want to change the packet reading, I think you could fix the issue with peer name by checking the first byte of the name. If it's 0xff then it's probably the next AppleMIDI-message and not the peer name. In my tests the next packet was always EndSession-packet which would be recognised by checking for 0xff.

[lathoub]

Well spotted @hugbug , I indeed did not parse for a missing sessionName.
Fixed in latest commit in feat/v2.1.0

[lathoub]

As for the parser, it written for devices with little memory (e.g. Arduino) .
The "save all packets into a stream, then parse" is not entirely true - it's "parse and execute as much as possible (even when all data is not yet received)" when the minimum size of data is received, even when the full UDP frame is not in (hence the _journalSectionComplete and _rtpHeadersComplete flags).

You should be able to mimic the Arduino Ethernet UDP socket behaviour in your MidiSocket class and use the provided readControlPackets(avoiding the #ifdef)

[hugbug]

I guess I misinterpreted readControlPackets loop then. Can you please clarify if the following statement is true:

controlBuffer and dataBuffer may contain one UDP-packet or a part of a packet but never more than one packet.

If that's true, then my version of the loop and MidiSocket may push multiple packets into the buffers and that's the reason why the parser cannot restore after parse errors. I do need to rewrite my MidiSocket class.

[lathoub]

(BTW, this is the best documented Issue on Github I have seen - ever, hands down! 👍🥇)

[lathoub]

Can you please clarify if the following statement is true:

controlBuffer and dataBuffer may contain one UDP-packet or a part of a packet but never more than one packet.

Correct

[hugbug]

readControlPackets and readDataPackets

I reworked my MidiSocket class. Now using the library without changes in read packet-loops. Great!

Read buffer size

UDP packet buffer is currently set to 24 bytes:

Arduino-AppleMIDI-Library/src/AppleMIDI.hpp

Line 7 in 5bdddc5

static byte packetBuffer[UDP_TX_PACKET_MAX_SIZE];

I'd like to use a much larger buffer. For that I do:

#define UDP_TX_PACKET_MAX_SIZE 2048
#include "AppleMIDI.h"

Although that works, it feels like a hack. A better design would be to be able to set the packet size via settings struct instead.

Determine if a work was done

In my app I have a separate thread for RTP. In the thread loop I call midi.read(), which in turn calls methods from AppleMIDISession.

To avoid excessive CPU load I need to call sleep periodically. Currently I do that after midi.read().

If I understand the workflow correctly one call to midi.read() can process only one UDP-packet (or even less). In that case sleeping after each packet would be too much. What I want is to determine if midi.read() resulted in any work done in AppleMIDISession. Then I can sleep if nothing was processed and go to the next loop execution without sleeping in other case.

I was able to implement this strategy by extending my MidiSocket with the info if the data was read or written through the socket. The only issue was to get access to MidiSocket from my thread loop. For that I needed to move declarations of controlPort and dataPort to protected. Please see commit hugbug/conpianist@d7da51c if you wants details.

So my question is: can you make them protected or do you have a better idea?

Compatibility with MS Visual C++

MS Visual C++ doesn't support GCC extension __attribute__ ((packed)). Instead it has pragma for that. This commit hugbug/conpianist@921144f
makes the library compatible with MS Visual C++. Can this be integrated upstream? I can provide a PR or please do the changes yourself. Whatever you prefer.

Warnings in endian.h

endian.h has some strange declaration which looks like a forgotten debug code:

Arduino-AppleMIDI-Library/src/utility/endian.h

Lines 132 to 136 in 5bdddc5

	void aa(uint64_t value)
	{
	if ( value >= 10 )
	aa(value / 10);
	}

Can't imagine having a function with name aa in global namespace on purpose.

The following lines generate warnings (redeclared define) on Mac:

Arduino-AppleMIDI-Library/src/utility/endian.h

Lines 116 to 129 in 5bdddc5

	// From Apple Open Source libc
	#define ntohl(x) ((uint32_t)(x))
	#define ntohs(x) ((uint16_t)(x))
	#define htonl(x) ((uint32_t)(x))
	#define htons(x) ((uint16_t)(x))
	#define ntohll(x) ((uint64_t)(x))
	#define htonll(x) ((uint64_t)(x))
	#define NTOHL(x) (x)
	#define NTOHS(x) (x)
	#define NTOHLL(x) (x)
	#define HTONL(x) (x)
	#define HTONS(x) (x)
	#define HTONLL(x) (x)

I recommend to surround the declarations with a check for Apple platform:

#ifndef __APPLE__
...
#endif

Full example here.

Warnings in rtpMIDI_Clock.h

These two defines produce warnings (redeclared defines) on Mac:

Arduino-AppleMIDI-Library/src/rtpMIDI_Clock.h

Lines 10 to 11 in 5bdddc5

	#define USEC_PER_SEC 1000000
	#define NSEC_PER_SEC 1000000000

Both of them are not used in the library. I suggest to remove them.

---

That's it. Thanks for the support!

[lathoub]

Thanks @hugbug

UDP_TX_PACKET_MAX_SIZE
Done

controlPort and dataPort to protected
done

Removed aa 🙈
Removed USEC_PER_SEC and NSEC_PER_SEC

Moved __attribute__((packed)) to platform specific header (AppleMIDI_Platform*.h)

Working separately on fixing the endian.h warning - stay tuned.

All changes pushed into feat/v2.1.0, pls check and test

[lathoub]

Working separately on fixing the endian.h warning - stay tuned.
done

[hugbug]

All is good, great work!

Nice cleanup of "endian.h" 👍

The only remained issue is "Using own settings". With an easy fix on my side with "proteced->public" it's not a big issue for me.

[hugbug]

Forget to mention: compiles and works on both Mac and Windows.

[lathoub]

The only remained issue is "Using own settings". With an easy fix on my side with "proteced->public" it's not a big issue for me.
Beat you to it :-)

[lathoub]

Forget to mention: compiles and works on both Mac and Windows.
Cool! 😎

I use Xcode to debug the code and try various input packets (emulating Arduino)

[hugbug]

Everything is perfect! 👍

Thank you for quick support 🥇

Closing the issue 🏃