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

Start implementing UI/GUI in Nodered #15

Closed goofy2k closed 1 year ago

goofy2k commented 2 years ago

CURRENT STATUS

I have been busy with upgrading of the synth side. I have a GUI under Nodered where I can set controls for the Digital Sound Processing (currently Wavesynth_FX engine under Faust (https://faust.grame.fr/), using the faust2api script for ESP32.

I can now select groups of DSP parameters (= presets / programs) , edit these with sliders and save / retrieve them on an SD card. Presets can be set per channel via the MIDI "Cn pp" command. This means that your test_advancedsequencer_noinput example can properly select presets/programs. I also have preset banks that can be played in "DRUMS" mode: the pitch input is used to select a drum preset, frequency is fixed in the preset.

Any MIDI commands issued by the Nodered GUI arrive at the sequencer app over MQTT (Midi In), and currently are forwarded to the synth board using NiCMidi thru.

The test_advancedsequencer_noinput example and a recorder based on test_recorder2 run pretty OK without any input.

NEXT STEPS

I will now add user interaction to get more control over the advanced_sequencer and recorder. I will base this on your test_recorder.cpp and/or test_advancedsequencer.cpp. First step is (I think) to create a handler for a set of commands as in "available commands" in test_recorder.cpp. A probable upcoming challenge for me is to have thru running when recorder or sequencer are idle.

I will return here when I have the Nodered GUI in place that is equivalent to your command despatcher in test_recorder.cpp

ncassetta commented 2 years ago

I am happy for your progress, and for the good working of NiCMidi. As for the thru, it is true that it could be a problem when using AdvancedSequencer and MIDIRecorder together (I hadn't thought of that). The AdvancedSequencer has a built-in MIDIThru (which can work even if the sequencer is not playing) while the MIDIRecorder sends the notes it receives to the out ports directly into its TickProc (only when it is recording). So during the recording there would be two thru's. Probably an option to disable the thru in the MIDIRecorder should be added.

goofy2k commented 2 years ago

Thanks for your advice/suggestions! So the MIDIRecorder has a built in thru. Of course I already noticed because my notes appear at MIDIout when recording.

If I disable this "internal" thru function, can I just add thru.cpp via the MidiTick stack, and use it together with sequencing and recording?

I will probably discover the answer later ;-).

ncassetta commented 2 years ago

Yes, but you can use also the embedded thru in the AdvancedSequencer class (which you can switch on and off regardless the sequencer playing). The critical lines in the recorder.cpp are these

recorder.cpp:441

                for (i = 0; i < tracks->GetNumTracks(); i++) {
                    if (!en_tracks[i]) continue;
                    signed char ch2 = tracks->GetTrack(i)->GetRecChannel();
                    if (ch1 == ch2 || ch2 == -1) {
                        // insert the event into the track
                        tracks->InsertEvent(i, msg);
                    }
                    // tell the driver to send this message
                    MIDIManager::GetOutDriver(tracks->GetTrack(i)->GetOutPort())->OutputMessage(msg);
                }

Comment the last line which sends the recorded message to the out driver.

goofy2k commented 2 years ago

OK, thanks. First thing to do now is to make that the most important commands can be sent from the GUI to the app . Currently creating a command dispatcher, similar to that in your test_ recorder.cpp, but with GetCommand replaced by an MQTT input handler. The buttons and switches (e.g. rec on/off) are already on the screen.

Op za 26 feb. 2022 22:02 schreef Nicola Cassetta @.***>:

Yes, but you can use also the embedded thru in the AdvancedSequencer class (which you can switch on and off regardless the sequencer playing). The critical lines in the recorder.cpp are these

recorder.cpp:441

            for (i = 0; i < tracks->GetNumTracks(); i++) {
                if (!en_tracks[i]) continue;
                signed char ch2 = tracks->GetTrack(i)->GetRecChannel();
                if (ch1 == ch2 || ch2 == -1) {
                    // insert the event into the track
                    tracks->InsertEvent(i, msg);
                }
                // tell the driver to send this message
                MIDIManager::GetOutDriver(tracks->GetTrack(i)->GetOutPort())->OutputMessage(msg);
            }

Comment the last line which sends the recorded message to the out driver.

— Reply to this email directly, view it on GitHub https://github.com/ncassetta/NiCMidi/issues/15#issuecomment-1052610403, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGII3TRFVHFC4V65V6ORPKLU5E5XFANCNFSM5PKBQ6NA . You are receiving this because you authored the thread.Message ID: @.***>

goofy2k commented 2 years ago

I could do with some of your advice on the following:

Initialisation of the MIDIout port is activated through the first call of manager. If I am correct, in the test_recorder.cpp this happens in the line AdvancedSequencer sequencer(&text_n). This line also makes sure that sequencer is globally accessible and can be called from main().

In my existing implementation of the recorder example without input, I created a procedure that contains this line and the rest of the code. This was fine as long as all calls to members of sequencer where in this single procedure.

For my new implementation the sequencer instance must also be accessible from other routines. So I created a code where the line AdvancedSequencer sequencer(&text_n) is in the global scope again. I get a problem: "(xQueueTakeMutexRecursive)- assert failed!" . This has probably to do with the fact that some other initialisations on startup are not yet ready when the sequencer is initiated. So, I am looking for a way to get global access to the sequencer instance but prevent getting the port problem.

A potential solution might be to prevent calling MIDIManager::Init on a global level, and call it explicitly in a routine (could be main). Do you foresee a problem with this?

goofy2k commented 2 years ago

I now think that I could also instantiate any NiCMidi instance that uses manager globally, but as late as possible. And I could adapt Init in such a way that it properly waits for availability of the required NimBLE resource.

goofy2k commented 2 years ago

To illustrate the nature of the issue I created a log of the run time error. Should be read from end to start to see the cause of events. For each reported line number I added the actual code:

error tracking.txt

The point is probably that (global) instantiation of sequencer ultimately triggers instantiation of the blueoooth port : NimBLEDevice::init("fckx_seq") . This latter is not suitable to be called globally.

A way out could be to define the sequencer instance globally, but to run it's call to NimBLE (via !MIDIManager::IsValidOutPortNumber(0)) only locally. This would mean that I would have to remove this call to MIDIManager from the seqeuencer constructor and put it in a separate sequencer.Init that can be called later locally. [Tried this, but does not help. All calls of MIDIManager::Init hit depend on availability of the port(s). I need to give port initialisation a good thought]

ncassetta commented 2 years ago

In previous versions of NiCMidi I initialized an instance of MIDIManager as a static variable. This however led to the "static initialization fiasco" because it relied on other static variables, whose initialization order depended on the compiler. So I devised the MIDIManager :: Init mechanism, which initializes the manager at runtime the first time one of its functions is called. Perhaps a solution to your problem would be to declare a pointer to the AdvancedSequencer as a global variable, and to instantiate it in main (). Then the other routines could be accessed by means of the pointer.

ncassetta commented 2 years ago

But once the static initialization phase is over the resources should be available, so it seems strange to me that you can't have bluetooth available. Is it not possible for you to work on this?

goofy2k commented 2 years ago

I now found out that I probably have put too much into the MIDIManager::Init procedure. Part of the code can be called later, after main has been started.

Currently my openPort does virtually nothing. The "part of the code" that I mentioned above must probably moved into the openPort procedure. This can be called after main has started.

I am not very familiar with terms like "static" but I smell that it has something to do with "global" code that is executed before main is started.

I already tried some implementation of the above but ran into some runtime issue with very little post mortem info.

Will be continued....

Op di 1 mrt. 2022 18:51 schreef Nicola Cassetta @.***>:

But once the static initialization phase is over the resources should be available, so it seems strange to me that you can't have bluetooth available. Is it not possible for you to work on this?

— Reply to this email directly, view it on GitHub https://github.com/ncassetta/NiCMidi/issues/15#issuecomment-1055700577, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGII3TQV3EPAKMENPJU2C33U5ZKK3ANCNFSM5PKBQ6NA . You are receiving this because you authored the thread.Message ID: @.***>

goofy2k commented 2 years ago

I hit upon an error when trying to use your suggestion on using global pointers. I posted a question on Stackoverflow. See: https://stackoverflow.com/questions/71329110/how-to-make-a-set-of-mutually-dependent-classes-globally-accessible

Based on the input in the forum I can now instantiate the sequencer and recorder anywhere in my code and also use these objects anywhere as the pointers at them were defined globally! More upcoming

ncassetta commented 2 years ago

Yes, I intended things as in the suggestion of stackoverflow. The pointer is a global variable (which isn't initialized before the main() start) and you can set it lately. The constructor of MIDIRecorder requires a pointer to a MIDISequencer, so (as your ptrAdvancedSequencer is itself a pointer) you don't need the & anymore.

goofy2k commented 2 years ago

Great, learning al the time :- ) I had of course seen this before, but when you don't encounter things regularly it is hard to have the knowledge directly available.

I can now proceed with "connecting" my command buttons of the GUI with actual code (command dispatcher).

[EDIT] I use Stackoverflow for questions that are in my view C++ or ESP32 specific and not NiCMidi specific. Then I don't have to bother you with those ;-) . I'll keep you up to date about that channel.....

goofy2k commented 2 years ago

I can use a number of commands now from the Nodered GUI. I can switch on/off thru (I use thru.cpp), start/stop recording, start/stop playing a recorded sequence.

I am now working on getting status information to the GUI. For this, I planned to modify your notifier. This is necessary for two reasons:

1) I need to send GUI status information to my "GUI server". The first step in the chain is to get it out of the sequencer over bluetooth/NimBLE. 2) To reduce communication bandwidth I want to send the raw event data (unsigned longint) and not readable text.

For using NimBLE I must either a) bring control over NimBLE into notifier.cpp OR b) use an approach, like you did, with output to an ostream ost object. In stead of using the default case where ost is cout I can create a class that captures this output and redirects it to a NimBLE port.

I already did some trials with approach a), without success. Approach b) involves a new challenge of writing the redirection class, but it appears more generic to me as it prevents bringing NimBLE code into the notifier (despite the the other adaptations. This prevents adding dependencies to NiCMidi.

I can practice with using redirection by choosing a non-default value for std::ostream& os

ncassetta commented 2 years ago

The class MIDISequencerGUINotifier is made to be subclassed. You can define your "MyNotifier" class inheriting from this and redefine the Notify() method. It accepts as parameter the raw data (a MIDISequencerGUIEvent), and should only send it to your "GUI server". If the Nodered ambient is event driven,when the notify arrives it should do the necessary work to decodify data (with the MIDISequencerGUIEvent methods) and display them. In this way you minimize the data transferred to your server

goofy2k commented 2 years ago

Understand. I have some trouble with getting control on my nimBLE device from within notifier,cpp. So I am trying to design a class derived from ostream that I can pass to the notifier (in stead of the default cout, which is also derived from ostream). But htis is pretty advanced/complex.

goofy2k commented 2 years ago

I think that I will expand my nimBLEdriver.cpp . Besides the MidiOutNimBLE, I will create a second port: GUIOutNimBLE.

In that way I can access the second port pretty similar to the midiout port.

goofy2k commented 2 years ago

It took me some while.... but at last I can now send GUI notifications over a customized output stream that replaces cout. The "raw" notifier creates 8 byte numbers that are in fact the event number. Sending of a small sequence of bytes is more efficient. Also I can do decoding of this number in my GUI server.

I now do this by the following sequence of commands:

Outbuf_buffered_fckx outbuf_buffered_fckx;        //create a customized streambuffer (code for this class  not shown here)
                                                                              //this streambuffer returns the event data (long unsigned int)
                                                                              //so, NOT human readable text, for transport efficiency
                                                                              //the transport is over MQTT (not nimBLE as mentioned earlier)

std::ostream custom_ostream(&outbuf_buffered_fckx);       //use the customized streambuffer in a standard ostream object  
MIDISequencerGUINotifierRaw raw_n(0,custom_ostream);`//use a specialised Notifier that returns long unsigned int,  8 bytes
                                                                                             //override the default std::cout by the custom ostream
ptrAdvancedSequencer = new AdvancedSequencer(&raw_n);  //use the raw GUI notifier in the sequencer

I am looking for a way to pack additional data into the 8 bytes that are used for ev (the event data). So I looked at the current bits occupation. If I number the bytes from 0-7, item is stored in bytes 0,1,2, group in bytes 3,4.

I believe that there are enough bits available for additional data. Bytes 5,6,7 are unused and there is room left over in the group and item bytes.

A thing is puzzling me: subgroup is using 3 bytes divided over 1,2,3,4 because it is shifted by 12 positions. Why is this? This storage overlaps with the group and item storage. On the other hand, you don't use subgroup in other than the USER group.

Once I understand the rationale I can adapt my MIDISequencerGUINotifierRaw to include e.g. the beat and measure numbers in the 8 bytes.

ncassetta commented 2 years ago

Yes, in the MIDISequencer GUIEvent I didn't notice this strange arrangement, I left the original jdksmidi code: I thought there was no need to include extra info in the messages because when the GUI receives a message it can query the sequencer via the various get methods (as I did in various examples). Probably in this way you could use only a 4 bytes message

goofy2k commented 2 years ago

Thanks. I think that I will design a scheme for my purpose. At first I will try to keep the current byte occupation and only add info to currently unused bytes/bits. I will try to make changes only to notifier.h/.cpp.

Op za 2 apr. 2022 07:22 schreef Nicola Cassetta @.***>:

Yes, in the MIDISequencer GUIEvent I didn't notice this strange arrangement, I left the original jdksmidi code: I thought there was no need to include extra info in the messages because when the GUI receives a message it can query the sequencer via the various get methods (as I did in various examples). Probably in this way you could use only a 4 bytes message

— Reply to this email directly, view it on GitHub https://github.com/ncassetta/NiCMidi/issues/15#issuecomment-1086550520, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGII3TXWS67DEWYTTP4VNUDVC7KRXANCNFSM5PKBQ6NA . You are receiving this because you authored the thread.Message ID: @.***>

goofy2k commented 2 years ago

I can now send raw GUI event data (4 bytes) over MQTT. I reserved 4 bits each for item, subgroup and group and 8 bits each for data1, data2 that can represent e.g. BEAT, MEASURE, time signatures etc. I have to decide how I will transport text data such as e.g. trackname. It may be necessary to extend the data item that is used by MEASURE to 12 bits to allow for longer tracks.

ncassetta commented 2 years ago

For this reason I (and also jdksmidi) enclosed into the messages info only on "what's happened". There are too many parameters to watch and embedding them all would be complicated. You can see the function ProcessNotifierMessage in the example win_32/test_win32_player.cpp:509 which is called when a message arrives to the GUI. It queries the sequencer about the appropriate parameter and updates the various textboxes (I think this shouldn't occupy much more band). Its code is Windows-specific but adapting it should be simple (without the need to modify the notifier)

goofy2k commented 2 years ago

The path where I use MQTT to transport GUI data to/from my panel is a dead end. It is too slow.

I want to create a faster way to transport the data. Currently investigating a wired connection (ethernet). I have the hardware, but I am struggling with the firmware...... This is far from NicMidi....