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

Added new method HidController::send_feature_report #3051

Merged

Conversation

JoergAtGithub
Copy link
Member

Made the method hid_send_feature_report from HIDAPI available to controller mappings.
This generic USB HID "Set Reports (Feature)" functionality is needed to set the NI Traktor Z2 in a mode, where it accepts the bulk HID reports for LED control. See also: https://mixxx.zulipchat.com/#narrow/stream/113295-controller-mapping/topic/Traktor.20Kontrol.20Z2
The USBlyzer dump looks now like this:
grafik

@JoergAtGithub
Copy link
Member Author

Do I need to document this new API method anywhere?

@Be-ing Be-ing changed the base branch from 2.3 to master August 30, 2020 10:58
@Be-ing
Copy link
Contributor

Be-ing commented Aug 30, 2020

This is a new feature so it should go to the master branch.

@Be-ing
Copy link
Contributor

Be-ing commented Aug 30, 2020

Do I need to document this new API method anywhere?

https://github.com/mixxxdj/mixxx/wiki/hid%20mapping#sending-data-to-the-controller

src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Aug 30, 2020

I'd argue we should even use the more general API spec where we mandate the reportID to be included in the buffer itself instead of being passed separately just of it to be prepended in the C++ later.

Hmm, we could do this. Or we could make the report ID the first argument so it can't be omitted by the script. However, that could get confusing because controller.send has the arguments the opposite way around. So I think requiring the script to provide the report ID in the buffer would be better.

src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.h Outdated Show resolved Hide resolved
src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
@JoergAtGithub

This comment has been minimized.

Always print warnings independent of the ControllerDebug::enabled() state
@JoergAtGithub
Copy link
Member Author

If it's the common opinion, that the method HidController::send_feature_report should have the Report ID at the begining while the nearly identical method HidController::send has it at the end, I will change it!
But my personal opinion is, that such an inconsistent order of arguments in one and the same API, is at least weird, if not a broken.

@uklotzde
Copy link
Contributor

uklotzde commented Sep 7, 2020

If it's the common opinion, that the method HidController::send_feature_report should have the Report ID at the begining while the nearly identical method HidController::send has it at the end, I will change it!
But my personal opinion is, that such an inconsistent order of arguments in one and the same API, is at least weird, if not a broken.

I agree with that opinion. Either change all signatures or mimic the old style, even if it is considered inappropriate. Consistency is key.

@Be-ing
Copy link
Contributor

Be-ing commented Sep 8, 2020

How about let's make this new function fit with the existing controller.send API, then in the new controller system we can make a separate HIDControllerJSProxy class with different function signatures? @Swiftb0y what do you think?

src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Code LGTM, unfortunately I can't test this as I don't own an HID controller.

@JoergAtGithub
Copy link
Member Author

@ywwg Hello Owen, can this be merged?

@Holzhaus
Copy link
Member

Ping @ywwg

@JoergAtGithub
Copy link
Member Author

What can I do, to get this PR merged?

@JoergAtGithub
Copy link
Member Author

@ywwg Do you've any objections to merge this?

@uklotzde
Copy link
Contributor

uklotzde commented Oct 1, 2020

Ping @ywwg

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

this looks straightforward. It would be nice if there was a way to add a test for this but we don't have any tests for hidcontroller :(

LGTM

src/controllers/hid/hidcontroller.cpp Outdated Show resolved Hide resolved
@ywwg
Copy link
Member

ywwg commented Oct 1, 2020

sorry for my lack of response, github notifies me about every single PR and doesn't highlight individual pings :/. Best to @ me on zulip to get my attention

@Holzhaus
Copy link
Member

Holzhaus commented Oct 2, 2020

github notifies me about every single PR and doesn't highlight individual pings :/

Btw, GitHub does show if you've been mentioned at least once in the PR, but it is non-obvious:

Screenshot_20201002_221104

It's probably still better to ping people on Zulip.

@JoergAtGithub
Copy link
Member Author

Merge?

@uklotzde
Copy link
Contributor

Safe to merge and after everyone approved the PR no reason for holding this back any longer. Thank your for contributing to Mixxx, Jörg. LGTM

@uklotzde uklotzde merged commit 3504684 into mixxxdj:master Oct 11, 2020
@JoergAtGithub JoergAtGithub deleted the HidController.send_feature_report branch October 13, 2020 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants