gligli / p600fw

Teensy++ based Sequential Circuits Prophet 600 firmware remake
http://gligli.github.io/p600fw/
41 stars 23 forks source link

Release #44

Closed polluxsynth closed 9 years ago

polluxsynth commented 9 years ago

These commits add a dead band for the bend wheel and zero-center parameters. This attempts to fix issue #24.

I've left the individual commits in place, feel free to squash them to one if you want to.

A couple of comments:

  1. I've tested that the bend range ends up being exactly what it is set to, i.e. if set to an octave, operating the bend wheel will bend exactly an octave up or down. My friend who actually owns the P600 I'm working on uses this feature from time to time.
  2. In order to reach the complete range, I've added a 'guard' zone in the case of the bend wheel, which is a range at the end of the pot travel where the resulting value from the ADC doesn't change. There is really the same issue with all panel pots (on the machine I have here, none of them display more than 96 or 97 as their maximum), but in trying to keep the behavior consistent across firmware upgrades, I haven't added a guard zone for the panel pots. It could easily be done if needed.
  3. The dead band and guard band settings have been done empirically using the P600 I have at hand. Please do check if they seem ok for your machine too. There are separate settings for the pitch wheel and panel pots.
  4. The pot value display in non-preset mode still reflects the actual pot value, not the deadband-adjusted value. I'm not sure if this is good or bad...
  5. I got a bit carried away and implemented a pre-run calculation of the dead band factor. The reason is that I figure since the CPU doesn't have a division instruction, but does have multiplication instructions, division operations will be costly in terms of execution time, compared to multiplication. So instead of doing a multiplication and subsequent division to rescale the bender value, I multiply by a pre-calculated factor that leaves the resulting value in the high word of the 32-bit result, which can then be simply shifted down (with a bit of luck the compiler will just fetch the high word from the relevant registers without doing any actual shifting operations). I realize though that there are lots of other division operations so just omitting the one in the case of bend will not make a huge (or even noticeable) difference, especially since the calculation only takes place while operating the bend wheel.
gligli commented 9 years ago

Seems great, I will merge it right away!

4) I think it's better to display the deadband adjusted value, to give the user visual feedback of what really happens, otherwise it could come up as broken/buggy. 5) That makes complete sense, division is really slow, 32bit ints are slow, freeform shifts are slow (not a multiple of 8) and there's not that much CPU time left!

gligli commented 9 years ago

2) You need to do the DAC offset adjustment procedure, it will fix it.

polluxsynth commented 9 years ago

4) Ok, I'll see if I can have a look at this too. 5) There are several more places that division are used which could be changed too.

2) Ok, I'll try this and see how it goes. Do the pots on your machine register up to 99 (instead of 96 or 97) then?

One thing I thought of is to add rounding to the deadband calculation, as it stands the first step up from the middle value is smaller than down which I think is due to the calculations being rounded down. Not a big issue, but I'll see if I can check it quickly tonight.

I'll be out of town next week so it'll be a while before I get back to any major P600 work however.

gligli commented 9 years ago

I changed 4) on my branch already. For 5) I changed one in ui value display but didn't try to find them all.

Yes my pots go from 0 to 99, DAC adjustment was the first thing I did on my P600.

You mean a 16bit step? Don't bother, the DAC is only 14bit :)

Ok, I'm going to work on transposing the internal keyboard / arp then build a new alpha this afternoon I think.

polluxsynth commented 9 years ago

Regarding the DAC adjustment, I assume you mean "DAC gain" (section 1-9 in the service manual)? If I remember the problem correctly it was not just upwards, it was also downwards that it didn't bend the correct interval. The display reads -50, but the actual value is apparently not really 0 when the pot is at the bottom. The difference wasn't great, but adding the 'guard band' feature fixed it, so that it now bends down exactly an octave when so requested.

You are right that there are only 14 bits used on the DAC of course (hadn't though of it), however, that would really just mean that the resulting value should be rounded up to the next 14 bit rather than next 16 bit step.

Regarding the 32-bit divisions, there are a couple of /=12 in computeBenderCV's which should be easy to fix, as well as a /=UINT16_MAX which maybe could be replaced by a =>>16, unless it really is necessary to divide by 65535 rather than 65536?

polluxsynth commented 9 years ago

It occurred to me that if any rounding is to be done it needs to be done after scaling the bender value according to the current bend range (bender semitones), i.e. in computerBenderCVs() rather than in the deadband calculation.

polluxsynth commented 9 years ago

I opened up the machine to check the DAC gain adjustment. First of all, strangely enough, the service manual says that machines with a Burr-Brown DAC71 fitted to not have the DAC gain trimpot R4333 mounted. Yet this machine does, despite having a DAC71.

Checking the adjustment procedure, it says to turn the mixer A knob (which I know is now osc A level only) fully up and check for a 4.9V reading a U426 pin 7,which is exactly what I get on my machine. So it would seem that this adjustment at least is ok. I still only get a max pot reading of 96 or 97 in the display. Odd.

polluxsynth commented 9 years ago

Fiddling a bit with computeBenderCV's reveals that, as you suspected, adding rounding does not really make a difference, and furthermore, the most accurate octave bend at least on the machine I have is achieved with /=65535. So I won't be trying to change anything in this region for now anyway.

polluxsynth commented 9 years ago

Sorry for the spam. Turns out (as should be expected) there's no practical difference between /=65535 and /=65536, the difference I thought I heard between then two was simply due to different calibration between the difference voices. So I will replace the /=65535 and /12 with shift preceded by (in the latter case) a multiplication operation.