-
Notifications
You must be signed in to change notification settings - Fork 66
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
Feedback to v2: Settings, Participants, Invite, parseControlPackets #83
Comments
Thanks @hugbug for bringing these issues to my attention - I'll look at them. To start, I'll spin off a v2.1 branch, so that we can test without interfering with the master branch. |
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 |
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
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 ( 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 I don't know how to do that properly on Arduino with small chunks (24 bytes). Back to invitation reject problemAnyway 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 |
Well spotted @hugbug , I indeed did not parse for a missing sessionName. |
As for the parser, it written for devices with little memory (e.g. Arduino) . You should be able to mimic the Arduino Ethernet UDP socket behaviour in your |
I guess I misinterpreted
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. |
(BTW, this is the best documented Issue on Github I have seen - ever, hands down! 👍🥇) |
Correct |
readControlPackets and readDataPacketsI reworked my MidiSocket class. Now using the library without changes in read packet-loops. Great! Read buffer sizeUDP packet buffer is currently set to 24 bytes:
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 doneIn my app I have a separate thread for RTP. In the thread loop I call To avoid excessive CPU load I need to call If I understand the workflow correctly one call to I was able to implement this strategy by extending my So my question is: can you make them Compatibility with MS Visual C++MS Visual C++ doesn't support GCC extension Warnings in endian.h
Arduino-AppleMIDI-Library/src/utility/endian.h Lines 132 to 136 in 5bdddc5
Can't imagine having a function with name The following lines generate warnings (redeclared define) on Mac: Arduino-AppleMIDI-Library/src/utility/endian.h Lines 116 to 129 in 5bdddc5
I recommend to surround the declarations with a check for Apple platform: #ifndef __APPLE__
...
#endif Full example here. Warnings in rtpMIDI_Clock.hThese two defines produce warnings (redeclared defines) on Mac: Arduino-AppleMIDI-Library/src/rtpMIDI_Clock.h Lines 10 to 11 in 5bdddc5
Both of them are not used in the library. I suggest to remove them. That's it. Thanks for the support! |
Thanks @hugbug
Removed Moved Working separately on fixing the endian.h warning - stay tuned. All changes pushed into |
|
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. |
Forget to mention: compiles and works on both Mac and Windows. |
|
I use Xcode to debug the code and try various input packets (emulating Arduino) |
Everything is perfect! 👍 Thank you for quick support 🥇 Closing the issue 🏃 |
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:
This code fails to compile when
MidiInterface
-class attempts to call methodsbeginTransmission
,write
and others from myAppleMIDISession
-subclass. That's because the friend declaration forMidiInterface
instantiates template without MidiSettings:Arduino-AppleMIDI-Library/src/AppleMIDI.h
Line 43 in 5ecc825
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
Because
writeRtpMidiBuffer
clears the buffer:Arduino-AppleMIDI-Library/src/AppleMIDI.hpp
Lines 533 to 539 in 5ecc825
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 makeparticipants
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
returnsNotSureGiveMeMoreData
orNotEnoughData
we stay in an endless loop.Arduino-AppleMIDI-Library/src/AppleMIDI.hpp
Lines 30 to 40 in eddc2a6
That happend to me before #82 but not after the fix. Anyway it's better to be prepared for all parse results, for example:
That's it. Thanks again for the library.
The text was updated successfully, but these errors were encountered: