ncassetta / NiCMidi

A MIDI C++ library with objects for reading, writing, playing, editing and recording midi files
GNU Lesser General Public License v3.0
2 stars 1 forks source link

Crash when use AdvancedSequencer::AdvancedSequencer(MIDISequencerGUINotifier *n) #6

Closed ncassetta closed 2 years ago

ncassetta commented 2 years ago

this was reported to me by goofy2k in another issue. The cause should be a missing line in AdvancedSequencer::AdvancedSequencer (MIDISequencerGUINotifier * n) For @goofy2k: try inserting this line after advancedsequencer.cpp::116

From

116:    MIDIManager::AddMIDITick(this);
117:    // sets the embedded MIDIThru only if the system has almost an in port
118:    if (MIDIManager::IsValidInPortNumber(0)) {

to

116:    MIDIManager::AddMIDITick(this);
117:    ExtractWarpPositions();
118:    // sets the embedded MIDIThru only if the system has almost an in port
119:    if (MIDIManager::IsValidInPortNumber(0)) {
ncassetta commented 2 years ago

... send me a log :-(

goofy2k commented 2 years ago

I also see that there a many types of MIDI message that are still not implemented in my synt. It is very basic! It only recognizes keyon and keyoff over channel 0. So most of the data sent just do nothing.

I will produce a log....

goofy2k commented 2 years ago

Can you tell me which of the lines in the example involve opening / closing the output port.

ncassetta commented 2 years ago

The AdvancedSequencer::Start() calls MIDIManager::OpenOutPorts() (which opens all the out ports in the system) in advancedsequencer.cpp:597, the AdvancedSequencer::Stop() calls MIDIManager::CloseOutPorts() in line 627. Every time a port is open or closed it logs "Out port XXXXX open" (and indeed I see these in your previous logs, so the methods are effectively called)

goofy2k commented 2 years ago

Here is a log WITHOUT the sequencer.UpdateStatus . It is also not error free.....

I will also produce one WITH sequencer.UpdateStatus to tell if there is a difference

It look like there is really something with the stability of the connection / port v25_log_without_sequencer.UpdateStatus.txt

The GATT lines are produced by the nimBLE code. I cannot easily switch those off.

goofy2k commented 2 years ago

Here is the one WITH

NOTE that a laborious initialisation of the nimBLE port/driver takes place after Entered in AdvancedSequencer::Start() This is not OK. I didn't yet analyse the difference between the two logs. v25_log_with_sequencer.UpdateStatus.txt

goofy2k commented 2 years ago

In both cases it is the interaction with init of the port !

First thing to do for me is making that light-light weight for the MIDIManager

goofy2k commented 2 years ago

But first I want to reproduce the settings WITHOUT sequencer.UpdateStatus.txt that ran error free. I lost that....

goofy2k commented 2 years ago

Reorganisation of intitialisation of the nimBLE Midi out port is quite involving and may take me some time. Possibly a few days. Yet I think it is important to make port open and port close in Manager/Driver as light weight as possible. I will be back over here when I have that in place! Have a good Christmas time!

ncassetta commented 2 years ago

Merry Christmas to you :-)

goofy2k commented 2 years ago

Thanks!

goofy2k commented 2 years ago

Succeeded in splitting output port initialisation in a one-of-a-time init part and a light-weight part that is in open/close Port.

An error is logged into the most recent .txt in the v25 folder. RUNTIME SEPARATE INIT AND OPEN.txt

It happens at advancedsequencer.cpp:634 or 628 ( GoToMeasure(state.cur_measure, state.cur_beat) or state.Notify (MIDISequencerGUIEvent::GROUP_TRANSPORT, MIDISequencerGUIEvent::GROUP_TRANSPORT_STOP); )

That part of the code is already passed successfully just before "Playing 3 tracks ..."

The main part of the example is exited successfully. Yet the error(s) that occur have to do with closing or de-init of the output port. This is called within the example, but obviously (?) still executing in a different process after the main code has been exited.

I will try to log somewhat deeper in the relevant part.

ncassetta commented 2 years ago

The error should't depend by UpdateStatus() (I hope) but by a bug in the sequencer autostop. This, in fact, calls Sequencer::Stop() in a separate thread, and this can cause data inconsistency if the main thread performs manipulations of the sequencer (as changing the time, or newly starting) before Stop() ends. Try to insert MIDITimer::Wait(1000); immediately before sequencer.UpdateStatus() at line 165: this waits a second and gives the autostop thread the time to end.

If this succeds, I have just done another commit which (on my computer) solves the problem (it only involves files sequencer.h, sequencer.cpp, advancedsequencer.spp, tick.cpp).

goofy2k commented 2 years ago

OK. I understand that the first action is a temp fix and that the second is a "final" solution?

I am currently upgrading the midi input handling for my synth. Until now it assumed presence of only keyon and keyoff messages. I will use the volume change to control the audio codec hardware. Maybe I define a set of presets containing synth parameters that can be activated by "program change" messages from your sequencer.

Will absorb your improvement later. And let you know the results.

Op zo 26 dec. 2021 11:27 schreef Nicola Cassetta @.***>:

The error should't depend by UpdateStatus() (I hope) but by a bug in the sequencer autostop. This, in fact, calls Sequencer::Stop() in a separate thread, and this can cause data inconsistency if the main thread performs manipulations of the sequencer (as changing the time, or newly starting) before Stop() ends. Try to insert MIDITimer::Wait(1000); immediately before sequencer.UpdateStatus() at line 165: this waits a second and gives the autostop thread the time to end.

If this succeds, I have just done another commit which (on my computer) solves the problem (it only involves files sequencer.h, sequencer.cpp, advancedsequencer.spp, tick.cpp).

— Reply to this email directly, view it on GitHub https://github.com/ncassetta/NiCMidi/issues/6#issuecomment-1001149679, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGII3TRCNFU4LCQSRPSXNBDUS3U27ANCNFSM5KL2VZ2A . You are receiving this because you were mentioned.Message ID: @.***>

goofy2k commented 2 years ago

The MIDITimer::Wait(1000) just before line 165 does not solve the issue.

I will also implement your last commit and report back.

goofy2k commented 2 years ago

I implemented your commit. The runtime error is gone!

For "continuous" testing I added a loop to your example:

while(true) before your first line ( MIDIMultiTrack* tracks = sequencer.GetMultiTrack();)

and end of loop ( } ) just before return EXIT_SUCCESS;

This goes on and on, still without errors.......

NOTE: I had to move AdvancedSequencer sequencer; into your int main(). In this way I can do preparations in my main() for the connections first. The sequencer can then use those after I called your int main.

I believe we have this in place now. Right?

Please let me now your suggestions for testing recording and playing the recordings.

In the mean time I implement more MIDI commands (status types) in my synth. The song doesn't sound very nice because of the non-implemented commands like control change B0 and program change C0 that are used in the song. Doesn't matter! It gives me a goal for further completion of the synth!

goofy2k commented 2 years ago

V26 is in the repo!

ncassetta commented 2 years ago

OK, I began to be a little discouraged :-)

For testing recording and playing you can try the code in the 4th message in this thread. Remember that I assumed incorrectly the char to be signed, while in your compiler they should be unsigned, so perhaps you must change something in recorder::SetTrackRecChannel() (I am working on these bugs on types, which are scattered throughout all the files of the library). Trace the MIDIRecorder::TickProc(), the line 452 // insert the event into the track 453 tracks->InsertEvent(i, msg); records the incoming event into the MIDIRecorder temporary multitrack, and should be executed every time a message arrives on the MIDI in port. If you set the recording channel to -1 every channel should be accepted, otherwise try to set a specific channel in SetTrackRecChannel() 2nd parameter and to send messages with that channel.

ncassetta commented 2 years ago

I would close also this issue (and the #5 ), please if something goes wrong open another issue.

goofy2k commented 2 years ago

OK, please do so!