MIDI Clock Input #702
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,18 @@ | |
| #include "controllers/midi/midiutils.h" | ||
| #include "controllers/defs_controllers.h" | ||
| #include "controlobject.h" | ||
| #include "controlobjectslave.h" | ||
| #include "errordialoghandler.h" | ||
| #include "playermanager.h" | ||
| #include "util/math.h" | ||
|
|
||
| MidiController::MidiController() | ||
| : Controller(), m_midiSourceClock(&m_mixxxClock) { | ||
| setDeviceCategory(tr("MIDI Controller")); | ||
| m_pClockBpm.reset(new ControlObjectSlave("[MidiSourceClock]", "bpm")); | ||
| m_pClockLastBeat.reset( | ||
| new ControlObjectSlave("[MidiSourceClock]", "last_beat_time")); | ||
| m_pClockRunning.reset(new ControlObjectSlave("[MidiSourceClock]", "play")); | ||
|
There was a problem hiding this comment. Just parent the ControlObjectSlave to "this". Qt does the clean up. |
||
| } | ||
|
|
||
| MidiController::~MidiController() { | ||
|
|
@@ -193,6 +198,15 @@ QString formatMidiMessage(unsigned char status, unsigned char control, unsigned | |
| QString::number((status & 255)>>4, 16).toUpper(), | ||
| QString::number(control, 16).toUpper().rightJustified(2,'0'), | ||
| QString::number(value, 16).toUpper().rightJustified(2,'0')); | ||
| case MIDI_START: | ||
| return QString("MIDI status 0x%1: Start Sequence") | ||
| .arg(QString::number(status, 16).toUpper()); | ||
| case MIDI_TIMING_CLK: | ||
|
There was a problem hiding this comment. MIDI_TIMING_CLK: What's that? What about "MIDI_TICK"? There was a problem hiding this comment. IMHO we should unify the wording towards the Midi standard and replace all "pulse" and "tick" with just There was a problem hiding this comment. This is the name of the message in the midi spec: https://www.cs.cf.ac.uk/Dave/Multimedia/node158.html "Timing Clock: F8". Note that midi has more than one timing mechanism, so it's important to stay close to the spec instead of inventing our own terminology. https://www.google.com/search?client=ubuntu&channel=fs&q=midi+timing+clock+f8&ie=utf-8&oe=utf-8 |
||
| return QString("MIDI status 0x%1: Clock Tick") | ||
|
There was a problem hiding this comment. "MIDI status 0x%1: Timing Clock" |
||
| .arg(QString::number(status, 16).toUpper()); | ||
| case MIDI_STOP: | ||
| return QString("MIDI status 0x%1: Stop Sequence") | ||
| .arg(QString::number(status, 16).toUpper()); | ||
| default: | ||
| return QString("MIDI status 0x%1") | ||
| .arg(QString::number(status, 16).toUpper()); | ||
|
|
@@ -245,6 +259,15 @@ void MidiController::receive(unsigned char status, unsigned char control, | |
| qDebug() << formatMidiMessage(status, control, value, channel, opCode); | ||
| } | ||
|
|
||
| // If MidiSourceClock handles the message, record the updated values and | ||
| // no further action is needed. | ||
| if (m_midiSourceClock.handleMessage(status)) { | ||
| m_pClockBpm->set(m_midiSourceClock.bpm()); | ||
| m_pClockLastBeat->set(m_midiSourceClock.lastBeatTime()); | ||
| m_pClockRunning->set(static_cast<double>(m_midiSourceClock.running())); | ||
| return; | ||
| } | ||
|
|
||
| MidiKey mappingKey(status, control); | ||
|
|
||
| if (isLearning()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| #include "controllers/midi/midimessage.h" | ||
| #include "controllers/midi/midisourceclock.h" | ||
| #include "util/math.h" | ||
|
|
||
| bool MidiSourceClock::handleMessage(unsigned char status) { | ||
| switch (status) { | ||
| case MIDI_START: | ||
|
There was a problem hiding this comment. to be complete, we need to support MIDI_CONTINUE There was a problem hiding this comment. supporting midi_continue is going to be tricky because right now the midisourceclock handles bpm calculation, but beat fraction information is handled by the calling code. It will take some work to figure out how to freeze the data, then thaw it, because all the timing is based on an absolute clock. For now this will be a todo. Since this code won't be exposed in Mixxx, I think it's safe to have it be somewhat feature incomplete. |
||
| start(); | ||
| return true; | ||
| case MIDI_STOP: | ||
| stop(); | ||
| return true; | ||
| case MIDI_TIMING_CLK: | ||
| tick(); | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| void MidiSourceClock::start() { | ||
| m_bRunning = true; | ||
| m_iFilled = 0; | ||
| m_iRingBufferPos = 0; | ||
|
There was a problem hiding this comment. Can we consider the first clock tick after start a bar? I think yes. We should drop a note about it. There was a problem hiding this comment. Yes! Here it is:
There was a problem hiding this comment. Please drop a comment about that. |
||
| } | ||
|
|
||
| void MidiSourceClock::stop() { | ||
| m_bRunning = false; | ||
| } | ||
|
|
||
| void MidiSourceClock::tick() { | ||
|
There was a problem hiding this comment. At least the Interface should be prepared to receive a time stamp. There was a problem hiding this comment. That would be a different implementation, so it would be done in a separate class, and we could update the API at that time with minimal refactoring. No need to engage in speculative coding. |
||
| // Update the ring buffer and calculate new bpm. Update the last beat time | ||
| // if we are on a beat. | ||
|
|
||
| if (!m_bRunning) { | ||
| qDebug() << "MidiSourceClock: Got clock tick but not started, starting now."; | ||
| start(); | ||
| } | ||
|
|
||
| // Ringbuffer filling. | ||
| // TODO(owen): We should have a ringbuffer convenience class. | ||
| const qint64 lastTickTime = m_pClock->now(); | ||
| m_iTickRingBuffer[m_iRingBufferPos] = lastTickTime; | ||
| m_iRingBufferPos = (m_iRingBufferPos + 1) % kRingBufferSize; | ||
| if (m_iFilled < kRingBufferSize) { | ||
| ++m_iFilled; | ||
| } | ||
|
|
||
| // If this tick is a beat mark, record it, even if we have very few samples. | ||
| if (m_iRingBufferPos % kPulsesPerQuarter == 0) { | ||
| m_iLastBeatTime = lastTickTime; | ||
| } | ||
|
There was a problem hiding this comment. Why is this thick a bar? Is there any rule inside the Midi standard to know which clock signal is really the bar? There was a problem hiding this comment.
what? The midi standard does not communicate measures. Just 24 ticks per quarter. Note that the standard says "quarter", and in music, a "quarter note" does not imply 4/4 time. I'm ok changing the word to "beat" but "PPQ" -- Pulses Per Quarter, is the industry standard term and I think we should stay with the standard: https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=midi+ppq There was a problem hiding this comment.
Yes, I just wonder if there is a quasi standard for measures since all connected devices have to map their measures to the midi clock ticks. According to https://en.wikipedia.org/wiki/Tempo Quaters per minute is the same as beats per minute. Quarter would be a new term in the Mixxx syncing code so I prefer to stick at "beat". There was a problem hiding this comment. This makes the wording issue worse:
But since we do not use the Midi beats, we can Ignore it. There was a problem hiding this comment. This is midi code so I will to stick to midi teminology |
||
|
|
||
| // Figure out the bpm if we have enough samples. | ||
| if (m_iFilled > 2) { | ||
| qint64 earlyTickTime = 0; | ||
| if (m_iFilled < kRingBufferSize) { | ||
| earlyTickTime = m_iTickRingBuffer[0]; | ||
| } else { | ||
| // In a filled ring buffer, the earliest tick is the next one that | ||
| // will get filled. | ||
| earlyTickTime = m_iTickRingBuffer[m_iRingBufferPos]; | ||
| } | ||
| m_dBpm = calcBpm(earlyTickTime, lastTickTime, m_iFilled); | ||
| } | ||
|
There was a problem hiding this comment. This mean value calculation kills all abrupt tempo changes. There was a problem hiding this comment. Not really. Midi is not precise enough to handle truly abrupt changes, and midi gear often gets out of sync when the tempo changes quickly, so that behavior is expected. Just google "midi sync problems" sometime. It's an old, imprecise standard. There was a problem hiding this comment. Thank you for you explanations. Can you add that as comment? |
||
| } | ||
|
|
||
| // static | ||
| double MidiSourceClock::calcBpm(qint64 early_tick, qint64 late_tick, | ||
| int tick_count) { | ||
| // Get the elapsed time between the latest tick and the earliest tick | ||
| // and divide by the number of ticks in the buffer to get bpm. | ||
|
|
||
| // If we have too few samples, we can't calculate a bpm, so return 0.0. | ||
| DEBUG_ASSERT_AND_HANDLE(tick_count >= 2) { | ||
| qWarning() << "MidiSourceClock::calcBpm called with too few ticks"; | ||
| return 0.0; | ||
| } | ||
|
|
||
| DEBUG_ASSERT_AND_HANDLE(late_tick >= early_tick) { | ||
| qWarning() << "MidiSourceClock asked to calculate beat percentage but " | ||
| << "late_tick < early_tick:" << late_tick << early_tick; | ||
| return 0.0; | ||
| } | ||
|
|
||
| const double elapsed_mins = static_cast<double>(late_tick - early_tick) | ||
| / (60.0 * 1e9); | ||
|
|
||
| // We subtract one since two time values denote a single span of time -- | ||
| // so a filled value of 3 indicates 2 tick periods, etc. | ||
| const double bpm = static_cast<double>(tick_count - 1) / kPulsesPerQuarter | ||
|
There was a problem hiding this comment. A Pentium can handle Mixed type calculations. No need for a cast. There was a problem hiding this comment. both numberator and denominator are ints, so cast is required I think. Only last divisor is double. |
||
| / elapsed_mins; | ||
|
|
||
| return math_clamp(bpm, kMinMidiBpm, kMaxMidiBpm); | ||
|
There was a problem hiding this comment. Clamping is always wrong here. Can we return just 0 and let the calling code decide? |
||
| } | ||
|
|
||
| // static | ||
| double MidiSourceClock::beatPercentage(const qint64 last_beat, const qint64 now, | ||
| const double bpm) { | ||
| DEBUG_ASSERT_AND_HANDLE(now >= last_beat) { | ||
| qWarning() << "MidiSourceClock asked to calculate beat percentage but " | ||
| << "now < last_beat:" << now << last_beat; | ||
| return 0.0; | ||
|
There was a problem hiding this comment. we can make a guess, but by the API I've written this situation is invalid and indicates a bad call upstream. |
||
| } | ||
| // Get seconds per beat. | ||
| const double beat_length = 60.0 / bpm; | ||
|
|
||
| // seconds / secondsperbeat = percentage of beat. | ||
| const double beat_percent = static_cast<double>(now - last_beat) / 1e9 | ||
| / beat_length; | ||
| // Ensure values are < 1.0. | ||
| return beat_percent - floor(beat_percent); | ||
| } | ||
|
|
||
| double MidiSourceClock::beatPercentage() const { | ||
| return beatPercentage(m_iLastBeatTime, m_pClock->now(), m_dBpm); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
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?