Merge lp:~mixxxdevelopers/mixxx/features_scriptTimers into lp:~mixxxdevelopers/mixxx/trunk
- features_scriptTimers
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~mixxxdevelopers/mixxx/features_scriptTimers |
Merge into: | lp:~mixxxdevelopers/mixxx/trunk |
Diff against target: |
893 lines (+406/-132) 10 files modified
mixxx/res/midi/midi-mappings-scripts.js (+1/-1) mixxx/src/main.cpp (+4/-0) mixxx/src/midi/mididevice.cpp (+4/-5) mixxx/src/midi/mididevice.h (+2/-0) mixxx/src/midi/midimapping.cpp (+43/-45) mixxx/src/midi/midimapping.h (+7/-2) mixxx/src/midi/midimessage.cpp (+1/-0) mixxx/src/midi/midiscriptengine.cpp (+325/-63) mixxx/src/midi/midiscriptengine.h (+19/-11) mixxx/src/mixxx.cpp (+0/-5) |
To merge this branch: | bzr merge lp:~mixxxdevelopers/mixxx/features_scriptTimers |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
RJ Skerry-Ryan | Approve | ||
Albert Santoni | Needs Fixing | ||
Sean M. Pappalardo | Needs Resubmitting | ||
Review via email: mp+17850@code.launchpad.net |
Commit message
Description of the change
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
RJ Skerry-Ryan (rryan) wrote : | # |
Hey Sean,
Thanks for working on this. It looks like a feature that will provide needed flexibility to script writers. I've looked it over and I have a few comments.
On the primer timer:
- Why did you pick 20ms? Will a slower timer serve the same purpose of making the other timers work? (e.g. can we set the primer timer to something like an hour? a day?)
- Did you ever discover why startTimer() stops working unless the primer timer is created? It seems like this is either a bug in Qt or a bug in our code.
Honestly this is the most concerning part of this branch and I think it needs fixing. This means that a high resolution timer will always be executing for every MIDI device you plug in (one per script engine) even if your scripts aren't using timers (high-or-low resolution) at all.
First off, the approach is very clean (other than the primer timer) -- which I do like. I am concerned that a script could bring down Mixxx by creating 100 high resolution timers. The script engine thread could strangle the engine. (Also, MidiScriptEngine should set its thread priority lower.) We already have bad thread contention on single-CPU machines so this makes me worry. When you were implementing this design, did you put any thought into the alternate (less clean) approach we discussed where you would create one Qt script timer and run the script timers on that main timer? You would just keep track of the last time each timer got triggered and trigger the timer if the current time is greater than the previous time plus the timer's interval. What made you go with this approach rather than that one?
- Do you have any profiling output to support that this approach is not taxing?
- Have you experimented with malicious (or dumb) script writing to figure out just how many timers your machine can support before Mixxx performance is degraded? I'm very interested to see data like this for single-CPU low-power machines.
I also have some minor code comments:
~MidiScriptEngine()
- When calling stopTimer(), use a QMutableHashIte
beginTimer()
- No bounds checking on the 'interval' variable (should at least be non-zero). Have you given any thought to preventing scripts from making high-resolution timers? (e.g. sub-10ms) I don't think that a script should be able to affect Mixxx's performance.
stopTimer()
- No validity checking on timerId variable. (Should check that the hash contains the timerId key first before calling stopTimer -- Qt doesn't specify its behavior if the timer id you pass is invalid, so you can only assume it crashes or asserts).
timerEvent()
- Please check that event.timerId() is in m_timers before indexing into the hash (would cause an assert and kill Mixxx). Suppose killTimer has a delayed effect or something and a timerEvent for a timer that was just killTimer'd can occur (Qt doesn't make any promises like this). You could run into a situation where the timer id is not in the hash. Better safe than sorry.
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
> On the primer timer:
> - Why did you pick 20ms? Will a slower timer serve the same purpose of making
> the other timers work? (e.g. can we set the primer timer to something like an
> hour? a day?)
No, since timers set up before exec() apparently block any other timers, forcing them to fire only up to as often as the shortest pre-exec() one. Also 20ms is the highest resolution that most platforms support. (Linux apparently supports higher, but I noticed a ~10% CPU usage increase at idle when I had it fire as often as possible (interval 0) on this 2.53GHz Celeron D.)
> - Did you ever discover why startTimer() stops working unless the primer timer
> is created? It seems like this is either a bug in Qt or a bug in our code.
Our code is straightforward. Unless the event loop/thread is somehow being set up incorrectly, I'm thinking it's Qt.
> Honestly this is the most concerning part of this branch and I think it needs
> fixing. This means that a high resolution timer will always be executing for
> every MIDI device you plug in (one per script engine) even if your scripts
> aren't using timers (high-or-low resolution) at all.
I agree. It bothers me a lot.
> First off, the approach is very clean (other than the primer timer) -- which I
> do like. I am concerned that a script could bring down Mixxx by creating 100
> high resolution timers. The script engine thread could strangle the engine.
That's possible, though Qt will refuse to set up a timer if the OS has reached its limit. But how do we decide what an acceptable upper bound is without limiting future expandability?
> (Also, MidiScriptEngine should set its thread priority lower.)
Done, set to QThread:
> When you
> were implementing this design, did you put any thought into the alternate
> (less clean) approach we discussed where you would create one Qt script timer
> and run the script timers on that main timer?
Not really. I just wanted to see if the simplest design would work acceptably, and it does! :) Though I don't see how doing what you suggest would help much since you'd still need a high resolution timer running all the time.
> You would just keep track of the
> last time each timer got triggered and trigger the timer if the current time
> is greater than the previous time plus the timer's interval. What made you go
> with this approach rather than that one?
Well, the OS already handles that, (and probably better than I would) so why do the work again? And like I said above, I wanted to try the simplest method first (not knowing about the need for this primer timer.) That and QBasicTimer/QTimer operate using the same event loop mechanism. They just make timer management more convenient, at the cost of some overhead.
> - Do you have any profiling output to support that this approach is not
> taxing?
It IS taxing if the primer timer runs any more often. (I saw a 10% CPU increase on this 2.53GHz Celeron-D when I tried the primer at interval 0 (run every time there are no more events in the queue.)) That said, running it at 20ms caused only a slight aver...
Adam Davison (adamdavison) wrote : | # |
This is mostly not my area of expertise but Sean I have one question. You mention that this is required for good scratching. Can you explain why this is the case? Surely ultimately for high performance scratching you want up to ~1ms precision which as you mention, operating system timers cannot provide. And in fact you aren't sampling the "velocity" of the midi wheels, you're getting discrete delta position numbers from the midi controller. Surely the best approach is to process these scratch deltas as they arrive in some way, not as part of a timer loop which necessarily introduces additional latency?
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
> This is mostly not my area of expertise but Sean I have one question. You
> mention that this is required for good scratching. Can you explain why this is
> the case?
I mean it's required in order to continue to do it in script. Ultimately we'll be doing it in the Engine, but that's a ways off and I'd like to get some improvement on this into 1.8.
> And in fact you aren't sampling the "velocity" of the midi wheels, you're
> getting discrete delta position numbers from the midi controller. Surely the
> best approach is to process these scratch deltas as they arrive in some way,
That's exactly my plan, using the alpha-beta filter code already in scratch.* script functions. But an alpha-beta filter is a time-sensitive one and without interval-based sampling, there is no way to tell it the wheel has stopped, so it's starved of data in that case instead of realizing that it needs to rapidly decelerate. (That's why in 1.7 it acts so screwy when you just hold your finger in one spot on the SCS.3d's red vinyl mode.)
Adam Davison (adamdavison) wrote : | # |
> > And in fact you aren't sampling the "velocity" of the midi wheels, you're
> > getting discrete delta position numbers from the midi controller. Surely the
> > best approach is to process these scratch deltas as they arrive in some way,
>
> That's exactly my plan, using the alpha-beta filter code already in scratch.*
> script functions. But an alpha-beta filter is a time-sensitive one and without
> interval-based sampling, there is no way to tell it the wheel has stopped, so
> it's starved of data in that case instead of realizing that it needs to
> rapidly decelerate. (That's why in 1.7 it acts so screwy when you just hold
> your finger in one spot on the SCS.3d's red vinyl mode.)
Ok, so the timer basically just provides some kind of nothing-
Thanks,
Adam
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
Resubmitting for review.
I discovered that the primer timer was a hack and the real problem is that the MidiDevice thread was executing all of the script functions, including timer creation! (Timers must be created in the thread in which they're to fire, i.e. the MidiScriptEngine thread.)
I've untangled all of that and now the MidiScriptEngine takes care of loading, initializing, and shutting down all MIDI scripts, as it should. It and the MidiMapping communicate via signals so things happen in the correct order (the MidiMapping tells the MidiScriptEngine to do one of those tasks then waits on its signal to continue.) Timers now work perfectly as originally expected on Linux and Windows with no appreciable CPU increase (unless you run 3000 timers at once...which I tried, and Mixxx is still responsive up but you notice it going slower. :) )
(This should also help resolve any strange script engine crashes we'd seen in bug reports.)
RJ Skerry-Ryan (rryan) wrote : | # |
Looking good Sean. Glad to see the primer timer is gone. I have a few comments / fixes.
MidiScriptEngin
- Passing references to m_scriptFileNames and m_scriptFunctio
good. They are QList's, which are not thread safe. Two threads accessing a
list at the same time will segfault. You should make these two lists arguments
to the signals that deal with them and pass by value, not reference.
- On the diversity of callMidiScriptF
multiple parameters: In the end we just make a QScriptValueList and we don't
care about the individual types of the parameters. I say we just accept one
signal for executing a script function: callMidiScriptF
QList<QVariant> arguments) and consolidate all the execute(...) methods into
one execute(QString function, QList<QVariant> arguments); method.
MidiScriptEngin
- Need to hold the engine lock before calling m_connectedCont
MidiScriptEngin
- As I mentioned, convert this to take m_rScriptFileNames as an argument instead
of sharing it between the classes. Is there any problem I'm not seeing with doing it this way?
MidiScriptEngin
- Please add comment to the top that the caller should hold the script lock.
MidiScriptEngin
- Need to hold the lock to user m_timers. I know that this is technically safe,
we're just going for overkill here :).
Overall
- qWarnings and qCritical - I really don't think these should anywhere in the
script engine. Maybe it's ok for a script function that only happens on
initialization -- but in the end no DJ cares about a syntax error more than
s/he cares about Mixxx not going down in the middle of their set.
- 2273. By Sean M. Pappalardo
-
Addressed RJ's code review concerns:
- got rid of the reference passing between threads and replaced with value passing on signals (it's actually neater this way too)
- moved the lock in gracefulShutdown() to include clearing m_connectedControls
- added lock assert to stopAllTimers() too
- moved the lock in timerEvent to include m_timers[] access - 2274. By Sean M. Pappalardo
-
- Made the midiDebug flag global in the MidiScriptEngine instead of asking the MidiDevice for it each time
- Replaced obsolete canEvaluate() with checkSyntax()
- Mixxx now shows friendly warning dialogs and no longer aborts on script errors (unless run with --midiDebug)
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
Another resubmit.
> - Passing references to m_scriptFileNames and m_scriptFunctio
> good.
Fixed.
> I say we just accept one
> signal for executing a script function: callMidiScriptF
> QList<QVariant> arguments) and consolidate all the execute(...) methods into
> one execute(QString function, QList<QVariant> arguments); method.
I agree, though right now the only thing that signals the MidiScriptEngine is MidiMapping and the existing two signals are all it needs. So I'm thinking we should defer this consolidation unless/until it ever becomes an issue. (Also, can you even send null QLists for situations where we want to execute a parameter-less function or a non-function piece of script code?)
> MidiScriptEngin
> - Need to hold the engine lock before calling m_connectedCont
Fixed.
> MidiScriptEngin
> - As I mentioned, convert this to take m_rScriptFileNames as an argument
> instead of sharing it between the classes.
Done, works fine.
> MidiScriptEngin
> - Please add comment to the top that the caller should hold the script lock.
Did you one better and asserted that the lock is held (kind of redundant since stopTimers() also checks, but whatever.)
> MidiScriptEngin
> - Need to hold the lock to user m_timers.
Fixed.
> - qWarnings and qCritical - I really don't think these should anywhere in the
> script engine. Maybe it's ok for a script function that only happens on
> initialization -- but in the end no DJ cares about a syntax error more than
> s/he cares about Mixxx not going down in the middle of their set.
Agreed. I examined the various things that were qCriticals and discovered that QScriptEngine:
One thing to note about allowing the user to continue on a script error: the offending function aborts on the statement that caused the exception, so the rest of the function doesn't execute and could leave the controller and script global variables in an indeterminate state (for example a mode change could have happened but the LEDs didn't get updated to show it.)
- 2275. By Sean M. Pappalardo
-
Merging from trunk
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
Resubmitting after trunk merge (seeing if it realizes I resolved the SConscript "conflict.")
Also, confirmed that the fixes in this branch eliminate the segfault-on-close 90% of the time in Windows resulting from a qWarning trying to appear to tell us of a race condition "[Main]: QWaitCondition: Destroyed while threads are still waiting." This must've been related to MidiScriptEngine threads since it doesn't happen in this branch.
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
One tester confirmed that this appears to indeed fix a scriptEngine crash from 1.7, though I'd have to back-port to be sure. I'm not sure that's worth the time though. https:/
- 2276. By Sean M. Pappalardo
-
- Removed _exit() hack for Windows
- Added "shutdown complete" message for closure - 2277. By Sean M. Pappalardo
-
Updated from trunk
Sean M. Pappalardo (pegasus-renegadetech) wrote : | # |
- 2278. By Sean M. Pappalardo
-
Merging from trunk
- 2279. By Sean M. Pappalardo
-
Merging from trunk
- 2280. By Sean M. Pappalardo
-
Mo' comments
- 2281. By Sean M. Pappalardo
-
Merging from trunk again
Albert Santoni (gamegod) wrote : | # |
> Another resubmit.
>
> > - Passing references to m_scriptFileNames and m_scriptFunctio
> not
> > good.
>
> Fixed.
>
> > I say we just accept one
> > signal for executing a script function: callMidiScriptF
> name,
> > QList<QVariant> arguments) and consolidate all the execute(...) methods
> into
> > one execute(QString function, QList<QVariant> arguments); method.
>
> I agree, though right now the only thing that signals the MidiScriptEngine is
> MidiMapping and the existing two signals are all it needs. So I'm thinking we
> should defer this consolidation unless/until it ever becomes an issue. (Also,
> can you even send null QLists for situations where we want to execute a
> parameter-less function or a non-function piece of script code?)
>
> > MidiScriptEngin
> > - Need to hold the engine lock before calling m_connectedCont
>
> Fixed.
>
> > MidiScriptEngin
> > - As I mentioned, convert this to take m_rScriptFileNames as an argument
> > instead of sharing it between the classes.
>
> Done, works fine.
>
> > MidiScriptEngin
> > - Please add comment to the top that the caller should hold the script lock.
>
> Did you one better and asserted that the lock is held (kind of redundant since
> stopTimers() also checks, but whatever.)
>
> > MidiScriptEngin
> > - Need to hold the lock to user m_timers.
>
> Fixed.
>
> > - qWarnings and qCritical - I really don't think these should anywhere in
> the
> > script engine. Maybe it's ok for a script function that only happens on
> > initialization -- but in the end no DJ cares about a syntax error more
> than
> > s/he cares about Mixxx not going down in the middle of their set.
>
> Agreed. I examined the various things that were qCriticals and discovered that
> QScriptEngine:
> removed. I added the checkSyntax() system in their place and I have it pop up
> qCriticals with the error details only if --midiDebug is enabled, otherwise it
> qDebugs the details and pops up a friendly qWarning which the user can
> acknowledge and continue to mix (though with the controller in a degraded
> state.) As for loops, only the first warning dialog comes up; all others are
> supressed until the first is acknowledged. (So the user will see one each time
> he does something that calls an offending function.) However, if the syntax
> error is discovered on file load, that warning dialog shows up only once (each
> time the script file is (re)loaded) and informs the user that the offending
> script will be disabled.
>
> One thing to note about allowing the user to continue on a script error: the
> offending function aborts on the statement that caused the exception, so the
> rest of the function doesn't execute and could leave the controller and script
> global variables in an indeterminate state (for example a mode change could
> have happened but the LEDs didn't get updated to show it.)
Sounds like you should give an option to reload the script when there's an exception. I'd rather have my controller lose whatever mode it's...
Albert Santoni (gamegod) wrote : | # |
How should I be testing this code? The scratching on the SCS.3d seems asymmetric to me in the branch, as in stabbing forwards and backwards go at different rates.
There's a lot of great code here, I think you've done a great job with the script engine overall and you should be very proud of it. :)
The only inconsistency in the code I've found (so far) is in MidiScriptEngin
Otherwise, looking good to me...
Thanks!
Albert
RJ Skerry-Ryan (rryan) wrote : | # |
I agree with Albert's timerEvent comment. I committed a fix and I re-reviewed the code. It all look good to me, so I'm marking approved. I'll let you do the honors Sean :).
- 2282. By RJ Skerry-Ryan
-
Move lock in MidiScriptEngin
e::timerEvent to resolve thread-safety issue.
Preview Diff
1 | === modified file 'mixxx/res/midi/midi-mappings-scripts.js' |
2 | --- mixxx/res/midi/midi-mappings-scripts.js 2010-03-06 21:32:16 +0000 |
3 | +++ mixxx/res/midi/midi-mappings-scripts.js 2010-03-24 04:40:48 +0000 |
4 | @@ -239,7 +239,7 @@ |
5 | |
6 | // The actual alpha-beta filter |
7 | scratch.filter = function (currentDeck, controlValue, revtime, alpha, beta) { |
8 | - // ------------- Thanks to Radiomark (of Xwax) for the info for below ------------------------ |
9 | + // ------------- Thanks to Mark Hills of Xwax (http://www.xwax.co.uk) for the info for below ------------------------ |
10 | |
11 | // ideal position = (initial_p + (y - x) / 128 * 1.8) |
12 | var ideal_p = scratch.variables["initialTrackPos"] + (controlValue - scratch.variables["initialControlValue"]) / 128 * revtime; |
13 | |
14 | === modified file 'mixxx/src/main.cpp' |
15 | --- mixxx/src/main.cpp 2010-03-11 12:14:05 +0000 |
16 | +++ mixxx/src/main.cpp 2010-03-24 04:40:48 +0000 |
17 | @@ -351,6 +351,10 @@ |
18 | |
19 | delete mixxx; |
20 | |
21 | + qDebug() << "Mixxx shutdown complete."; |
22 | + |
23 | + // Don't make any more output after this |
24 | + // or mixxx.log will get clobbered! |
25 | if(Logfile.isOpen()) |
26 | Logfile.close(); |
27 | |
28 | |
29 | === modified file 'mixxx/src/midi/mididevice.cpp' |
30 | --- mixxx/src/midi/mididevice.cpp 2009-11-26 06:15:01 +0000 |
31 | +++ mixxx/src/midi/mididevice.cpp 2010-03-24 04:40:48 +0000 |
32 | @@ -210,11 +210,10 @@ |
33 | //to the device. (sendShortMessage() would try to lock m_mutex...) |
34 | locker.unlock(); |
35 | |
36 | - if (!m_pMidiMapping->getMidiScriptEngine()->execute(configKey.item, channel, |
37 | - control, value, status, |
38 | - mixxxControl.getControlObjectGroup())) { |
39 | - qDebug() << "MidiDevice: Invalid script function" << configKey.item; |
40 | - } |
41 | + // This needs to be a signal because the MIDI Script Engine thread must execute |
42 | + // script functions, not this MidiDevice one |
43 | + emit(callMidiScriptFunction(configKey.item, channel, control, value, status, |
44 | + mixxxControl.getControlObjectGroup())); |
45 | return; |
46 | } |
47 | #endif |
48 | |
49 | === modified file 'mixxx/src/midi/mididevice.h' |
50 | --- mixxx/src/midi/mididevice.h 2009-11-26 06:15:01 +0000 |
51 | +++ mixxx/src/midi/mididevice.h 2010-03-24 04:40:48 +0000 |
52 | @@ -71,6 +71,8 @@ |
53 | |
54 | signals: |
55 | void midiEvent(MidiMessage message); |
56 | + void callMidiScriptFunction(QString function, char channel, char control, char value, MidiStatusByte status, QString group); |
57 | + |
58 | protected: |
59 | /** Verbose device name, in format "[index]. [device name]". Suitable for display in GUI. */ |
60 | QString m_strDeviceName; |
61 | |
62 | === modified file 'mixxx/src/midi/midimapping.cpp' |
63 | --- mixxx/src/midi/midimapping.cpp 2010-01-05 03:05:02 +0000 |
64 | +++ mixxx/src/midi/midimapping.cpp 2010-03-24 04:40:48 +0000 |
65 | @@ -47,6 +47,8 @@ |
66 | qDebug() << "Creating new MIDI presets directory" << BINDINGS_PATH; |
67 | QDir().mkpath(BINDINGS_PATH); |
68 | } |
69 | + // So we can signal the MidiScriptEngine and pass a QList |
70 | + qRegisterMetaType<QList<QString> >("QList<QString>"); |
71 | |
72 | //Q_ASSERT(outputMidiDevice); |
73 | |
74 | @@ -85,6 +87,7 @@ |
75 | |
76 | m_pScriptEngine->moveToThread(m_pScriptEngine); |
77 | |
78 | + // Let the script engine tell us when it's done with each task |
79 | connect(m_pScriptEngine, SIGNAL(initialized()), |
80 | this, SLOT(slotScriptEngineReady()), |
81 | Qt::DirectConnection); |
82 | @@ -94,58 +97,53 @@ |
83 | m_scriptEngineInitializedCondition.wait(&m_scriptEngineInitializedMutex); |
84 | m_scriptEngineInitializedMutex.unlock(); |
85 | |
86 | + // Allow this object to signal the MidiScriptEngine to load the list of script files |
87 | + connect(this, SIGNAL(loadMidiScriptFiles(QList<QString>)), m_pScriptEngine, SLOT(loadScriptFiles(QList<QString>))); |
88 | + // Allow this object to signal the MidiScriptEngine to run the initialization |
89 | + // functions in the loaded scripts |
90 | + connect(this, SIGNAL(initMidiScripts(QList<QString>)), m_pScriptEngine, SLOT(initializeScripts(QList<QString>))); |
91 | + // Allow this object to signal the MidiScriptEngine to run its shutdown routines |
92 | + connect(this, SIGNAL(shutdownMidiScriptEngine(QList<QString>)), m_pScriptEngine, SLOT(gracefulShutdown(QList<QString>))); |
93 | + |
94 | + // Allow this object to signal the MidiScriptEngine to call functions |
95 | + connect(this, SIGNAL(callMidiScriptFunction(QString)), |
96 | + m_pScriptEngine, SLOT(execute(QString))); |
97 | + connect(this, SIGNAL(callMidiScriptFunction(QString, QString)), |
98 | + m_pScriptEngine, SLOT(execute(QString, QString))); |
99 | } |
100 | |
101 | void MidiMapping::loadScriptCode() { |
102 | QMutexLocker Locker(&m_mappingLock); |
103 | - |
104 | - ConfigObject<ConfigValue> *config = new ConfigObject<ConfigValue>(QDir::homePath().append("/").append(SETTINGS_PATH).append(SETTINGS_FILE)); |
105 | - |
106 | - qDebug() << "MidiMapping: Loading & evaluating all MIDI script code"; |
107 | - |
108 | - QListIterator<QString> it(m_pScriptFileNames); |
109 | - while (it.hasNext()) { |
110 | - QString curScriptFileName = it.next(); |
111 | - m_pScriptEngine->evaluate(config->getConfigPath().append("midi/").append(curScriptFileName)); |
112 | - |
113 | - if(m_pScriptEngine->hasErrors(curScriptFileName)) { |
114 | - qDebug() << "Errors occured while loading " << curScriptFileName; |
115 | - } |
116 | + if(m_pScriptEngine) { |
117 | + m_scriptEngineInitializedMutex.lock(); |
118 | + // Tell the script engine to run the init function in all loaded scripts |
119 | + emit(loadMidiScriptFiles(m_scriptFileNames)); |
120 | + // Wait until it's done |
121 | + m_scriptEngineInitializedCondition.wait(&m_scriptEngineInitializedMutex); |
122 | + m_scriptEngineInitializedMutex.unlock(); |
123 | } |
124 | } |
125 | |
126 | void MidiMapping::initializeScripts() { |
127 | QMutexLocker Locker(&m_mappingLock); |
128 | if(m_pScriptEngine) { |
129 | - // Call each script's init function if it exists |
130 | - QListIterator<QString> prefixIt(m_pScriptFunctionPrefixes); |
131 | - while (prefixIt.hasNext()) { |
132 | - QString initName = prefixIt.next(); |
133 | - if (initName!="") { |
134 | - initName.append(".init"); |
135 | - qDebug() << "MidiMapping: Executing" << initName; |
136 | - if (!m_pScriptEngine->execute(initName, m_deviceName)) |
137 | - qWarning() << "MidiMapping: No" << initName << "function in script"; |
138 | - } |
139 | - } |
140 | - } |
141 | + m_scriptEngineInitializedMutex.lock(); |
142 | + // Tell the script engine to run the init function in all loaded scripts |
143 | + emit(initMidiScripts(m_scriptFunctionPrefixes)); |
144 | + // Wait until it's done |
145 | + m_scriptEngineInitializedCondition.wait(&m_scriptEngineInitializedMutex); |
146 | + m_scriptEngineInitializedMutex.unlock(); |
147 | + } |
148 | } |
149 | |
150 | void MidiMapping::shutdownScriptEngine() { |
151 | QMutexLocker Locker(&m_mappingLock); |
152 | if(m_pScriptEngine) { |
153 | - // Call each script's shutdown function if it exists |
154 | - QListIterator<QString> prefixIt(m_pScriptFunctionPrefixes); |
155 | - while (prefixIt.hasNext()) { |
156 | - QString shutName = prefixIt.next(); |
157 | - if (shutName!="") { |
158 | - shutName.append(".shutdown"); |
159 | - qDebug() << "MidiMapping: Executing" << shutName; |
160 | - if (!m_pScriptEngine->execute(shutName)) |
161 | - qWarning() << "MidiMapping: No" << shutName << "function in script"; |
162 | - } |
163 | - } |
164 | - qDebug() << "MidiMapping: Deleting MIDI script engine..."; |
165 | + // Tell the script engine to do its shutdown sequence |
166 | + emit(shutdownMidiScriptEngine(m_scriptFunctionPrefixes)); |
167 | + // ...and wait for it to finish |
168 | + m_pScriptEngine->wait(); |
169 | + |
170 | MidiScriptEngine *engine = m_pScriptEngine; |
171 | m_pScriptEngine = NULL; |
172 | delete engine; |
173 | @@ -484,8 +482,8 @@ |
174 | -------- ------------------------------------------------------ */ |
175 | void MidiMapping::addScriptFile(QString filename, QString functionprefix) { |
176 | QMutexLocker Locker(&m_mappingLock); |
177 | - m_pScriptFileNames.append(filename); |
178 | - m_pScriptFunctionPrefixes.append(functionprefix); |
179 | + m_scriptFileNames.append(filename); |
180 | + m_scriptFunctionPrefixes.append(functionprefix); |
181 | } |
182 | #endif |
183 | |
184 | @@ -539,8 +537,8 @@ |
185 | m_mappingLock.lock(); |
186 | |
187 | #ifdef __MIDISCRIPT__ |
188 | - m_pScriptFileNames.clear(); |
189 | - m_pScriptFunctionPrefixes.clear(); |
190 | + m_scriptFileNames.clear(); |
191 | + m_scriptFunctionPrefixes.clear(); |
192 | #endif |
193 | |
194 | // For each controller in the DOM |
195 | @@ -753,15 +751,15 @@ |
196 | controller.appendChild(scriptFiles); |
197 | |
198 | |
199 | - for (int i = 0; i < m_pScriptFileNames.count(); i++) { |
200 | -// qDebug() << "MidiMapping: writing script block for" << m_pScriptFileNames[i]; |
201 | - QString filename = m_pScriptFileNames[i]; |
202 | + for (int i = 0; i < m_scriptFileNames.count(); i++) { |
203 | +// qDebug() << "MidiMapping: writing script block for" << m_scriptFileNames[i]; |
204 | + QString filename = m_scriptFileNames[i]; |
205 | |
206 | |
207 | //Don't need to write anything for the required mapping file. |
208 | if (filename != REQUIRED_SCRIPT_FILE) { |
209 | qDebug() << "MidiMapping: writing script block for" << filename; |
210 | - QString functionPrefix = m_pScriptFunctionPrefixes[i]; |
211 | + QString functionPrefix = m_scriptFunctionPrefixes[i]; |
212 | QDomElement scriptFile = doc.createElement("file"); |
213 | |
214 | |
215 | |
216 | === modified file 'mixxx/src/midi/midimapping.h' |
217 | --- mixxx/src/midi/midimapping.h 2009-11-25 02:31:11 +0000 |
218 | +++ mixxx/src/midi/midimapping.h 2010-03-24 04:40:48 +0000 |
219 | @@ -111,6 +111,11 @@ |
220 | void midiLearningStarted(); |
221 | void midiLearningFinished(MidiMessage); |
222 | void midiLearningFinished(); |
223 | + void callMidiScriptFunction(QString); |
224 | + void callMidiScriptFunction(QString, QString); |
225 | + void loadMidiScriptFiles(QList<QString>); |
226 | + void initMidiScripts(QList<QString>); |
227 | + void shutdownMidiScriptEngine(QList<QString>); |
228 | |
229 | private: |
230 | int internalNumInputMidiMessages(); |
231 | @@ -142,8 +147,8 @@ |
232 | /** Actually loads script code from the files in the list */ |
233 | void loadScriptCode(); |
234 | |
235 | - QList<QString> m_pScriptFileNames; |
236 | - QList<QString> m_pScriptFunctionPrefixes; |
237 | + QList<QString> m_scriptFileNames; |
238 | + QList<QString> m_scriptFunctionPrefixes; |
239 | MidiScriptEngine *m_pScriptEngine; |
240 | |
241 | QMutex m_scriptEngineInitializedMutex; |
242 | |
243 | === modified file 'mixxx/src/midi/midimessage.cpp' |
244 | --- mixxx/src/midi/midimessage.cpp 2009-09-01 15:33:21 +0000 |
245 | +++ mixxx/src/midi/midimessage.cpp 2010-03-24 04:40:48 +0000 |
246 | @@ -7,6 +7,7 @@ |
247 | { |
248 | //Register this class with QT so we can use this bad boy in signals/slots. |
249 | qRegisterMetaType<MidiMessage>("MidiMessage"); |
250 | + qRegisterMetaType<MidiStatusByte>("MidiStatusByte"); |
251 | |
252 | m_midiStatusByte = status; |
253 | m_midiNo = midino; |
254 | |
255 | === modified file 'mixxx/src/midi/midiscriptengine.cpp' |
256 | --- mixxx/src/midi/midiscriptengine.cpp 2010-02-22 23:13:52 +0000 |
257 | +++ mixxx/src/midi/midiscriptengine.cpp 2010-03-24 04:40:48 +0000 |
258 | @@ -20,7 +20,7 @@ |
259 | #include "controlobjectthread.h" |
260 | #include "mididevice.h" |
261 | #include "midiscriptengine.h" |
262 | - |
263 | +// #include <QScriptSyntaxCheckResult> |
264 | |
265 | #ifdef _MSC_VER |
266 | #include <float.h> // for _isnan() on VC++ |
267 | @@ -38,16 +38,6 @@ |
268 | |
269 | MidiScriptEngine::~MidiScriptEngine() { |
270 | |
271 | - // Stop processing the event loop and terminate the thread. |
272 | - quit(); |
273 | - |
274 | - // Clear the m_connectedControls hash so we stop responding |
275 | - // to signals. |
276 | - m_connectedControls.clear(); |
277 | - |
278 | - // Wait for the thread to terminate |
279 | - wait(); |
280 | - |
281 | // Delete the script engine, first clearing the pointer so that |
282 | // other threads will not get the dead pointer after we delete it. |
283 | if(m_pEngine != NULL) { |
284 | @@ -56,6 +46,41 @@ |
285 | engine->deleteLater(); |
286 | } |
287 | |
288 | +} |
289 | + |
290 | +/* -------- ------------------------------------------------------ |
291 | +Purpose: Shuts down MIDI scripts in an orderly fashion |
292 | + (stops timers then executes shutdown functions) |
293 | +Input: - |
294 | +Output: - |
295 | +-------- ------------------------------------------------------ */ |
296 | +void MidiScriptEngine::gracefulShutdown(QList<QString> scriptFunctionPrefixes) { |
297 | + qDebug() << "MidiScriptEngine shutting down..."; |
298 | + m_scriptEngineLock.lock(); |
299 | + // Clear the m_connectedControls hash so we stop responding |
300 | + // to signals. |
301 | + m_connectedControls.clear(); |
302 | + |
303 | + // Disconnect the function call signal |
304 | + disconnect(m_pMidiDevice, SIGNAL(callMidiScriptFunction(QString, char, char, |
305 | + char, MidiStatusByte, QString)), |
306 | + this, SLOT(execute(QString, char, char, char, MidiStatusByte, QString))); |
307 | + |
308 | + // Stop all timers |
309 | + stopAllTimers(); |
310 | + |
311 | + // Call each script's shutdown function if it exists |
312 | + QListIterator<QString> prefixIt(scriptFunctionPrefixes); |
313 | + while (prefixIt.hasNext()) { |
314 | + QString shutName = prefixIt.next(); |
315 | + if (shutName!="") { |
316 | + shutName.append(".shutdown"); |
317 | + if (m_midiDebug) qDebug() << "MidiScriptEngine: Executing" << shutName; |
318 | + if (!internalExecute(shutName)) |
319 | + qWarning() << "MidiScriptEngine: No" << shutName << "function in script"; |
320 | + } |
321 | + } |
322 | + |
323 | // Free all the control object threads |
324 | QList<ConfigKey> keys = m_controlCache.keys(); |
325 | QList<ConfigKey>::iterator it = keys.begin(); |
326 | @@ -63,12 +88,14 @@ |
327 | while(it != end) { |
328 | ConfigKey key = *it; |
329 | ControlObjectThread *cot = m_controlCache.take(key); |
330 | - if(cot != NULL) { |
331 | - delete cot; |
332 | - } |
333 | + delete cot; |
334 | it++; |
335 | } |
336 | |
337 | + m_scriptEngineLock.unlock(); |
338 | + |
339 | + // Stop processing the event loop and terminate the thread. |
340 | + quit(); |
341 | } |
342 | |
343 | bool MidiScriptEngine::isReady() { |
344 | @@ -95,11 +122,73 @@ |
345 | // 'engine'. |
346 | QScriptValue engineGlobalObject = m_pEngine->globalObject(); |
347 | engineGlobalObject.setProperty("engine", m_pEngine->newQObject(this)); |
348 | + |
349 | + // Make the MidiDevice instance available to scripts as |
350 | + // 'midi'. |
351 | if (m_pMidiDevice) |
352 | engineGlobalObject.setProperty("midi", m_pEngine->newQObject(m_pMidiDevice)); |
353 | |
354 | -} |
355 | - |
356 | + // Allow the MidiDevice to signal script function calls |
357 | + connect(m_pMidiDevice, SIGNAL(callMidiScriptFunction(QString, char, char, |
358 | + char, MidiStatusByte, QString)), |
359 | + this, SLOT(execute(QString, char, char, char, MidiStatusByte, QString))); |
360 | +} |
361 | + |
362 | +/* -------- ------------------------------------------------------ |
363 | + Purpose: Load all script files given in the shared list |
364 | + Input: - |
365 | + Output: - |
366 | + -------- ------------------------------------------------------ */ |
367 | +void MidiScriptEngine::loadScriptFiles(QList<QString> scriptFileNames) { |
368 | + |
369 | + // Set the Midi Debug flag |
370 | + if (m_pMidiDevice) |
371 | + m_midiDebug = m_pMidiDevice->midiDebugging(); |
372 | + |
373 | + qDebug() << "MidiScriptEngine: Loading & evaluating all MIDI script code"; |
374 | + |
375 | + ConfigObject<ConfigValue> *config = new ConfigObject<ConfigValue>(QDir::homePath().append("/").append(SETTINGS_PATH).append(SETTINGS_FILE)); |
376 | + |
377 | + QString scriptPath = config->getConfigPath().append("midi/"); |
378 | + delete config; |
379 | + |
380 | + QListIterator<QString> it(scriptFileNames); |
381 | + m_scriptEngineLock.lock(); |
382 | + while (it.hasNext()) { |
383 | + QString curScriptFileName = it.next(); |
384 | + safeEvaluate(scriptPath+curScriptFileName); |
385 | + |
386 | + if(m_scriptErrors.contains(curScriptFileName)) { |
387 | + qDebug() << "Errors occured while loading " << curScriptFileName; |
388 | + } |
389 | + } |
390 | + |
391 | + m_scriptEngineLock.unlock(); |
392 | + emit(initialized()); |
393 | +} |
394 | + |
395 | +/* -------- ------------------------------------------------------ |
396 | + Purpose: Run the initialization function for each loaded script |
397 | + if it exists |
398 | + Input: - |
399 | + Output: - |
400 | + -------- ------------------------------------------------------ */ |
401 | +void MidiScriptEngine::initializeScripts(QList<QString> scriptFunctionPrefixes) { |
402 | + m_scriptEngineLock.lock(); |
403 | + |
404 | + QListIterator<QString> prefixIt(scriptFunctionPrefixes); |
405 | + while (prefixIt.hasNext()) { |
406 | + QString initName = prefixIt.next(); |
407 | + if (initName!="") { |
408 | + initName.append(".init"); |
409 | + if (m_midiDebug) qDebug() << "MidiScriptEngine: Executing" << initName; |
410 | + if (!safeExecute(initName, m_pMidiDevice->getName())) |
411 | + qWarning() << "MidiScriptEngine: No" << initName << "function in script"; |
412 | + } |
413 | + } |
414 | + m_scriptEngineLock.unlock(); |
415 | + emit(initialized()); |
416 | +} |
417 | |
418 | /* -------- ------------------------------------------------------ |
419 | Purpose: Create the MidiScriptEngine object (so it is owned in this |
420 | @@ -111,7 +200,8 @@ |
421 | unsigned static id = 0; //the id of this thread, for debugging purposes //XXX copypasta (should factor this out somehow), -kousu 2/2009 |
422 | QThread::currentThread()->setObjectName(QString("MidiScriptEngine %1").arg(++id)); |
423 | |
424 | - //qDebug() << QString("----------------------------------MidiScriptEngine: Run Thread ID=%1").arg(QThread::currentThreadId(),0,16); |
425 | + // Prevent the script engine from strangling other parts of Mixxx |
426 | + QThread::currentThread()->setPriority(QThread::LowPriority); |
427 | |
428 | m_scriptEngineLock.lock(); |
429 | initializeScriptEngine(); |
430 | @@ -143,6 +233,7 @@ |
431 | bool MidiScriptEngine::execute(QString function) { |
432 | m_scriptEngineLock.lock(); |
433 | bool ret = safeExecute(function); |
434 | + if (!ret) qDebug() << "MidiScriptEngine: Invalid script function" << function; |
435 | m_scriptEngineLock.unlock(); |
436 | return ret; |
437 | } |
438 | @@ -155,6 +246,7 @@ |
439 | bool MidiScriptEngine::execute(QString function, QString data) { |
440 | m_scriptEngineLock.lock(); |
441 | bool ret = safeExecute(function, data); |
442 | + if (!ret) qDebug() << "MidiScriptEngine: Invalid script function" << function; |
443 | m_scriptEngineLock.unlock(); |
444 | return ret; |
445 | } |
446 | @@ -170,6 +262,7 @@ |
447 | QString group) { |
448 | m_scriptEngineLock.lock(); |
449 | bool ret = safeExecute(function, channel, control, value, status, group); |
450 | + if (!ret) qDebug() << "MidiScriptEngine: Invalid script function" << function; |
451 | m_scriptEngineLock.unlock(); |
452 | return ret; |
453 | } |
454 | @@ -182,22 +275,79 @@ |
455 | bool MidiScriptEngine::safeExecute(QString function) { |
456 | //qDebug() << QString("MidiScriptEngine: Exec1 Thread ID=%1").arg(QThread::currentThreadId(),0,16); |
457 | |
458 | - if(m_pEngine == NULL) { |
459 | - return false; |
460 | - } |
461 | - |
462 | - if (!m_pEngine->canEvaluate(function)) { |
463 | - qCritical() << "MidiScriptEngine: ?Syntax error in function " << function; |
464 | - return false; |
465 | - } |
466 | + if(m_pEngine == NULL) |
467 | + return false; |
468 | |
469 | QScriptValue scriptFunction = m_pEngine->evaluate(function); |
470 | |
471 | - if (checkException()) return false; |
472 | - if (!scriptFunction.isFunction()) return false; |
473 | - |
474 | - scriptFunction.call(QScriptValue()); |
475 | - if (checkException()) return false; |
476 | + if (checkException()) |
477 | + return false; |
478 | + |
479 | + if (!scriptFunction.isFunction()) |
480 | + return false; |
481 | + |
482 | + scriptFunction.call(QScriptValue()); |
483 | + if (checkException()) |
484 | + return false; |
485 | + |
486 | + return true; |
487 | +} |
488 | + |
489 | + |
490 | +/* -------- ------------------------------------------------------ |
491 | +Purpose: Evaluate & run script code |
492 | +Input: Code string |
493 | +Output: false if an exception |
494 | +-------- ------------------------------------------------------ */ |
495 | +bool MidiScriptEngine::internalExecute(QString scriptCode) { |
496 | + // A special version of safeExecute since we're evaluating strings, not actual functions |
497 | + // (execute() would print an error that it's not a function every time a timer fires.) |
498 | + if(m_pEngine == NULL) |
499 | + return false; |
500 | + |
501 | + // Check syntax |
502 | + QScriptSyntaxCheckResult result = m_pEngine->checkSyntax(scriptCode); |
503 | + QString error=""; |
504 | + switch (result.state()) { |
505 | + case (QScriptSyntaxCheckResult::Valid): break; |
506 | + case (QScriptSyntaxCheckResult::Intermediate): |
507 | + error = "Incomplete code"; |
508 | + break; |
509 | + case (QScriptSyntaxCheckResult::Error): |
510 | + error = "Syntax error"; |
511 | + break; |
512 | + } |
513 | + if (error!="") { |
514 | + error = QString("%1: %2 at line %3, column %4 of script code:\n%5\n") |
515 | + .arg(error) |
516 | + .arg(result.errorMessage()) |
517 | + .arg(result.errorLineNumber()) |
518 | + .arg(result.errorColumnNumber()) |
519 | + .arg(scriptCode); |
520 | + |
521 | + if (m_midiDebug) qCritical() << "MidiScriptEngine:" << error; |
522 | + else { |
523 | + qDebug() << "MidiScriptEngine:" << error; |
524 | + qWarning() << "There was an error in a MIDI script." |
525 | + "\nA control you just used is not working properly and you may experience erratic behavior." |
526 | + "\nCheck the console or mixxx.log file for details."; |
527 | + } |
528 | + return false; |
529 | + } |
530 | + |
531 | + QScriptValue scriptFunction = m_pEngine->evaluate(scriptCode); |
532 | + |
533 | + if (checkException()) |
534 | + return false; |
535 | + |
536 | + // If it's not a function, we're done. |
537 | + if (!scriptFunction.isFunction()) |
538 | + return true; |
539 | + |
540 | + // If it does happen to be a function, call it. |
541 | + scriptFunction.call(QScriptValue()); |
542 | + if (checkException()) |
543 | + return false; |
544 | |
545 | return true; |
546 | } |
547 | @@ -214,11 +364,6 @@ |
548 | return false; |
549 | } |
550 | |
551 | - if (!m_pEngine->canEvaluate(function)) { |
552 | - qCritical() << "MidiScriptEngine: ?Syntax error in function " << function; |
553 | - return false; |
554 | - } |
555 | - |
556 | QScriptValue scriptFunction = m_pEngine->evaluate(function); |
557 | |
558 | if (checkException()) |
559 | @@ -250,11 +395,6 @@ |
560 | return false; |
561 | } |
562 | |
563 | - if (!m_pEngine->canEvaluate(function)) { |
564 | - qCritical() << "MidiScriptEngine: ?Syntax error in function " << function; |
565 | - return false; |
566 | - } |
567 | - |
568 | QScriptValue scriptFunction = m_pEngine->evaluate(function); |
569 | |
570 | if (checkException()) |
571 | @@ -296,15 +436,22 @@ |
572 | error << filename << errorMessage << QString(line); |
573 | m_scriptErrors.insert(filename, error); |
574 | |
575 | - qCritical() << "MidiScriptEngine uncaught exception : " |
576 | - << errorMessage |
577 | - << "at " << filename << " line" |
578 | - << line; |
579 | - // qCritical() << "MidiScriptEngine: uncaught exception" |
580 | - // << m_pEngine->uncaughtException().toString() |
581 | - // << "\nBacktrace:\n" |
582 | - // << m_pEngine->uncaughtExceptionBacktrace(); |
583 | - |
584 | + if (m_midiDebug) |
585 | + qCritical() << "MidiScriptEngine: uncaught exception:" |
586 | + << errorMessage |
587 | + << "in" << filename << "at line" |
588 | + << line |
589 | + << "\nBacktrace:\n" |
590 | + << backtrace; |
591 | + else { |
592 | + qDebug() << "MidiScriptEngine WARNING: uncaught exception:" |
593 | + << errorMessage |
594 | + << "in" << filename << "at line" |
595 | + << line; |
596 | + qWarning() << "There was a problem with a MIDI script." |
597 | + "\nA control you just used is not working properly and you may experience erratic behavior." |
598 | + "\nCheck the console or mixxx.log file for details."; |
599 | + } |
600 | return true; |
601 | } |
602 | return false; |
603 | @@ -331,7 +478,8 @@ |
604 | |
605 | // qDebug() << "MidiScriptEngine: m_scriptCode=" << m_scriptCode; |
606 | |
607 | - qDebug() << "MidiScriptEngine:" << codeLines.count() << "lines of code being searched for functions"; |
608 | + if (m_midiDebug) |
609 | + qDebug() << "MidiScriptEngine:" << codeLines.count() << "lines of code being searched for functions"; |
610 | |
611 | // grep 'function' midi/midi-mappings-scripts.js|grep -i '(msg)'|sed -e 's/function \(.*\)(msg).*/\1/i' -e 's/[= ]//g' |
612 | QRegExp rx("*.*function*(*)*"); // Find all lines with function names in them |
613 | @@ -345,16 +493,13 @@ |
614 | |
615 | if (line.indexOf('#') != 0 && line.indexOf("//") != 0) { // ignore commented out lines |
616 | QStringList field = line.split(" "); |
617 | - if (m_pMidiDevice && m_pMidiDevice->midiDebugging()) |
618 | - qDebug() << "MidiScriptEngine: Found function:" << field[0] |
619 | - << "at line" << position; |
620 | -// functionList.append(field[0]); |
621 | + if (m_midiDebug) qDebug() << "MidiScriptEngine: Found function:" << field[0] |
622 | + << "at line" << position; |
623 | m_scriptFunctions.append(field[0]); |
624 | } |
625 | position = codeLines.indexOf(rx); |
626 | } |
627 | |
628 | -// m_scriptFunctions = functionList; |
629 | } |
630 | |
631 | ControlObjectThread* MidiScriptEngine::getControlObjectThread(QString group, QString name) { |
632 | @@ -478,13 +623,8 @@ |
633 | return false; |
634 | } |
635 | |
636 | - if (!m_pEngine->canEvaluate(function)) { |
637 | - qCritical() << "MidiScriptEngine: ?Syntax error in function " << function; |
638 | - return false; |
639 | - } |
640 | QScriptValue slot = m_pEngine->evaluate(function); |
641 | |
642 | - |
643 | if(!checkException() && slot.isFunction()) { |
644 | if(disconnect) { |
645 | // qDebug() << "MidiScriptEngine::connectControl disconnected " << group << name << " from " << function; |
646 | @@ -560,6 +700,8 @@ |
647 | return false; |
648 | } |
649 | |
650 | + qDebug() << "MidiScriptEngine: Loading" << filename; |
651 | + |
652 | // Read in the script file |
653 | QFile input(filename); |
654 | if (!input.open(QIODevice::ReadOnly)) { |
655 | @@ -574,8 +716,33 @@ |
656 | scriptCode.append('\n'); |
657 | input.close(); |
658 | |
659 | - if (!m_pEngine->canEvaluate(scriptCode)) { |
660 | - qCritical() << "MidiScriptEngine: ?Syntax error in script file:" << filename; |
661 | + // Check syntax |
662 | + QScriptSyntaxCheckResult result = m_pEngine->checkSyntax(scriptCode); |
663 | + QString error=""; |
664 | + switch (result.state()) { |
665 | + case (QScriptSyntaxCheckResult::Valid): break; |
666 | + case (QScriptSyntaxCheckResult::Intermediate): |
667 | + error = "Incomplete code"; |
668 | + break; |
669 | + case (QScriptSyntaxCheckResult::Error): |
670 | + error = "Syntax error"; |
671 | + break; |
672 | + } |
673 | + if (error!="") { |
674 | + error = QString("%1 at line %2, column %3 in file %4: %5") |
675 | + .arg(error) |
676 | + .arg(result.errorLineNumber()) |
677 | + .arg(result.errorColumnNumber()) |
678 | + .arg(filename) |
679 | + .arg(result.errorMessage()); |
680 | + |
681 | + if (m_midiDebug) qCritical() << "MidiScriptEngine:" << error; |
682 | + else { |
683 | + qDebug() << "MidiScriptEngine:" << error; |
684 | + qWarning() << "There was an error in the MIDI script file" << filename |
685 | + << "\nThe functionality provided by this script file will be disabled." |
686 | + "\nCheck the console or mixxx.log file for details."; |
687 | + } |
688 | return false; |
689 | } |
690 | |
691 | @@ -587,7 +754,6 @@ |
692 | return false; |
693 | |
694 | // Add the code we evaluated to our index |
695 | - qDebug() << "MidiScriptEngine: Loading" << filename; |
696 | generateScriptFunctions(scriptCode); |
697 | |
698 | return true; |
699 | @@ -616,3 +782,99 @@ |
700 | } |
701 | |
702 | |
703 | +/* -------- ------------------------------------------------------ |
704 | + Purpose: Creates & starts a timer that runs some script code |
705 | + on timeout |
706 | + Input: Number of milliseconds, script function to call, |
707 | + whether it should fire just once |
708 | + Output: The timer's ID, 0 if starting it failed |
709 | + -------- ------------------------------------------------------ */ |
710 | +int MidiScriptEngine::beginTimer(int interval, QString scriptCode, bool oneShot) { |
711 | + // When this function runs, assert that somebody is holding the script |
712 | + // engine lock. |
713 | + bool lock = m_scriptEngineLock.tryLock(); |
714 | + Q_ASSERT(!lock); |
715 | + if(lock) { |
716 | + m_scriptEngineLock.unlock(); |
717 | + } |
718 | + |
719 | + if (interval<20) { |
720 | + qDebug() << "Timer request for" << interval << "ms is too short. Setting to the minimum of 20ms."; |
721 | + interval=20; |
722 | + } |
723 | + // This makes use of every QObject's internal timer mechanism. Nice, clean, and simple. |
724 | + // See http://doc.trolltech.com/4.6/qobject.html#startTimer for details |
725 | + int timerId = startTimer(interval); |
726 | + QPair<QString, bool> timerTarget; |
727 | + timerTarget.first = scriptCode; |
728 | + timerTarget.second = oneShot; |
729 | + m_timers[timerId]=timerTarget; |
730 | + if (timerId==0) qDebug() << "MIDI Script timer could not be created"; |
731 | + else if (m_midiDebug) { |
732 | + if (oneShot) qDebug() << "Starting one-shot timer:" << timerId; |
733 | + else qDebug() << "Starting timer:" << timerId; |
734 | + } |
735 | + return timerId; |
736 | +} |
737 | + |
738 | +/* -------- ------------------------------------------------------ |
739 | + Purpose: Stops & removes a timer |
740 | + Input: ID of timer to stop |
741 | + Output: - |
742 | + -------- ------------------------------------------------------ */ |
743 | +void MidiScriptEngine::stopTimer(int timerId) { |
744 | + // When this function runs, assert that somebody is holding the script |
745 | + // engine lock. |
746 | + bool lock = m_scriptEngineLock.tryLock(); |
747 | + Q_ASSERT(!lock); |
748 | + if(lock) m_scriptEngineLock.unlock(); |
749 | + |
750 | + if (!m_timers.contains(timerId)) { |
751 | + qDebug() << "Killing timer" << timerId << ": That timer does not exist!"; |
752 | + return; |
753 | + } |
754 | + if (m_midiDebug) qDebug() << "Killing timer:" << timerId; |
755 | + |
756 | + killTimer(timerId); |
757 | + m_timers.remove(timerId); |
758 | +} |
759 | + |
760 | +/* -------- ------------------------------------------------------ |
761 | + Purpose: Stops & removes all timers (for shutdown) |
762 | + Input: - |
763 | + Output: - |
764 | + -------- ------------------------------------------------------ */ |
765 | +void MidiScriptEngine::stopAllTimers() { |
766 | + // When this function runs, assert that somebody is holding the script |
767 | + // engine lock. |
768 | + bool lock = m_scriptEngineLock.tryLock(); |
769 | + Q_ASSERT(!lock); |
770 | + if(lock) m_scriptEngineLock.unlock(); |
771 | + |
772 | + QMutableHashIterator<int, QPair<QString, bool> > i(m_timers); |
773 | + while (i.hasNext()) { |
774 | + i.next(); |
775 | + stopTimer(i.key()); |
776 | + } |
777 | +} |
778 | + |
779 | +/* -------- ------------------------------------------------------ |
780 | + Purpose: Runs the appropriate script code on timer events |
781 | + Input: - |
782 | + Output: - |
783 | + -------- ------------------------------------------------------ */ |
784 | +void MidiScriptEngine::timerEvent(QTimerEvent *event) { |
785 | + int timerId = event->timerId(); |
786 | + |
787 | + m_scriptEngineLock.lock(); |
788 | + if (!m_timers.contains(timerId)) { |
789 | + qDebug() << "Timer" << timerId << "fired but there's no function mapped to it!"; |
790 | + return; |
791 | + } |
792 | + |
793 | + QPair<QString, bool> timerTarget = m_timers[timerId]; |
794 | + if (timerTarget.second) stopTimer(timerId); |
795 | + |
796 | + internalExecute(timerTarget.first); |
797 | + m_scriptEngineLock.unlock(); |
798 | +} |
799 | |
800 | === modified file 'mixxx/src/midi/midiscriptengine.h' |
801 | --- mixxx/src/midi/midiscriptengine.h 2009-10-27 07:31:35 +0000 |
802 | +++ mixxx/src/midi/midiscriptengine.h 2010-03-24 04:40:48 +0000 |
803 | @@ -38,16 +38,6 @@ |
804 | bool hasErrors(QString filename); |
805 | const QStringList getErrors(QString filename); |
806 | |
807 | - // Evaluate a script file |
808 | - bool evaluate(QString filepath); |
809 | - // Execute a particular function |
810 | - bool execute(QString function); |
811 | - // Execute a particular function with a data string (e.g. a device ID) |
812 | - bool execute(QString function, QString data); |
813 | - // Execute a particular function with all the data |
814 | - bool execute(QString function, char channel, |
815 | - char control, char value, MidiStatusByte status, QString group); |
816 | - |
817 | // Lookup registered script functions |
818 | QStringList getScriptFunctions(); |
819 | |
820 | @@ -58,19 +48,35 @@ |
821 | Q_INVOKABLE bool connectControl(QString group, QString name, |
822 | QString function, bool disconnect = false); |
823 | Q_INVOKABLE void trigger(QString group, QString name); |
824 | + Q_INVOKABLE int beginTimer(int interval, QString scriptCode, bool oneShot = false); |
825 | + Q_INVOKABLE void stopTimer(int timerId); |
826 | |
827 | public slots: |
828 | void slotValueChanged(double value); |
829 | + // Evaluate a script file |
830 | + bool evaluate(QString filepath); |
831 | + // Execute a particular function |
832 | + bool execute(QString function); |
833 | + // Execute a particular function with a data string (e.g. a device ID) |
834 | + bool execute(QString function, QString data); |
835 | + // Execute a particular function with all the data |
836 | + bool execute(QString function, char channel, |
837 | + char control, char value, MidiStatusByte status, QString group); |
838 | + void loadScriptFiles(QList<QString> scriptFileNames); |
839 | + void initializeScripts(QList<QString> scriptFunctionPrefixes); |
840 | + void gracefulShutdown(QList<QString> scriptFunctionPrefixes); |
841 | |
842 | signals: |
843 | void initialized(); |
844 | |
845 | protected: |
846 | void run(); |
847 | + void timerEvent(QTimerEvent *event); |
848 | |
849 | private: |
850 | // Only call these with the scriptEngineLock |
851 | bool safeEvaluate(QString filepath); |
852 | + bool internalExecute(QString scriptCode); |
853 | bool safeExecute(QString function); |
854 | bool safeExecute(QString function, QString data); |
855 | bool safeExecute(QString function, char channel, |
856 | @@ -79,18 +85,20 @@ |
857 | |
858 | void generateScriptFunctions(QString code); |
859 | bool checkException(); |
860 | + void stopAllTimers(); |
861 | |
862 | ControlObjectThread* getControlObjectThread(QString group, QString name); |
863 | |
864 | |
865 | MidiDevice* m_pMidiDevice; |
866 | + bool m_midiDebug; |
867 | QHash<ConfigKey, QString> m_connectedControls; |
868 | QScriptEngine *m_pEngine; |
869 | QStringList m_scriptFunctions; |
870 | QMap<QString,QStringList> m_scriptErrors; |
871 | QMutex m_scriptEngineLock; |
872 | QHash<ConfigKey, ControlObjectThread*> m_controlCache; |
873 | + QHash<int, QPair<QString, bool> > m_timers; |
874 | }; |
875 | |
876 | #endif |
877 | - |
878 | |
879 | === modified file 'mixxx/src/mixxx.cpp' |
880 | --- mixxx/src/mixxx.cpp 2010-02-23 12:10:51 +0000 |
881 | +++ mixxx/src/mixxx.cpp 2010-03-24 04:40:48 +0000 |
882 | @@ -476,11 +476,6 @@ |
883 | |
884 | qDebug() << "delete config, " << qTime.elapsed(); |
885 | delete config; |
886 | - |
887 | -// Why is this here? The (MSVC 2008) linker even complains about it. |
888 | -//#ifdef __WINDOWS__ |
889 | -// _exit(0); |
890 | -//#endif |
891 | } |
892 | |
893 | int MixxxApp::noSoundDlg(void) |
MIDI script timers have been successfully added to the MIDI script engine, and tested on an SCS.3d using a light show script. (The venerable light show script first seen on YouTube has been updated to no longer run an endless loop! :) The light show can now be paused, restarted, and run without noticeably impacting Mixxx's performance.)
I respectfully request consideration of merging this in time for the 1.8 release for the following reasons:
1) The timer functionality has been added on to the MIDI script engine (as opposed to changing anything,) so the core functionality has not been altered
2) Good scratching performance requires the use of fixed-interval control sampling which is not possible without timers. This is one area where Mixxx is lagging its competitors and having it ready for 1.8 would help close the gap. (I plan to modify the common script scratching functions to make use of the timers (which won't take long) after this is merged, allowing all scripts that currently use the scratch.* common functions to take advantage of it immediately. (Some minor tweaks may be needed.))
3) Timers also allow script programmers to flash lights on MIDI controllers (such as when nearing the end of a track, setting a cue point (like CDJs,) or changing modes) without busy-wait loops which are inaccurate and have an adverse effect on Mixxx's playback.