Numark Party Mix: added initial mapping #4720
Conversation
ronso0
changed the title
|
Thank you for starting to map this controller! Usually new mappings go to the stable branch (2.3 in this case) unless they require features from the develoment branch (main). |
|
the pre-commit hooks are failing. |
|
now pre-commit passed on my laptop |
it fails here because of unused variables |
|
Please don't resolve (collapse) review issues yourself, that is done by the reviewer(s). As a reviewer it's tedious to unfold again to check the reply / resolution. |
|
Also: don't force-push once a review has been started. The comments are visible here in the PR view but in the file view they are detached = deleted. |
|
lets try again on my laptop the unused variables haven't been highlighted. |
|
@Swiftb0y i clicked the wrong button i didn't wanted to commit that suggestion. I will fix that later. |
| <key>NumarkPartyMix.deck[1].scratchToggle.input</key> | ||
| <status>0x81</status> | ||
| <midino>0x07</midino> | ||
| <key>NumarkPartyMix.deck[1].wheelTurn.input</key> | ||
| <status>0xB1</status> | ||
| <midino>0x06</midino> |
There was a problem hiding this comment.
removing the NoteOff message handlers is not a good idea generally. Otherwises there might be some surprises when you change the handler later.
There was a problem hiding this comment.
I (aimed) to remove all NoteOff message handlers that do not have a function, to keep the specification focused. In my opinion having specifications for unused NoteOff messages would only make sense if you could specify an option to ignore them.
There was a problem hiding this comment.
Can you elaborate? My reasoning for keeping all the NoteOffs even if they seem unnecessary, is that the components handlers might depend on them. For buttons, you can specify type: components.Button.prototype.types.toggle (or something like that, I don't remember exactly).
The XML is not exactly a specification either, its more like a legacy artifact we aim to remove some time in the future.
There was a problem hiding this comment.
I will not change it.
There was a problem hiding this comment.
this device uses 0x9Y for NoteOn and 0x8Y for NoteOff messages so the unmapped values shouldn't cause trouble. i improved the scratchtoggle code
There was a problem hiding this comment.
the issue I see is that the unmapped values don't end up at the input handlers of the components, even though those components probably expect them. So it could cause weird behavior from the mapping code.
| // I am not sure if the jogwheel is behaving consistently, | ||
| // or if my variables are wrong. | ||
| // If one is scratching the point in a track is moving. | ||
| // Maybe these variables need improvement. |
There was a problem hiding this comment.
if you put your finger on the jogwheel in scratch mode (without pressing play) and circle clockwise and one circle counter clockwise that the the track is at exactly the same position. this is unfortunately not the case. But I don't know if this is a problem of the hardware or my variables.
There was a problem hiding this comment.
Mhmm. You could do a test with a simple handler that just counts all the ticks from each jogwheel.
There was a problem hiding this comment.
someone who is interested in it can do that
There was a problem hiding this comment.
its ~298 the jump happens when you scratch back and forth fast around the same spot ... i guess its when you move too fast for the clock of the device, so it misses some ticks.
There was a problem hiding this comment.
Mhmm. So you can reproduce it by just summing the values the hardware sends?
| // dim LEDs for scratch buttons | ||
| midi.sendShortMsg(0x90, 0x07, ledOff); | ||
| midi.sendShortMsg(0x91, 0x07, ledOff); | ||
|
|
||
| // untoggle (dim) PFL switches | ||
| midi.sendShortMsg(0x80, 0x1B, ledOff); | ||
| midi.sendShortMsg(0x81, 0x1B, ledOff); |
There was a problem hiding this comment.
please overwrite the shutdown hook on the component.Button instances instead. They might not even be needed at at all really.
| // dim LEDs for scratch buttons | |
| midi.sendShortMsg(0x90, 0x07, ledOff); | |
| midi.sendShortMsg(0x91, 0x07, ledOff); | |
| // untoggle (dim) PFL switches | |
| midi.sendShortMsg(0x80, 0x1B, ledOff); | |
| midi.sendShortMsg(0x81, 0x1B, ledOff); | |
| NumarkPartyMix.deck[0].shutdown(); | |
| NumarkPartyMix.deck[1].shutdown(); |
There was a problem hiding this comment.
I haven't really taken care of shutdown before you mention it. I tested your suggestion but it didn't work. So I just corrected the global function. I don't think it is important to improve this. I am not interested improving the code further.
There was a problem hiding this comment.
I don't see why my suggestion shouldn't work... The shutdown "signal" should get propagated the entire tree down and should eventually be called on every component.
| <key>NumarkPartyMix.deck[1].scratchToggle.input</key> | ||
| <status>0x81</status> | ||
| <midino>0x07</midino> | ||
| <key>NumarkPartyMix.deck[1].wheelTurn.input</key> | ||
| <status>0xB1</status> | ||
| <midino>0x06</midino> |
There was a problem hiding this comment.
Can you elaborate? My reasoning for keeping all the NoteOffs even if they seem unnecessary, is that the components handlers might depend on them. For buttons, you can specify type: components.Button.prototype.types.toggle (or something like that, I don't remember exactly).
The XML is not exactly a specification either, its more like a legacy artifact we aim to remove some time in the future.
| // I am not sure if the jogwheel is behaving consistently, | ||
| // or if my variables are wrong. | ||
| // If one is scratching the point in a track is moving. | ||
| // Maybe these variables need improvement. |
There was a problem hiding this comment.
Mhmm. You could do a test with a simple handler that just counts all the ticks from each jogwheel.
|
I am not sure if I will continue to work on this PR. I used existing code that is not nice but it is also not important to make this a nice implementation. The device is old not professional and a standard mapping is not what I will use. The interest in the previous versions of the mappings for the device has been limited. One reason for this might be that the jogwheel is not behaving consistently as described above. The current version works and doesn't create any problems. |
|
I'm sorry for being so rigorous with my review comments. Allow me to explain my reasoning:
I wish to eliminate this, its very common that people just copy-paste mapping code together. Having to review the same copy-pasted mistakes each and every time is tiring for the few people that review mappings. Since you seem to have good knowledge of javascript. I was hoping that my reviews would be able increase the quality of the code, thus benefiting mixxx in the longterm.
That is not super relevant. We love to have old devices in mixxx, so they can continue to be used while other manufacturers drop their support. The majority of mixxx users does not use professional hardware, so the inclusion of this entry-level device is very much appreciated. You are also very welcome to use your own mapping, so I'm even more grateful that you are willing to spend your time making another mapping just for mixxx.
I don't doubt that. Though as I previously explained, the quality of the mapping code is also important (at least for me personally). Since no other mixxx developer has access to this device, any change to the mapping is essentially working in the dark as we can't verify that none of our fixes introduced any problems. So working on the mapping without having access to the corresponding hardware is practically impossible. I hope I was able to convince you that it would be very much appreciated if you could take care of the remaining review comments. I have a couple questions after that but otherwise your mapping is already in very good shape. Thanks. |
|
ok maybe the process will be just slow. and sorry for getting inpatient. I finished my "derivations" so maybe I have more of a mind again to look at the "standard" mapping. I also have to say that there are too many ways to write javascript and I have to admit that i am not so familiar with the style used here. So some parts of my code must be not nice and i am not even sure if some lines make sense. I recommend you to put a link to the implementation of a reference device (the one with the best up to date javascript/xml) into the manual and into the wiki. |
Your code is mostly fine. The few remaining issues I have primarily are primarily "separation of concerns" issues. For example, if an LED is initialized or reset on shutdown, it should be handled on the component that encapsulates that unit. Take your time, we aren't usually fast with reviewing mappings either.
I thought we had that somewhere I couldn't find any mention while searching right now though. The mappings I can recommend you take a look at are the ones we ship for the Roland DJ-505 and the Numark N4 (disclaimer: the latter being from me). |
My theory is that the sampling frequency of the encoder is too low so that fast movements are not tracked properly. |
mixxx doesn't give enough time for shutdown to happen see this bug: https://bugs.launchpad.net/mixxx/+bug/1660488 So i cannot see if it would switch of the LEDs if there would be enough time to iterate through the containers. The current solution seems to be the only reliable one. (yes not beautiful) |
|
I looked at the other comments i don't see any other change you suggested. If there are no other suggestions I would squash the PRs. |
As I explained earlier, let's do this when merging after the final LGTM I'll take another look at my review soonish. |
|
@Swiftb0y Do you want to take another look? I didn't do an deep code review, just a skimmed through.. |
|
Yes, I'll try take a look later today. |
I don't think that applies here. The bug report is about timers not running after the shutdown has been completed, but if you don't use any timers in your shutdown code. It should still work (assuming your controller can handle the amount of incoming midi messages). |
There was a problem hiding this comment.
Thank you very much for your patience with my review. I consider this PR to be top notch now. Now I'm just waiting for the manual page. Also, are you sufficient enough to clean up the commit history a bit? If you're lazy, just squashing everything would be fine as well. If not, I can also take care of the squashing for you.
|
This PR is marked as stale because it has been open 90 days with no activity. |
github-actions
bot
added
the
stale
|
@ronso0 anything that needs to be done before merging? |
github-actions
bot
removed
the
stale
|
@Swiftb0y This is ready, right? |
don't break my code
|
@Swiftb0y the manual seems to be ok according to @ronso0 mixxxdj/manual#479 (comment) |
|
@olafklingt Thanks for this mapping, and for your patience, of course! |
|
@olafklingt Thank you for your contribution. I am preparing the 2.3.4 release and have notice that we don't have the formal confirm that we can distribute your work with Mixxx. Can you sign https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ and comment here when done? |
|
Done :-) |
|
Thank you. I like to add your name to the contributor list in the Mixxx about box. Do you like to see your nick name or your legal name? |
|
olafklingt , thanks for asking |
Creating a PR now to see if it pass the tests. will create Documentation at another weekend.