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

Jump to bottom

Fix MIDI sustain effect issue #10361

Merged
merged 1 commit into from
Oct 4, 2020
Merged

Fix MIDI sustain effect issue #10361

merged 1 commit into from
Oct 4, 2020

Conversation

3araht
Copy link
Contributor

@3araht 3araht commented Sep 19, 2020 •
edited
Loading

Description

When duplicate notes are pressed, the duplicate notes keep ringing or make the sustain effect until you unplug the USB cable.
For example, "duplicate notes" here means C, E and G when C chord (C, E, and G) and C7 chord (C, E, G, and A#) are pressed at the same time.
This issue can be replicated with GarageBand, Logic Pro, Piano 10 (Windows), etc.
(Ableton Live 10 didn't have this problem.)

A quick fix was found as the modification shown in the process_midi.c code.
Assuming QMK Firmware is mainly based on the "common sense" that there are no duplicate keys on the single keyboard.
Number keys on the upper row and num-pad keys are individual keys.
Probably handling duplicate keys are not well-developed because of such reason.
I guess something irregular happens when the same keys are pressed before the first one being released.

The modification is to suppress the second or further midi_send_noteon() if the note is already turned on.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Known Issues, which we define it is out of the scope of this push request

It should play the notes which are pressed all the time. However, in some sequences, the notes turn OFF even though the keys are remained pressed.
It happens when the same note(s) is/are played with multiple keys.
For example, when two keys are assigned for MI_C and when one of the keys is released after both were pressed, MI_C should stay ON since another key is still pressed. However, MI_C turns OFF when the first key is released.

Thanks to jakobkg and tzarc, the details are described here and here.

First of all, we'd like to get rid of the problematic sustain effect immediately by this push request.

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@tzarc
Copy link
Member

tzarc commented Oct 3, 2020

Would this not cause issues upon release?

Chord C State E State G State A# State
C down 1 1 1 0
C7 down 1 1 1 1
C up 0 0 0 1
C7 up 0 0 0 0

Seems like it'd be safer to modify the state to be an integer -- increment on any keydown (and only transmit for a 0->1 transition), and decrement on any keyup (and only transmit on a 1->0 transition).

@tzarc tzarc requested a review from a team October 3, 2020 18:49
@3araht
Copy link
Contributor Author

3araht commented Oct 4, 2020

Would this not cause issues upon release?

Chord C State E State G State A# State
C down 1 1 1 0
C7 down 1 1 1 1
C up 0 0 0 1
C7 up 0 0 0 0
Seems like it'd be safer to modify the state to be an integer -- increment on any keydown (and only transmit for a 0->1 transition), and decrement on any keyup (and only transmit on a 1->0 transition).

The symptom you pointed out is already there with the current version if you use Abelton Live 10 (Lite).
Thanks to jakobkg, he pointed out the symptom here.
This fix is a quick fix, and it will solve the sustain effect for GarageBand, Logic Pro X, and so on.
Then, such software will share the symptom you mentioned.

If your method requires a bit more time to realize, I'd like to make a baby step here to solve the sustain effect immediately and live with it for a while.
The sustain effect is the malicious issue we'd like to get rid of yesterday :-).

What do you think?

@tzarc
Copy link
Member

tzarc commented Oct 4, 2020

Thanks to jakobkg, he pointed out the symptom here.

Apologies for that -- didn't see that there was an attached issue.
If you're okay to get this in as-is, with a followup fix later to handle the down/up logic, then I'm okay with getting this in, in its current form. Please add some comments describing how it should work, so that if anybody comes across this issue and tries to fix it, they're able to ascertain that it's a known problem and needs fixing.

@3araht
Copy link
Contributor Author

3araht commented Oct 4, 2020

Thanks to jakobkg, he pointed out the symptom here.

Apologies for that -- didn't see that there was an attached issue.
If you're okay to get this in as-is, with a followup fix later to handle the down/up logic, then I'm okay with getting this in, in its current form. Please add some comments describing how it should work, so that if anybody comes across this issue and tries to fix it, they're able to ascertain that it's a known problem and needs fixing.

No problem at all. Thank YOU for the review!
I updated the push request statement. See above.

tzarc
tzarc approved these changes Oct 4, 2020
View reviewed changes
Copy link
Member

@tzarc tzarc left a comment

Choose a reason for hiding this comment

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

Weak approval; downsides are documented and are intended to be fixed up in a future PR.

@tzarc tzarc requested a review from a team October 4, 2020 06:01
@3araht
Copy link
Contributor Author

3araht commented Oct 4, 2020

Weak approval; downsides are documented and are intended to be fixed up in a future PR.

Thank you!
Well, the "downside" exists in the current software anyway so I'm sure this is a progress.

Erovia
Erovia approved these changes Oct 4, 2020
View reviewed changes
Copy link
Member

@Erovia Erovia left a comment

Choose a reason for hiding this comment

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

__attribute__((weak)) ✔️

@Erovia Erovia requested a review from a team October 4, 2020 11:17
drashna
drashna approved these changes Oct 4, 2020
View reviewed changes
Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

LGTM.

@drashna drashna requested a review from a team October 4, 2020 19:38
@drashna drashna merged commit 2bcac45 into qmk:master Oct 4, 2020
@3araht 3araht mentioned this pull request Oct 4, 2020
Closed
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Oct 7, 2020
@Shinichi-Ohki
Merge branch 'master' of https://github.com/qmk/qmk_firmware
15be960 
* 'master' of https://github.com/qmk/qmk_firmware: (178 commits)
  Docs: fix udev rules
  [CLI] Add c2json (qmk#8817)
  Improve LAYOUT macro searching (qmk#9530)
  CLI: update subcommands to use return instead of exit() (qmk#10323)
  Fixes small typo in docs (qmk#10515)
  Update personal keymap for Let's Split keyboard. (qmk#10536)
  [Keymap] Move my custom functions and keymaps to userspace (qmk#10502)
  [Keyboard] add support for ymd75 rev3 (qmk#10483)
  [Keyboard] Add soy20 PCB (qmk#10440)
  Fix for MIDI sustain effect issue (qmk#10361)
  format code according to conventions [skip ci]
  [Keyboard] Add Yugo-M Controller (qmk#10389)
  [Keymap] Add onekey keymap for OLED testing (qmk#10380)
  [Keymap] Add winterNebs keymaps  (qmk#10328)
  [Keymap] Added 333fred 5x6_5 keymap (qmk#10272)
  [Keyboard] Add hannah60rgb rev.2 PCB (qmk#10287)
  Adding VIA support to katana60 rev2 (qmk#10442)
  OLED driver fixes (qmk#10377)
  IS31FL3741 driver fixup (qmk#10519)
  add info.json for XD75 keyboard (qmk#10523)
  ...
barrettclark pushed a commit to barrettclark/qmk_firmware that referenced this pull request Oct 8, 2020
Merge remote-tracking branch 'upstream/master' into master
5a529c4 
* upstream/master: (81 commits)
  [Keyboard] New keyboard - eiri (qmk#10529)
  [Keymap] Add niu mini dye sub keymap (qmk#10525)
  Clean ChibiOS platform files (qmk#10505)
  [Keyboard] LeftyNumpad Keyboard (qmk#10500)
  [Keyboard] add maja capslock indicator (qmk#10151)
  Fix issue introduced by PR#10404 (qmk#10559)
  Docs: fix udev rules
  [CLI] Add c2json (qmk#8817)
  Improve LAYOUT macro searching (qmk#9530)
  CLI: update subcommands to use return instead of exit() (qmk#10323)
  Fixes small typo in docs (qmk#10515)
  Update personal keymap for Let's Split keyboard. (qmk#10536)
  [Keymap] Move my custom functions and keymaps to userspace (qmk#10502)
  [Keyboard] add support for ymd75 rev3 (qmk#10483)
  [Keyboard] Add soy20 PCB (qmk#10440)
  Fix for MIDI sustain effect issue (qmk#10361)
  format code according to conventions [skip ci]
  [Keyboard] Add Yugo-M Controller (qmk#10389)
  [Keymap] Add onekey keymap for OLED testing (qmk#10380)
  [Keymap] Add winterNebs keymaps  (qmk#10328)
  ...
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
@3araht
Fix for MIDI sustain effect issue (qmk#10361)
779692c
oscarcarlsson pushed a commit to oscarcarlsson/qmk_firmware that referenced this pull request Nov 2, 2020
@3araht
Fix for MIDI sustain effect issue (qmk#10361)
315b2c0
@3araht 3araht mentioned this pull request Nov 14, 2020
Closed
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Nov 24, 2020
@3araht @drashna
Fix for MIDI sustain effect issue (qmk#10361)
cbdcdef
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Jan 13, 2021
@3araht @drashna
Fix for MIDI sustain effect issue (qmk#10361)
5872797
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@3araht
Fix for MIDI sustain effect issue (qmk#10361)
fb97138
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna approved these changes

@tzarc tzarc tzarc approved these changes

@Erovia Erovia Erovia approved these changes

Assignees
No one assigned
Labels
bug core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@3araht @tzarc @drashna @Erovia @zvecr