corrados / edrumulus

Open Source E-Drum Trigger Module
GNU General Public License v2.0
97 stars 17 forks source link

Misc hihat pedal fixes #110

Closed 3hhh closed 11 months ago

3hhh commented 11 months ago

See the individual commits for details.

corrados commented 11 months ago

hihat pedal: remove ctrl_velocity_threshold

It just reduces the MIDI velocity obtainable by the pedal by a constant arbitrary factor. The control sensitivity & threshold can already be used for that.

No, I don't agree. With sensitivity&threshold you adjust the dynamic of your pedal. Usually, you would set these parameters that you achieve a 0 if the pedal is free (i.e., no foot on the pedal) and a 127 if it is completely pressed down.

The additional parameter (which is not configurable via the GUI at the moment) provides you with the flexibility to suppress very low velocity pedal presses. I use (and need) both of them in my setup.

corrados commented 11 months ago

Right now, the control_threshold is coupled with pad_settings.velocity_threshold and control_range is coupled with pad_settings.velocity_sensitivity. We have three parameters which are not yet controllable with the GUI: ctrl_history_len, ctrl_velocity_range_fact, ctrl_velocity_threshold.

Since positional sensing does not make sense for the pedal, I'll soon implement the following:

This will be a little break of compatibility since now the fundamental pedal parameters are controlled with the "positional" GUI parameters. But this makes sense since we control how the position of the pedal is converted to MIDI signals. The velocity threshold/sensitivity now configures the pedal stomp sound which makes sense to me.

The remaining parameter ctrl_history_len may be coupled with the GUI cancellation setting. I would like to re-use existing parameters since I want to avoid adding an additional GUI setting for this.

I have added this to the TODO list.

3hhh commented 11 months ago

The additional parameter (which is not configurable via the GUI at the moment) provides you with the flexibility to suppress very low velocity pedal presses. I use (and need) both of them in my setup.

Imho you only needed these constants because distances cannot be mapped to Midi velocity and you needed to accomodate for that. Yes, they are proportional, but that's it.

It's more reasonable to me to map the speed at which the pedal moves. The speed is proportional to (cur_ctrl_average - prev_ctrl_average)/ctrl_history_len and should be around 2*F*(cur_ctrl_average - prev_ctrl_average)/ctrl_history_len Midi range/s (the factor 2 is meant to accomodate for the averaging losing 1 bit). One would then need to find out from empirical evidence how fast a drummer can possibly move the pedal and define a linear function y = m*x +n with x = 2*F*(cur_ctrl_average - prev_ctrl_average)/ctrl_history_len from that. A negative n would be the offset of ignored MIDI velocity changes in that case. m and n don't necessarily need to be configurable unless there's a drummer who moves really fast/slow (for m) or there's someone around who likes to linger at around pedal Midi control value 99 and doesn't want it to trigger continuously as his foot is not that steady and the value goes up to 100 - 103 (n=4 case) from time to time (even then the resulting Midi note would have an imho negligible value of 1-3).

ctrl_history_len just needs to be large enough for a drummer to possibly reach the entire ADC range during that time frame as otherwise you'll only register those tiny changes from slightly below hi_hat_is_open_MIDI_threshold = 100 to slightly above 100.

corrados commented 11 months ago

It's more reasonable to me to map the speed at which the pedal moves.

That's basically what I am doing. What I calculate is an estimate of the gradient. This is equivalent to the speed the foot has hit the pedal.

One would then need to find out from empirical evidence how fast a drummer can possibly move the pedal [...] don't necessarily need to be configurable

You should always let the user customize the behavior of the foot stomp sound. It's similar to the normal pad sensitivity which will be specific to a players style. Some will hit the pedal stronger than others.

ctrl_history_len just needs to be large enough for a drummer to possibly reach the entire ADC range during that time frame

The reason for the buffering is simply a noise reduction of the ADC readings. The task is to calculate a gradient at the moment the pedal signal crosses the "closed hi-hat" threshold. You could simply use just two values but the result will be very noisy. If you average multiple samples and estimate the gradient based on these averages, the result will be much more stable. The length of the buffer should not be too long since then you would not only cover the transition phase but also some samples of the state when the pedal has already reached the bottom. But it is not only depending on the ADC range since the pedal might be already pressed down a bit for a while until the stomp happens.

Also, the larger the buffers are, the longer is the delay until a MIDI note will be played.

3hhh commented 11 months ago

It's more reasonable to me to map the speed at which the pedal moves.

That's basically what I am doing. What I calculate is an estimate of the gradient. This is equivalent to the speed the foot has hit the pedal.

My main point was that currently the code is not dividing by ctrl_history_len. This may have somewhat worked in the past when it was a constant. However if it becomes a variable, other variables (e.g. the distance travelled during a pedal press) will depend on it. Configuration variables should be as independent as possible though.

So making all of these variables configurable would help, but also be rather inconvenient (changing one variable would require changing other 1-2 dependent variables).

ctrl_history_len just needs to be large enough for a drummer to possibly reach the entire ADC range during that time frame

The reason for the buffering is simply a noise reduction of the ADC readings. The task is to calculate a gradient at the moment the pedal signal crosses the "closed hi-hat" threshold. You could simply use just two values but the result will be very noisy. If you average multiple samples and estimate the gradient based on these averages, the result will be much more stable. The length of the buffer should not be too long since then you would not only cover the transition phase but also some samples of the state when the pedal has already reached the bottom. But it is not only depending on the ADC range since the pedal might be already pressed down a bit for a while until the stomp happens.

Also, the larger the buffers are, the longer is the delay until a MIDI note will be played.

Let me correct myself: ctrl_history_len needs to be large enough to obtain log_2(127)=7 bit range. ADC range is indeed not needed, but anything below 7 bits will provide less than the available pedal dynamics. With the current value I observed 1-2 bit range with my pedal, which I consider not enough.

corrados commented 11 months ago

My main point was that currently the code is not dividing by ctrl_history_len.

Hmm, I actually do divide by the length:

https://github.com/corrados/edrumulus/blob/db9d2e0c0284790e65d1e90261d41987366bdd60/edrumulus.cpp#L1512-L1513

Let me correct myself: ctrl_history_len needs to be large enough to obtain log_2(127)=7 bit range.

I do not get your point. As I said, the history is only for noise reduction. It is not to improve the precision. You actually shrink the dynamic a bit by enlarging the history length.

E.g., if you assume that the pedal goes down and the history length is quite small, you will estimate the gradient pretty well (blue circles gradient is the same as the black line). But in the second example where the pedal goes up and down, the estimated gradient is approx. zero but the true gradient at the end of the second red region is the same as in the first example: grafik Therefore, the history length is a trade-off between noise reduction and estimation error of the gradient.

3hhh commented 11 months ago

My main point was that currently the code is not dividing by ctrl_history_len.

Hmm, I actually do divide by the length:

https://github.com/corrados/edrumulus/blob/db9d2e0c0284790e65d1e90261d41987366bdd60/edrumulus.cpp#L1512-L1513

Those lines are just part of the averaging (you just remove the dependency on ctrl_history_len which you had introduced here). prev_ctrl_average and cur_ctrl_average are still absolute numbers/distances in the range of 0..127. They aren't speeds/gradients as you even acknowledge here. (cur_ctrl_average - prev_ctrl_average) / ctrl_history_len is a gradient/speed. Averaging over distances doesn't make them speeds. If I for example average over the numbers 1,2,3,4,5,6 we'll obtain 3.5, but not the gradient 1. If I take the upper half as you currently do, I'll obtain 7.5 and 3 for the lower. Subtracting still gives me an absolute 3.5, which is the average. The gradient can be approximated as 2*3.5/6 as I had previously mentioned.

And since you're currently mapping a difference in distances to MIDI velocities the history length matters as obviously I can reach the value of 127 from 0 (average difference of 63) in maybe 0.1s, but likely not in 0.001s (maybe you'll see a window of averages such as 103-95=8 or so in that time frame). So the history length could be smaller (assuming a perfectly linearly applied pedal strength), if you actually were using gradients, but currently you're not so it needs to be somewhat large. I still suggest switching to gradients.

corrados commented 11 months ago

Ok, now I get your point and you are absolutely right. I have to admit that I implemented the "pedal stomp support" in a hurry without spending too much time on algorithm design... ;-)

The good thing is that the current implementation still does what it is supposed to do since the length of the history is assumed to be constant. In that case the subtraction is proportional to the gradient.

But I agree with you, I should switch to a true gradient calculation for the reason you already mentioned that we eliminate the unnecessary dependencies of the configuration parameters.

corrados commented 11 months ago

Coming back to your remark:

ctrl_history_len just needs to be large enough for a drummer to possibly reach the entire ADC range during that time frame as otherwise you'll only register those tiny changes from slightly below hi_hat_is_open_MIDI_threshold = 100 to slightly above 100.

I multiply the averaged MIDI note differences with a factor of 4 (ctrl_velocity_range_fact) and I can easily reach a pedal stomp MIDI note of 127. That concludes that the difference between the two MIDI not averages is at least 32. Of course, if you hit the pedal with low velocity, you will get something like you mentioned, i.e., the values are slightly above/below the value of 100. But I would not like to increase the buffer size (and therefore increase the latency) just to improve the precision of the velocity estimation of these very soft pedal stomp actions. The length of the history buffer should just be as long as it is required to reduce the ADC noise sufficiently.

corrados commented 11 months ago

I have now fixed the gradient calculation for the pedal stomp detection. I scaled the constants that it should behave the same as the previous code.

3hhh commented 11 months ago

OK, now I got my pedal somehwat running with ctrl_velocity_range_fact = 80 and ctrl_velocity_threshold = 0 (my gradient is in the range of 0.5 - 1.2). Also - as expected - changing ctrl_history_len has no effect apart from the intended noise filtering anymore. I still observe very strong cross-talk with the hihat itself, but I'll create a separate issue for that.

corrados commented 11 months ago

Now I have implemented the support to configure the pedal stomp parameters in the Edrumulus GUI. For this, I have moved the settings for the pedal position to "pos thres" and "pos sens" and the new stomp parameters are the "thresh" and "sens" (which were previously the pedal position parameters):

https://github.com/corrados/edrumulus/commit/095c44c50b986f5a0f90de62f5bb600cd560a2e5 https://github.com/corrados/edrumulus/commit/ff1bd9347e9d22057ac1a482a0b7d3801eda42f3

3hhh commented 11 months ago

Nice, thanks!