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

DDJ-200: Listen to changes from mixxx #3793

Merged
merged 3 commits into from
May 17, 2021
Merged

DDJ-200: Listen to changes from mixxx #3793

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e27fd43
Listen to changes from mixxx
geovannimp Apr 17, 2021
1923a3d
Optimize code changing only necessary leds
geovannimp Apr 18, 2021
f0ff2fc
Fix lint problems
geovannimp Apr 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 44 additions & 8 deletions res/controllers/Pioneer-DDJ-200-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,36 @@ DDJ200.init = function() {

var vgroup = "[Channel" + i + "]";

// run onTrackLoad after every track load to set LEDs accordingly
// run updateDeckLeds after every track load to set LEDs accordingly
engine.makeConnection(vgroup, "track_loaded", function(ch, vgroup) {
DDJ200.onTrackLoad(ch, vgroup);
DDJ200.updateDeckLeds(vgroup);
});

// run switchPlayLED after play/pause track to set LEDs accordingly
engine.makeConnection(vgroup, "play", function(ch, vgroup) {
var vDeckNo = script.deckFromGroup(vgroup);
var d = (vDeckNo % 2) ? 0 : 1;
DDJ200.switchPlayLED(d, ch);
});

// run switchSyncLED after sync toogle to set LEDs accordingly
engine.makeConnection(vgroup, "sync_enabled", function(ch, vgroup) {
var vDeckNo = script.deckFromGroup(vgroup);
var d = (vDeckNo % 2) ? 0 : 1;
DDJ200.switchSyncLED(d, ch);
});

// listen to changes on hotcues
for (var j = 1; j <= 8; j++) {
// run switchPadLED after every hotcue update
engine.makeConnection(vgroup, "hotcue_" + j + "_enabled", function(ch, vgroup, control) {
var pad = Number(control.split("_")[1]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var pad = Number(control.split("_")[1]);
var pad = j;

Should have the same effect, but please test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This don't work cause the callback function is in another context. As the function executes outside the for loop, j will be always 8.

I could write this instead:

            engine.makeConnection(vgroup, "hotcue_" + j + "_enabled", function (pad) {
                return function(ch, vgroup, control) {
                    var vDeckNo = script.deckFromGroup(vgroup);
                    var d = (vDeckNo % 2) ? 0 : 1;           // d = deckNo - 1
                    DDJ200.switchPadLED(d, pad, ch);
                };
            }(j));

But I think the code readability would be compromised.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you are right: Creating closures in loops: A common mistake

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I figured j was copied in the loop because its just number. Both of your solutions are fine. You can keep it as is if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Swiftb0y If I had to use JavaScript I would fail miserably. Looked it up out of curiosity and was surprised, unpleasantly of course.

var vDeckNo = script.deckFromGroup(vgroup);
var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1
DDJ200.switchPadLED(d, pad, ch);
Comment on lines +46 to +48
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use one-character variable names (except i/j for counters maybe) and go with something more meaningful:

Suggested change
var vDeckNo = script.deckFromGroup(vgroup);
var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1
DDJ200.switchPadLED(d, pad, ch);
var deckNo = script.deckFromGroup(vgroup);
var deckOffset = (deckNo - 1) % 2;
DDJ200.switchPadLED(deckOffset, pad, ch);

I know the rest of the script does that too, but let's at least fix it for new code :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like one-character variables too, but at same time is good to keep a pattern at the code. Don't you think would be better open a new PR fixing all one-character variables after this one?

Copy link

Choose a reason for hiding this comment

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

I second @Holzhaus, and I think it should probably be done in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just the new code or a full refactor?

Copy link
Contributor

Choose a reason for hiding this comment

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

var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1
seems clear and short to me.

var deckOffset = (deckNo - 1) % 2;
is neither short nor clear.
It is less obvious that the value is either 0 or 1.
And it is not correct, because it is not an offset. I don't know what to call this, but for sure not offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var controllerDeckIndex = (vDeckNo % 2) ? 0 : 1;?
Case this represents the controller deck and an index starting from 0 to 1

});
}

// set Pioneer CDJ cue mode for all decks
engine.setValue(vgroup, "cue_cdj", true);
}
Expand Down Expand Up @@ -54,7 +79,7 @@ DDJ200.LEDsOff = function() { // turn off LED buttons:
}
};

DDJ200.onTrackLoad = function(channel, vgroup) {
DDJ200.updateDeckLeds = function(vgroup) {
// set LEDs (hotcues, etc.) for the loaded deck
// if controller is switched to this deck
var vDeckNo = script.deckFromGroup(vgroup);
Expand Down Expand Up @@ -320,22 +345,33 @@ DDJ200.switchLEDs = function(vDeckNo) {
// set LEDs of controller deck 1 or 2 according to virtual deck
var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1
var vgroup = "[Channel" + vDeckNo + "]";
midi.sendShortMsg(0x90 + d, 0x0B, 0x7F * engine.getValue(vgroup, "play"));
DDJ200.switchPlayLED(d, engine.getValue(vgroup, "play"));
midi.sendShortMsg(0x90 + d, 0x0C, 0x7F *
(engine.getValue(vgroup, "cue_point") !== -1));
midi.sendShortMsg(0x90 + d, 0x58, 0x7F * engine.getValue(vgroup,
"sync_enabled"));
DDJ200.switchSyncLED(d, engine.getValue(vgroup, "sync_enabled"));
if (!DDJ200.fourDeckMode) {
midi.sendShortMsg(0x90 + d, 0x54,
0x7F * engine.getValue(vgroup, "pfl"));
}

for (var i = 1; i <= 8; i++) {
midi.sendShortMsg(0x97 + 2 * d, i - 1, 0x7F * engine.getValue(
vgroup, "hotcue_" + i + "_enabled"));
var isButtonEnabled = engine.getValue(vgroup, "hotcue_" + i + "_enabled");
DDJ200.switchPadLED(d, i, isButtonEnabled);
}
};

DDJ200.switchPlayLED = function(deck, enabled) {
midi.sendShortMsg(0x90 + deck, 0x0B, 0x7F * enabled);
};

DDJ200.switchSyncLED = function(deck, enabled) {
midi.sendShortMsg(0x90 + deck, 0x58, 0x7F * enabled);
};

DDJ200.switchPadLED = function(deck, pad, enabled) {
midi.sendShortMsg(0x97 + 2 * deck, pad - 1, 0x7F * enabled);
};

DDJ200.toggleDeck = function(channel, control, value, status, group) {
if (value) { // only if button pressed, not releases, i.e. value === 0
if (DDJ200.shiftPressed["left"]) {
Expand Down