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

Numark DJ2GO2 Touch mapping #4108

Merged
merged 22 commits into from Jul 29, 2021
Merged

Numark DJ2GO2 Touch mapping #4108

merged 22 commits into from Jul 29, 2021

Conversation

tandy-1000
Copy link
Contributor

I wrote a components.js mapping with help from @Holzhaus , @Swiftb0y , and others on the Zulip.

All functionality that I know of is available, however the browse encoder functionality still has some work to be done.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks, mapping looks very good.
Could you remove all of the unnecessary comments from the template?
Thanks

res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
@Swiftb0y
Copy link
Member

Also, in the future, make a feature branch when implementing new features such as a controller mapping otherwise you might get into trouble with git later. This was outlined here: https://github.com/mixxxdj/mixxx/wiki/Contributing-Mappings#setting-up-git

@tandy-1000
Copy link
Contributor Author

I managed to fix the encoder issues :) completely mapped now.

res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
// create an instance of your custom Deck object for each side of your controller
DJ2GO2Touch.leftDeck = new DJ2GO2Touch.Deck([1], 0);
DJ2GO2Touch.rightDeck = new DJ2GO2Touch.Deck([2], 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
};
midi.sendSysexMsg([0xF0, 0x00, 0x20, 0x7F, 0x03, 0x01, 0xF7], 0);
};

Not sure if your controller supports it, but if you add this, your controller should send the values of all of its controls. This way, Mixxx should be instantly synced with the controlled when the mapping was loaded.

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 just tried adding this, it didnt seem to make a difference, is there anything I should look out for in the controller debug output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debug [Controller]: DJ2GO2 Touch MIDI 1: outgoing: status 0x95 (ch 6, opcode 0x9), ctrl 0x23, val 0x00
Debug [Controller]: DJ2GO2 Touch MIDI 1: outgoing: 7 byte sysex: [F0 00 20 7F 03 01 F7]
Debug [Controller]: DJ2GO2 Touch MIDI 1: outgoing: status 0x94 (ch 5, opcode 0x9), ctrl 0x33, val 0x7F
Debug [Controller]: DJ2GO2 Touch MIDI 1: outgoing: status 0x94 (ch 5, opcode 0x9), ctrl 0x34, val 0x7F
Debug [Controller]: DJ2GO2 Touch MIDI 1: outgoing: status 0x94 (ch 5, opcode 0x9), ctrl 0x35, val 0x7F

I spotted it, but not sure ...

Copy link
Member

Choose a reason for hiding this comment

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

The controller should usually respond with a bunch of messages. We have a wiki page on this if you need more context: https://github.com/mixxxdj/mixxx/wiki/serato_sysex

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 understand now, it mostly works, the faders are accurate to their state on the controller, potentiometers aren't though, I'll commit that now.

Copy link
Member

Choose a reason for hiding this comment

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

Mhmm, thats strange...

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thanks. This is very close to being finished.
I enabled CI to check for possible issues.

res/controllers/Numark_DJ2GO2_Touch_scripts.js Outdated Show resolved Hide resolved
Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
@Holzhaus
Copy link
Member

Thanks. This is very close to being finished.

Please note that we require a corresponding, finished documentation PR for the mixxxdj/manual repo before we can merge.

@tandy-1000
Copy link
Contributor Author

I made a PR for my manual entry :)

@Swiftb0y
Copy link
Member

@tandy-1000 thanks for your work.
we need you to sign the contributor agreement so we can merge and publish your mapping.
https://docs.google.com/forms/d/e/1FAIpQLScC9QG327sjLL0eWftmfGUasxFFLxg6LCXJ2xHDYRzFIRqyiw/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ
Thanks.

@tandy-1000
Copy link
Contributor Author

Done

@Swiftb0y
Copy link
Member

Thanks

@Swiftb0y Swiftb0y added the changelog This PR should be included in the changelog label Jul 28, 2021
@Swiftb0y
Copy link
Member

Give me some time to take a final look before merging. Thanks.

@Holzhaus
Copy link
Member

Give me some time to take a final look before merging. Thanks.

@tandy-1000 was quite responsive and fixed all issues pointed out during review quickly. Considering that the manual PR was already merged, I suppose we can merge now and if we find more issues, these can be taken care of in a separate PR.

Ideally we should merge the mapping and the manual page at the same time. Now its a bit confusing that the manual already lists the mapping as "added in 2.3.1" but its actually missing.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you very much for bringing this controller to mixxx.

@Holzhaus I'm sorry I merged the manual page pre-maturely, I'll make sure to merge both PRs simultaneously next time.

@Swiftb0y Swiftb0y merged commit 9c70a34 into mixxxdj:2.3 Jul 29, 2021
@Holzhaus Holzhaus added this to the 2.3.1 milestone Jul 29, 2021
@tandy-1000
Copy link
Contributor Author

Thank you everyone for the support in getting this merged, I didn't expect to be able to do it but it was possible with everyone's help :)

Mixxx is a great piece of software and I am happy to have been able to contribute.

@uklotzde
Copy link
Contributor

@Swiftb0y Next time please squash the commits before merging.

@uklotzde
Copy link
Contributor

...especially controller mapping branches are often a bit messy with lots of back and forth commits and unhelpful commit messages.

@Holzhaus Holzhaus changed the title Numark DJ2GO Touch mapping Numark DJ2GO2 Touch mapping Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog controller mappings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants