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

Improve documentation on Hercules Inpulse 300 controller #425

Merged

Conversation

michalvankodev
Copy link
Contributor

I've made some improvements for Hercules Inpulse 300 controller here in PR: mixxxdj/mixxx#4246 (review)

The controller manual page has been improved and the changes reflected are updated in this PR.

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.

Thank you very much for massively improving the manual page for this controller.

@@ -54,58 +54,120 @@ Controls not included in this mapping
- Beatmatch guide (Hardware control)
- PADS: Slicer/Slicer Loop
- PADS: Toneplay
- PADS: FX
Copy link
Member

Choose a reason for hiding this comment

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

Please move this "not working" list to a new "Known Issues" section at the bottom of the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved and created a new section:
cd53b3e

control effect and the mapping might be modified eventually to take
advantage of this possibility.
send multiple and different Note and CC messages. It is recommended to apply **Firmware
v1.72** update, that will patch each FX pad to send a simple Note On/Note Off (as is the case
Copy link
Member

Choose a reason for hiding this comment

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

So the mapping only works correctly with firmware 1.72 or later? I suggest rephrase this to make it clear that users should upgrade and move this note into the compatibility section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well.. it works with SHIFT+Pad without the upgrade but for the functionality without SHIFT it requires upgrade. I've bought my Inpulse just last month and it was already upgraded to the latest firmware.

Do you have a suggestion for the rewording?

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've tried something
cd53b3e

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

I still think that this note is confusing to regular users that are not familiar with the MIDI spec and don't know what "cc messages" are.

I suggest something concise that also mentions what happens if you don't upgrade:

Please upgrade the controller firmware to 1.72 or later. With older firmware versions, pressing SHIFT + Pad will do nothing.

The technical details could be added as a comment to the source instead.

Copy link
Contributor Author

@michalvankodev michalvankodev Sep 1, 2021

Choose a reason for hiding this comment

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

Moved it into Compatibility section
09934aa

With older firmware the pads will only work with SHIFT + Pad

@uklotzde
Copy link
Contributor

uklotzde commented Sep 5, 2021

Please fix the pre-commit issues, probably just trailing whitespace.

@michalvankodev
Copy link
Contributor Author

Please fix the pre-commit issues, probably just trailing whitespace.

I've fixed the trailing whitespace issue but the build is still failing due to invalid links in other manuals.

@Holzhaus
Copy link
Member

Holzhaus commented Sep 8, 2021

Please fix the pre-commit issues, probably just trailing whitespace.

I've fixed the trailing whitespace issue but the build is still failing due to invalid links in other manuals.

Yeah, don't worry about that. Looks like Numark forgot to update their SSL certificate.

@uklotzde
Copy link
Contributor

@Holzhaus I guess your comments have already been resolved? Will merge this now after I already merged the mapping.

@uklotzde uklotzde merged commit 7730407 into mixxxdj:2.3 Sep 17, 2021
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.

3 participants