Open MrMep opened 3 months ago
7680 << this shouldn't be here
The issue occurs here:
In particular, the "odd" value seen is the one taken here:
holeLatched should contain the slide hole set to zero, but in the meanwhile the value for that hole in fp.holes (holeCovered) has changed. I don't understand what that line is supposed to do: it says "Max downward", but that value is not maximum, for me it's currently 512 (1024/2), which seems oddly the same as default values.
If modified in
if (bitRead(currentFP.fp.holes, i) == 1 && !tf.timing ) {
it works, but I am not sure that is a solution or
there's something else going on.
EDIT: No, it doesn't work, it just sets pitchbend to zero. The problem is that adjvibdepth
doesn't contain the maxed value, I don't understand why... I'll try again tomorrow
Another issue I found with the transient filter involves silent notes
A check was missing in debounceFingerHoles
I merged a new PR, let me know
Yep, the silent notes seem correct now. Thanks!
I haven't been able to figure out the pitchbend thing yet either, but I think this line is working correctly:
iPitchBend[i] = adjvibdepth; // Set vibrato to max downward bend
...it should be setting vibrato to the depth currently set by the vibrato depth slider, which is 512 by default. The purpose of that line is that when you are using vibrato and you lower a finger all the way, it triggers a new note but the vibrato pitchbend should still be set to the max. We have to do that deliberately because that finger is no longer in the "middle" range that would normally trigger pitchbend.
It seems to me like that vibrato stuff is working correctly, and the issue is coming from the slide pitchbend (calculated in getSlide() ) not being reset at the same time as a new note is being triggered by the transition filter. I assume that the currentFP.fp.holes is the fingering pattern after being filtered, correct? If so, I'm not sure why slide isn't working correctly unless there's somewhere that the unfiltered hole states are being used instead. This seems plausible because the delay between the incorrect pitchbend value and the new triggered not gets audibly longer when the transition delay is set longer.
I assume that the currentFP.fp.holes is the fingering pattern after being filtered, correct?
No, that was the problem: currentFP.fp.holes is exactly the same as the previous holeCovered (I changed its name back to holeCovered in my new PR, to avoid this confusion).
I'm not sure why, but with the new transient filter, it doesn't work any more because, yes, currentFP.fp.holes has changed, PB is set to zero and vibrato gets in the way. I changed a couple of lines and it looks to me it's working, let me know.
As for the Half-Thumb-in-the-middle" request, I managed to get to it.
I moved the last conditional in getShift:
to getNote and left the same conditions.
Here's how I organized the octave matrix for the fingerings in that conditional (if ((breathMode == kPressureThumb && (modeSelector[mode] == kModeWhistle || modeSelector[mode] == kModeChromatic || modeSelector[mode] == kModeNAF))
) :
` Octave Matrix
THUMB REGISTER ON
HALF THUMB ON
_________________________________________________________________________________________________
| Invert Thumb/Bell | Invert Half Thumb | 1st octave | 2nd octave | 3rd octave |
_________________________________________________________________________________________________
| off | off | closed | open | half |
| on | off | open | closed | half |
| off | on | closed | half | open |
| on | on | open | half | closed |
_________________________________________________________________________________________________
`
I'm pushing it, let me know if that does what you wanted. Of course, there shouldn't be problems for other fingerings, but you never know, let me know.
gl
The thumb hole stuff seems to work for me, thanks! At some point I'll see what happens if we enable that for custom charts too.
The slide does seem better. An interesting thing happens when the transition filter is set to longer times-- the slide has more time to bend all the way to the next note and so you get more of a "blip" sound when the new note is triggered, but that makes sense and I think it's unavoidable with any filter that adds a delay.
I am seeing an issue with slide and the thumb hole: With the "normal" slide/vibrato mode, the thumb is triggering a large pitchbend value if the transient filter is set to anything other than 0 and the "invert thumb/bell" switch is turned on. It doesn't happen with the legato/slide vibrato mode, and it doesn't matter if half holing is turned on. There shouldn't ever be any pitchbend associated with the thumb. It's possible this was already in there, but I haven't noticed it until now. Edit: I checked and this wasn't happening with v 4.1 and the old transition filter.
so you get more of a "blip" sound
the thumb is triggering a large pitchbend value if the transient filter is set
I'll have a look at that this weekend and see if I can fix them both. Keep 'em coming! 👯♂️
Thanks! I don't think the first one is a bug, rather just an artifact of having more time to detect the finger location before triggering a new note. @essej may have thoughts on that later.
I now see that there is an option to use the thumb for vibrato (not enabled by default) but it shouldn't ever trigger any slide.
I now see that there is an option to use the thumb for vibrato (not enabled by default) but it shouldn't ever trigger any slide.
I can't see how it wouldn't: all for loops include the thumb hole, including the one in getSlide() in handlePitchBend() there's a filter on vibratoHoles, but in getSlide() I can't see anything like it. What are we missing?
It looks like the difference in behavior is with line 1769:
int offsetSteps = findStepsOffsetFor(i);
That function used to return a 0 for the thumb when using the tin whistle fingering because the thumb register control was added in getShift(), but it's now moved to getNote(), so getNote() returns an offset for the thumb now. It would be nice if the thumb register stuff could remain in getShift() because that's where the rest of the register control is done and I think it's going to cause a number of problems elsewhere if it's in getNote(). There are several places where we need to get a note from that function without having a register offset. If you can roll back your changes involving the thumb register I'll just play with that at a later time. Thanks!
That function used to return a 0 for the thumb when using the tin whistle fingering
Oooh, I see it now, thanks!
If you can roll back your changes involving the thumb register
I moved that "shift" there for two reasons:
I'll first move back those lines to getShift, as you asked, then I'll wrap my head around 2. to see if there's an alternative...
EDIT: by the way, if sliding on the thumb hole is not an option, and never will be, wouldn't it be better if we avoided calculating slide for it at all?
EDIT 2: merged PR for 1.
Thanks -- I see now that I was mistaken about slide never being used for thumb. It isn't used for tin whistle but it is used for fingering charts that use the thumb. I'm not sure if that's necessary, though. If we keep it that way I guess I need to make it so that that slide will be disabled if the thumb is used for half-holing (currently slide is allowed if only the thumb is used).
I wouldn't spend any more time on this stuff now-- I need to give some thought about whether to include it all, as I didn't realize quite how much complexity the transition filter adds. I don't want to waste your time tracking down all these little things right now.
Yes, slide is a bit fancier than it once was since I did my changes a while ago…. It’s not just about the “next” hole anymore, and it works in all sorts of fingerings. Also, I actually can see the benefit of moving the shift register out of its own function and becoming something that can be transient filtered because register blips are actually pretty common too, and could be helped by the filter.
the benefit of moving the shift register out of its own function
Maybe, not to run the risk of breaking some other stuff, we could split getShift in two: byte calculateShift() and getShift(). This way we could consider the (future) shift already during transitions, but actually apply it afterwards, as it is now.
I wouldn't spend any more time on this stuff now
Yes, unless otherwise explicitly required, I wouldn't modify the develop branch until you've sorted out what to include in the next release.
I think the confusing thing for me is that with the old transition filter getShift() was called after the tone holes had already been debounced, correct? But it sounds like you are saying that the toneholes might have changed between when getNote() is called and when getShift() is called?
it sounds like you are saying that the toneholes might have changed between when getNote() is called and when getShift() is called?
No, the old filter considered toneholes only, so all was good. The new one considers note intervals too, so it may base its calculations on false data: the "pending note", when debounced, may be then modified by getShift. Evidently, it's best to leave getShift as is, but we could anticipate its effects and consider them in the transition filter, and apply them only when debouncing is done, as it is now.
It's easier done than said :)
There are already other new ideas for the transition filter, but we can evaluate all of that later: what do you think?
Ah I see, thanks. I'll try to get a better understanding of how the transition filter works while I'm away this week. I'll need to test it with overblowing at some point too because overblowing needs to know the fingering pattern but also changes the interval. It's always been a little tricky maintaining the order of operations with that, which is why getState() is currently called twice (before debounceFingerHoles() and after).
I just submitted a new PR https://github.com/amowry/WARBL2/pull/17 in the develop branch of this repository.
If you can compile and test the new modifications, you are more than welcome to do that and report back here or in the PR discussion thread.
Here's an extract from the PR description.
First of all, there's support for half-holing.
EDIT:
It is limited to:The user can activated the half-hole detection for each single supported tonehole. In the "Advanced" settings for half-holing it is possibile to tweak the sensitivity of the detection, hopefully to accomodate different playing (and instrument gripping) styles.
The current hardware finger sensing mechanism doesn't allow for precise half-hole sensing, so a lot of changes had to be made to the debouncing machine. Basically, the transient note filter delay is now dynamic: it adjusts based on the player's gestures, and it extends or reduces the delay consequently. For now, gesture detection is limited to note interval, number of fingers changed and status of half-holes. It can be extended to other types of gesture detection. I removed the previous sensing based on senseDistance, because it is redundant at this point. Being dynamic, the "sluggish" feeling should be reduced a lot, while most of the glitches are more effectively removed.
I added my own EWI Fingering ("Mep's EWI") and the relative fingering PDF chart. Based on this, there a new "Mep's Recorder" that takes advantage of half-holing and has a lot of alternate fingerings (all based on real acoustic recorders). Now the recorder fingering is exactly the same as real recorders. After validation, it could replace the current Soprano recorder fingering (and drop the "Mep's" part ;)
Config Tool I did my best to follow Andrew's style and prefs: of course, @amowry feel free to move things around as you see fit.
A few notes about the source code:
- You'll see portion of code related to a "baseline auto-calibration", but this seems redundant at this point and can be possibly removed before merging.EDIT: @amowry added a flash.uf2 file of this beta version, just drag&drop to install: https://github.com/amowry/WARBL2/blob/develop/warbl2_firmware/build/adafruit.nrf52.warbl2/flash.uf2
EDIT 2: I fixed a few things and added an
autocontinuous optical calibration for toneholeCovered