-
Notifications
You must be signed in to change notification settings - Fork 33
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
setHandleNoteOn called when it shouldn't #27
Comments
I have seen the same bug too. |
DAW's tend to send a lot of MIDI messages, could you add a Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 103 to 110 in aa0f6bc
|
do you have the exact same setup @RobertoHE ? |
Hi @lathoub I apologize for the lenght of this message. I have seen the bug in 2 differents ways: the first one with ServerNimBLE, and the second one with my Client proposal (pull request). In the server Mode (BleNimble), I used the following setup: Server (ESP32 ServerNimBLE): Client: I generated "Midi Chords" (some notes simultaneously) and some afterTouch automatizations in channel 5 using DAW while playing MidiController generating notes, pitch-bends and aftertouchs messages in channels 1, 2 and 3. The server was in OMNI_CHANNEL mode (receiving 1 ,2 ,3 and 5 channels). In the second setup I used: Server (AX-EDGE) This setup is "a little weird" because this client is not in this repo yet, but I hope that you might accept it in the future if it works properly (I am still working on it, but I have seen some lines that I want to change). I generated notes, pitch-bends and aftertouchs messages in channels 1, 2 and 3 when I played MidiController (server in this case with BLE notify characteristic) and I could see the NoteOn events using the callback handler. I received in some ocassions weird notesOn events like @aphleps and, at other times, NoteOn callbacks stopped working during severals seconds (between 10s and 90s) but BLE notifications were still received (debug traces were activated and they worked propertly, it still dettected any message generated by MidiController). I have added some traces in notifyCB(), receive() and read() (and others) functions in Client , Transport and Midi layers and I have seen that BLE notify works fine. NimBLE and notifyCB() still receive incoming data correctly and the data uplinks correctly to the receive() transport layer. Receive() function adds data to buffer, but I don't know whether it adds correctly F7 (SysEx) type messages. I think that the problem may appear when SysEx message comes between other midi messages in the same pakage. I used the callback setHandleMessage() and when this error appears MidiMessage callback object is message.type = F0 (system Exclusive) and message.lenght = 128 (max buffer size in my case). After that, a weird noteOn or other type event is reveived. I think that way of extracting data from the package ( while(true){}, lPtr and rPtr) may origine the issue when F7 appear in the middle of the package. |
Thanks @RobertoHE
Good catch, this might be the cause of the issue. Let me check over the weekend with the specs. (A message dump would be ideal for me to debug) |
Hi @lathoub I have to apologize again for the lenght of the message. I send you a log of one of the errors. In this case, the error is which cause that parser get crashes and it stops to trigger callbacks (second error of the previous message). I used the second setup (Client Mode) that it was explained in the previous comment. In this case, the crash occurred when I was sending a lot of afterTouch events (syntetizer may sends more than 50 events in a second). I think that BLEMIDI_Transport.receive() has some problems when it receives a second message that arrives while it is processing other one (for example: DAW sends a note and SysEx or other system data in a sort time or, in this case, a lot of affterTouch messages in sort time). Client mode uses a callback that it is trigged automaticly when a notification occurred, and it may cause that two parser may run simultaneously and they may add data at the same time in the RX buffer. I don't sure completly if the bug occurres because the two parser try to add() simultaneously their data in the buffer and they mix the data accidentally, but it may be a possibility. After the crash bug, some events continued to arrive and parser sended again a event randomly some times until it started to work fine again. I think that the machine of states of parse() suffers when a bad-created message is added to RX buffer and it tries to process it. Until some "bad midis" pass throught the parser and one of them cleans the machine of states, the parser can't trigger any callback. The extra bad-created messages don't trigger error handler, and it isn't normal and it may be a sympthom. I have tested a little be other MIDI parser in C++ (https://github.com/TheKikGen/midiXparser) that may do the same that received() in less time. I haven't tested it enoughtly, I will continue to investigate it. But unfortunately, it may present the same inconvenient when it tries to insert data in RX buffer ( add() method). Alternatively, a flag or semaphore may be added in add() and read() to prevent similtaneous read/write or write/write errors. I show you the traces that I added for "debugging". When BLE notification is received (notifiCB()) from AX-EDGE, it prints "-New MSG-" and the incoming data in HEX format. I set MIDI callbacks as follow:
LOGS:
|
Thanks @RobertoHE , appreciate the long message :-)
Does this would mean that ESP32's Or, is it because SysEx messages are intertwined in the normal messages (and would occur on all hardware)? Where in the above stream are things going wrong? |
Hi again @lathoub . The last bug is NOT generated by a SysEx. The data streams only had I have analyzed In
I am a beginner in FreeRTOS, so I am not completely sure whether I think that it may be necessary to call Midi Messages have a variable length and One inconvenient of Using I will try to test this hypothesis using my setup (client version). Regards |
protect RxQueue with a Mutex See #27 Add() calls to xQueueSend() and copies only 1 byte to Queue. When xQueueSend() is running, the queue is locked and protected, that it is fine. But when xQueueSend() ends, it created some little gaps between a add() function and the next one witch Queue is unprotected, doesn't?
Thanks @RobertoHE
True, very good catch! Admitting, I'm not an ESP32 threading expert, just relying on some old knowledge of thread syncing; please check |
Hi @lathoub I tried to adapt your code to my client class and it still crashed in the same way. I captured another log. In this case, parser detected a I used the same setup as the last test: I only added 3 types of events ( In this case, Example MSG: 08 F7 80 34 2B F7 81 34 2B F8 82 34 2B The You can see how parser missed when it returned a type: LOGS:
|
Hi @lathoub I think that I have found why I think that it is relate with the Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 236 to 239 in aa0f6bc
Let me to put an example: In the Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 223 to 224 in aa0f6bc
Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 236 to 239 in aa0f6bc
The size-filter interprete the event has 4 byte lenght (else line 251). *You can see how a Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 240 to 284 in aa0f6bc
After that,
Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 227 to 232 in aa0f6bc
Now, leftPointer in 8th position (0x34) has a value lesser than 0x80, and it triggers the return prematurely. The 3rd event of the package is skiped completely.
For this package ( When https://github.com/TheKikGen/midiXparser -> This midi parser works byte byte and I haven't see the same bug for the same input if you removed the first 2 bytes. It may help you how to the parser must discriminate correctly a SysEx and don't add a timeStamp F7 to buffer. By the way, the semaphore protection may still necessary. |
Great digging @RobertoHE So remove: Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Line 238 in aa0f6bc
Somehow there was never full support for SysEx across multiple packets, i'll look into that right now. |
Reading up on |
The first byte ( |
It is true. First byte is wrong in my example. It is possible that when I copied this I accidentally changed 0x80 to 0x08. First bit always must be 1 in high timeStamp byte |
I have commented this line and I couldn't replicate the bug. This is good. I don't know how this can affect to a SysEx message splitted in any package. If you need some help I would investigate or try code something. Please, review my pull request of Client #30. Don't accept it now, it needs 2 corrections (uxQueueSpacesAvailable() is useless) but I will appreciate your feedback so much. |
Yes, will let you know :-)
Wilco |
I fixed the initial bug and initial support for (MultiPart) SysEx Can you test? I used these test sets: byte sysEx1Packet[] = { 0xB0, 0xF4, 0xF0, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0xF4, 0xF7 };
receive(sysEx1Packet, sizeof(sysEx1Packet));
std::cout << std::endl << "SysEx part 1" << std::endl;
byte sysExPart1[] = { 0xB0, 0xF4, 0xF0, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08 };
receive(sysExPart1, sizeof(sysExPart1));
std::cout << std::endl << "SysEx part 2" << std::endl;
byte sysExPart2[] = { 0xB0, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0xF4, 0xF7 };
receive(sysExPart2, sizeof(sysExPart2));
std::cout << "ble Packet with 1 MIDI messages" << std::endl;
byte blePacketWithOneMIDIMessage[] = { 0xA8, 0xC0, 0x90, 0x3E, 0x3E };
receive(blePacketWithOneMIDIMessage, sizeof(blePacketWithOneMIDIMessage));
std::cout << std::endl << "ble Packet with 2 MIDI messages" << std::endl;
byte blePacketWithTwoMIDIMessage[] = { 0xA8, 0xC0, 0x90, 0x3E, 0x3E, 0xC1, 0x91, 0x3E, 0x3E };
receive(blePacketWithTwoMIDIMessage, sizeof(blePacketWithTwoMIDIMessage));
std::cout << std::endl << "2 MIDI messages with running status" << std::endl;
byte twoMIDIMessageWithRunningStatus[] = { 0xA9, 0xAE, 0xD1, 0x74, 0xAF, 0xD2, 0x74, 0xB1 };
receive(twoMIDIMessageWithRunningStatus, sizeof(twoMIDIMessageWithRunningStatus));
std::cout << std::endl << "2 MIDI messages with running status" << std::endl;
byte multipleMIDIMessagesMixedType[] = { 0x00 };
receive(multipleMIDIMessagesMixedType, sizeof(multipleMIDIMessagesMixedType));
|
Hi @lathoub
I will try to test it this evening. I may add some 0xF0 to 0xFF events to complete the test, if I have time.
I have some doubts: Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 224 to 242 in 802f7a3
Lines 353-356 may be redundant only if Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 340 to 356 in 802f7a3
Lines 321 to 332 are duplicated inside and afer Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 316 to 334 in 802f7a3
What do you opine about these corrections? |
@RobertoHE very well spotted Sir! (I was trying to prematurely anticipating the removing of the timestamp statements) |
Hi again Are you sure that these lines are necessary?
I don't know if after a splitted SysEx message can appear other event in the same package. In that case, it moves I let me put an example (I don't know if it possible): Package 1: PCK1: PCK2: PCK1 set the flag PCK2 is processed correctly until F7 as SysEx. Inside In lines 323-327, it captures the In line 329, Lines 337 to 346 are the same that lines 319 to 327 and parser does twice the same for a SysEx. At the line 337, In resume, it miss-interpreted everything after F7 SysEx end (NoteOn and NoteOff events). Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 314 to 352 in 687676d
If you repeat the sequence with the 2 packages and you remove the lines 319-329, you will can see that message will be processed correctly. Would you check it, please? |
I fixed an additional bug (test for SysExEnd) and added MSVC parser test code |
Further cleanup and testing (in MSVC, not in the Arduino) and cleared all of the test data - getting closer :-) |
Please, check #32. |
Changes accepted - thank you @RobertoHE |
Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 229 to 231 in c20062e
You recommend not using Based on the current examples in BLEMIDI_Sim.h, i recommend making Arduino-BLE-MIDI/test/msvc/BLEMIDI_Sim.h Lines 106 to 110 in c20062e
|
Arduino-BLE-MIDI/src/BLEMIDI_Transport.h Lines 341 to 342 in c20062e
Is this code needed? Looks like it can be removed. The switch below does not test for |
You are right, it is not neccessary. 0xF0s have 1 byte lenght, except SysEx.
|
I only have tested the parser with default settings of serial parser of main midi class. I haven't have time for check if any other options of setting class would interference with runningStatus mode. If any setting option not interferences with it, runningStatus enable is prefered because it uses less resorces. Other option is change #defines by if(Settings::option). This will add some computer cycles to parser but it would be transparent to end user or end application. Would you check it in paralle? |
Parser is going OK on an ESP32 Devkit v1 using the default ESP32 BLE stack - I tried all kinds on MIDI messages and all come through - good stuff. I'm good to commit to master! FYI: I also tried the NimBLE stack and also ran OK, but I noticed that when Disconnecting, the connect button on the Apple MIDI setup does not come back and the device disappears. |
I have never used Apple MIDI. I don't know anything about it. What is the problem? What is the setup? |
oho - you see, i can't do 2 things at the same time: work on AppleMIDI and BLE-MIDI :-). What i was trying to say: the ESP32 connects fine to the Apple BLE stack, but after disconnect, the ESP32 does not re-appear and i can no longer connect. Does it behave correctly with you? |
Hi again @lathoub I have seen that in this repo there is a common open issue: the server doesn't advertise (not only for apple midi). When I was coding the client class, I used as a template the nimble server of this repo and the client example from nimble repo. I noticed that nimble example allowed multi-client connection and it advertised continuously inside loop. Your nimble server only advertise in setup and it doesn't do anything after a disconnection (the server doesn't advertise itself after a disconnection). I am not sure that nimble has a method which advertices continously. Client mode doesn't use advertiments, it scans the advertised devices and it connects to them, but you can use that mechanism as an example. Client mode has an "auto-reconnect mechanism" inside read (available) function that is managed with onConnect and onDisconnect callbacks and a global variable. For a server, a sofisticate mechanism for advertisement is not needed, but it may need to advertise periodically when it accepts connections from clients (after disconnection, for example). You must decide whether the server accepts one or more clients, because it determines the strategy of performing advertisements. If the server only supports one client (which I think is preferable), you can use a similar mechanism, such as client class, changing scan functions for advertising. I can't code anything until September. |
Hi @lathoub . Check this function: |
Fixed! |
All code moved to the master branch. Thank @RobertoHE |
I'm using the Arduino-BLE-MIDI library in my project, latest version, it's almost identical to the MidiBle.ino example. On my computer I have a 1 bar MIDI loop going in Ableton Live. Everything works for some time, but every now and then I get two weird Note-On events.
HUB :: Note-On channel: 2 note: 61 velocity: 100
HUB :: Note-Off channel: 2 note: 61 velocity: 64
HUB :: Note-On channel: 2 note: 129 velocity: 71 <- weird
HUB :: Note-On channel: 2 note: 64 velocity: 145 <- weird
HUB :: Note-On channel: 2 note: 61 velocity: 100
HUB :: Note-Off channel: 2 note: 61 velocity: 64
HUB :: Note-On channel: 2 note: 68 velocity: 100
HUB :: Note-Off channel: 2 note: 68 velocity: 64
The notes in my clip are 61, 57, 68 and 71 and have a velocity of 100. You can see that after a note-on message a note-off if received. In my clip there is no note 129 with velocity 71, and there is no note 64 with velocity 145 (obviously).
The text was updated successfully, but these errors were encountered: