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.

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

MIDI Clock Input #702

Closed
wants to merge 31 commits into from
Closed

MIDI Clock Input #702

wants to merge 31 commits into from

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Sep 2, 2015

This PR implements master sync using an external midi clock as the source. It does not add a midi output clock. It is still early and has some bugs, and the skin support is extremely basic and just for testing, but it works.

Includes unit tests.

@@ -173,6 +173,7 @@ bool PortMidiController::poll() {
if ((status & 0xF8) == 0xF8) {
// Handle real-time MIDI messages at any time
receive(status, 0, 0);
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a big bug in current master.

@ywwg
Copy link
Member Author

ywwg commented Sep 3, 2015

Ah, I forgot that I hadn't created a way of enabling or disabling midi clock source on a per-controller basis. I'll have to figure that out or disable this by default before merging.

setDeviceCategory(tr("MIDI Controller"));
m_pClockBpm.reset(new ControlObjectSlave("[MidiSourceClock]", "bpm"));
Copy link
Member

Choose a reason for hiding this comment

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

Why you do not use the constructor in the init list?

@daschuer
Copy link
Member

daschuer commented Sep 3, 2015

Do you have a clue how accurate this is to beat-match with a external device.
I am afraid this will not work without using the midi time stamp.
Do you think it will be hard to use it?

const double bpm = static_cast<double>(tick_count - 1) / kPulsesPerQuarter
/ elapsed_mins;

return math_clamp(bpm, kMinMidiBpm, kMaxMidiBpm);
Copy link
Member

Choose a reason for hiding this comment

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

Clamping is always wrong here. Can we return just 0 and let the calling code decide?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 18, 2018

This pull request has not been updated in years so I am closing it to reduce the clutter of open PRs. If you solve the merge conflicts and would like to have this merged in the future, please reopen it. If you want to continue working on this, I think we should consider how to make this more generic so the code could also be used for Ableton Link.

@Be-ing Be-ing closed this Apr 18, 2018
@daschuer
Copy link
Member

This is still a desired feature.

@daschuer daschuer reopened this Apr 18, 2018
@Be-ing Be-ing added stalled and removed stalled labels Apr 18, 2018
@Be-ing Be-ing added this to the stalled milestone Apr 18, 2018
@Holzhaus
Copy link
Member

Holzhaus commented Sep 10, 2020

@ywwg Syncing from MIDI clock would be a nice feature. Any plans to continue working on this?

@ywwg
Copy link
Member Author

ywwg commented Sep 16, 2020

blast from the past! I'll see how bad the conflicts are

@kbdnr
Copy link

kbdnr commented Sep 16, 2020

Any reason to opt for midi over something like Ableton Link which will compensate for latency and solve output clocking? I see the original PR was made in 2015 which predates link.

In my experience, while the midi standard specifies tick/cycle, this is not as commonly standardized across devices (before even looking into how devices handle stop/start messages).

@Holzhaus
Copy link
Member

Holzhaus commented Sep 16, 2020

Any reason to opt for midi over something like Ableton Link which will compensate for latency and solve output clocking?

Ableton Link support would be nice as an additional feature, but cannot replace MIDI clock because it's not nearly as widely supported by actual hardware. For example, MIDI clock output is necessary to sync the TR-S drum machine on the Roland DJ-505 to a playing deck in Mixxx.

@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:54
@ronso0 ronso0 marked this pull request as draft November 17, 2020 16:01
@Holzhaus
Copy link
Member

Holzhaus commented Feb 7, 2021

Let's try to get this PR merged. I tried to fix the merge conflicts here: #702

Tests don't pass due to some debug assertions. @ywwg would be great if you could take a look.

@Holzhaus Holzhaus changed the title Midi master MIDI Clock Input Feb 7, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Feb 7, 2021

Let's try to get this PR merged. I tried to fix the merge conflicts here: #702

I think you meant to link ywwg#11

@Holzhaus
Copy link
Member

Holzhaus commented Feb 7, 2021

Let's try to get this PR merged. I tried to fix the merge conflicts here: #702

I think you meant to link ywwg#11

Oops ;-)

Resolve merge conflicts for MIDI clock output PR 702
@Holzhaus
Copy link
Member

Holzhaus commented Feb 8, 2021

Currently this PR only considers a single input clock source. IMHO each attached controller might provide a separate input clock, hence each controller should act a separate clock provider, correct?

I'm not really familiar with the details of the sync API, but I have some PoC code for per-controller COs here: #2884

@Be-ing
Copy link
Contributor

Be-ing commented Feb 8, 2021

I'm quite concerned about how this PR expands the scope of MidiController which is already overburdened with implementing the legacy mapping system. I started considering how to decouple controller hardware IO from the legacy mapping system and this seems like it might make it more challenging...

@daschuer
Copy link
Member

daschuer commented Feb 8, 2021

The idea of midi clock sync is to have a single clock source for all devices. Normally a sequencer is used or a dedicated device like this: https://www.musicstore.de/de_DE/EUR/E-RM-MIDIclock-/art-SYN0004625-000?campaign=GShopping/DE&ProgramUUID=rrLAqJarLjMAAAFl.gZyjI8h&gclid=EAIaIQobChMI7YzM6ojZ7gIVF-DtCh31ZQaREAQYAiABEgJ2FfD_BwE

@daschuer
Copy link
Member

daschuer commented Feb 8, 2021

Of cause a controller can also do the job. Do we know controllers with this capability? In this case only one is the clock provider. the others are the clock receiver. Mixxx can also become the clock source, but this is probably a bad idea because all the jitter we have until the signal is on the midi line.

@Holzhaus
Copy link
Member

Holzhaus commented Feb 8, 2021

Of cause a controller can also do the job. Do we know controllers with this capability? In this case only one is the clock provider. the others are the clock receiver. Mixxx can also become the clock source, but this is probably a bad idea because all the jitter we have until the signal is on the midi line.

My Roland DJ-505 sends MIDI clock to the computer (for syncing the decks with the built-in TR-S drum computer), and can also receive MIDI clock from the computer to sync the TR-S to a playing deck.

@Holzhaus
Copy link
Member

Holzhaus commented Feb 8, 2021

I think each MIDI device should have the same sync COs as a normal deck. This means that I can either ignore the controller's clock signal, or enable sync lock (i.e. Midi output clock follows sync clock), or even set it as explicit leader (sync clock follows midi input clock). If I have another drum machine plugged into my computer, it should also be possible to switch between the two clock sources (or ignore both) by toggling the sync button on my midi device.

@daschuer
Copy link
Member

daschuer commented Feb 8, 2021

That sounds reasonable, but I think the use case for this PR is the central clock case, where Mixxx is synced to a central clock like any other instrument.

@ywwg ywwg closed this Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants