Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve Midi Input Part 1 #142

Merged
merged 13 commits into from
Oct 22, 2024
Merged

Improve Midi Input Part 1 #142

merged 13 commits into from
Oct 22, 2024

Conversation

simotek
Copy link

@simotek simotek commented Sep 27, 2024

  • Rather then only using midi input from the config, it now also follows the midi input in the project settings.
  • lgpt now starts and stops with, start and stop events from Midi In.
  • General cleanup on parts of the code that were touched.

Todo in Pt 2:

  • Disable midi transport via a config file option.
  • Use midi in tempo events for sync and setting tempo
  • Fix continue behavior (Currently it restarts)

@djdiskmachine
Copy link
Owner

Wow midi in how cool!
The clang formatting makes it very difficult to see what you actually did though.

@djdiskmachine
Copy link
Owner

Hide whitespace makes it reviewable.

So this is working as is now and you'll do the rest on a second commit?
How are you planning to present to the user if the device is midi lead or midi follow?

@simotek
Copy link
Author

simotek commented Sep 29, 2024

Hide whitespace makes it reviewable.

So this is working as is now and you'll do the rest on a second commit? How are you planning to present to the user if the device is midi lead or midi follow?

It is working now as is, the fact there is no setting to disable it is maybe the only reason not to include it as is.

As for a setting I was thinking something along the lines of Midi Transport with the options of None, Control and Sync with the last option changing the tempo to match the midi clock messages and possibly using the midi messages rather then the internal clock (But I need to test how well that actually works).

Currently you can add MIDISENDSYNC=YES to the config to enable sending midi sync messages (I haven't actually tested that). But maybe it makes sense to use the transport setting for both and maybe have a MIDIMODE setting with Host, Client and Client+Through with the last option receiving messages as well as sending them. Given that you may be receiving Midi clock messages from a sequencer but then also sending messages out to a synth running on a completely different channel that may not get the messages from the sequencer. But maybe @maks has some ideas although I can't tag him here it seems so I guess ill ping him on Discord

MIDISENDSYNC

@maks
Copy link

maks commented Sep 29, 2024

This is great @simotek !
I'll see if I can make some time to start working on similar fixes for picoTracker too based on your work here. Its not possible to directly use the code as pT's midi implementation is already starting to diverge a fair bit from lgpt.

And his is how I implemented the UI so far on picoTracker, my plan is to add field to control MIDI transport and have setting in "MIDI sync" for using external clock in.

PXL_20240929_221903904

@simotek
Copy link
Author

simotek commented Sep 30, 2024

This is great @simotek ! I'll see if I can make some time to start working on similar fixes for picoTracker too based on your work here. Its not possible to directly use the code as pT's midi implementation is already starting to diverge a fair bit from lgpt.

And his is how I implemented the UI so far on picoTracker, my plan is to add field to control MIDI transport and have setting in "MIDI sync" for using external clock in.

Not relevant for this PR but once the picoTracker has USB Midi working maybe a both option for outputs would make sense, I could see someone hooking one synth up via USB and another via TRS and wanting to control both on separate channels. For lgpt I think its easy enough to do this with OS tools on any devices that actually have multiple midi options.

@djdiskmachine
Copy link
Owner

This is great!
No way to disable as in there's no way to stop an incoming MIDI start message from starting the sequencer?
I find this to be a very minor issue, looking forward to the Part 2 =)

@djdiskmachine
Copy link
Owner

Again sorry for the rogue clang-format, you can copy the suggested changes and apply as a patch :)

@djdiskmachine djdiskmachine added the enhancement New feature or request label Oct 1, 2024
simotek added 12 commits October 3, 2024 11:02
@simotek
MidiIn: Reenable old code
015f40b
@simotek
Midi In: Infra to handle midi start stop cont
e6aba6e 
The implementation doesn't actually do anything yet but the
messages should be handled.
@simotek
MidiIn: Implement Start Stop Continue
7e8711f
@simotek
MidiService Cleanup
c44fe28 
* Use 4 spaces everywhere
* Remove extra semicolons
@simotek
MidiService: Use outDevice instead of device
4558f74 
This is so that we can add a midiInDevice in the near future.
@simotek
MidiService: Order Header Files
5559536
@simotek
Application.cpp/h: Cleanup
9384eba 
* Use 4 spaces
* Order header files
* Remove extra semicolons
@simotek
Fix Starting MidiInDevice
a8c291d 
Previously the midi in was only started if the midi device was
present in the config. With this change it is now started and
updated whenever the project settings change. This includes
starting input when a project is loaded if a device is set in the
project.
@simotek
MidiInDevice: Cleanup codebase
cde0866 
* 4 spaces everywhere
* Remove extra semicolons
* No space before semicolon
@simotek
Fix Midi In Start Stop
47a3b0e 
Now that midi devices are created correctly I could actually test
this.
@simotek
RG35XXPLUS: Fix mapping file
85436f5 
rshoulder and lshoulder were the wrong way
@simotek
RG35XXPLUS: Clean up mapping.xml
e1b2e77 
* Use 4 spaces for indent everywhere
* Put everything in a more sensible order
@simotek
Copy link
Author

simotek commented Oct 3, 2024

Clang format is done, I also ran it on the header files that go with the .cpp files so there may be some headers with only formatting changes.

djdiskmachine
djdiskmachine reviewed Oct 3, 2024
View reviewed changes
sources/Services/Midi/MidiMessage.h Outdated
@@ -16,9 +16,6 @@ struct MidiMessage:public I_ObservableData
MIDI_CHANNEL_AFTERTOUCH = 0xD0,
MIDI_PITCH_BEND = 0xE0,
MIDI_MIDI_CLOCK = 0xF0,
MIDI_START = 0xFA,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we get to keep these?

@djdiskmachine djdiskmachine merged commit fbebbcf into djdiskmachine:master Oct 22, 2024
12 checks passed
djdiskmachine added a commit that referenced this pull request Oct 22, 2024
@djdiskmachine
Revert "Improve Midi Input Part 1 (#142)"
13cdfa0 
This reverts commit fbebbcf.
djdiskmachine added a commit that referenced this pull request Oct 22, 2024
@djdiskmachine
Revert "Improve Midi Input Part 1 (#142)" (#156)
8816157 
This reverts commit fbebbcf.
@djdiskmachine
Copy link
Owner

On playing project without any MIDI devices connected I get a start/stop loop on X64

@djdiskmachine
Copy link
Owner

djdiskmachine commented Oct 22, 2024 •
edited
Loading

On playing project without any MIDI devices connected I get a start/stop loop on X64

This issue shows up when the default midi device in project view is set to Midi through:0
When setting it to null or any actual midi device this behavior stops

Sample log output:

[MidiService] midi device Midi Through:0 started
[EVENT] key(Space:44):0
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Down:81):1
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Down:81):0
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Up:82):1
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Up:82):0
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Space:44):1
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Space:44):0
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(W:26):1
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[EVENT] key(Up:82):1
[-D-] midi in:FA:0:0
[EVENT] midi:start
[EVENT] key(Up:82):0
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[EVENT] key(W:26):0
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[EVENT] key(S:22):1
[EVENT] key(Left:80):1
[-D-] midi in:FA:0:0
[EVENT] midi:start
[MidiService] midi device Midi Through:0 started
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[EVENT] key(Left:80):0
[-D-] midi in:FA:0:0
[EVENT] midi:start
[-D-] midi in:FC:0:0
[EVENT] midi:stop
[EVENT] key(Right:79):1

RtMidiIn::setCallback: a callback function is already set!

[MIDI] Controlling activated for MIDI interface Saffire 6USB2.0:0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djdiskmachine djdiskmachine djdiskmachine approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simotek @djdiskmachine @maks