Allen-Synthesis / EuroPi

EuroPi: A reprogrammable Eurorack module based on the Raspberry Pi Pico
Creative Commons Zero v1.0 Universal
431 stars 84 forks source link

fixed right possibility to 0-100 range instead of 0-99 #206

Closed gamecat69 closed 1 year ago

gamecat69 commented 1 year ago

percent() only returns values from 0-0.99 so never actually gets to 100. In this script, it meant that the right possibility could never get to 100%. This fix ensures that reading a knob value always returns a 0-100 value.

mjaskula commented 1 year ago

[discussion] This sounds like a hardware issue.

percent() only returns values from 0-0.99 so never actually gets to 100.

This is not the intention of this method. It should return 0-100. I just ran this program as is (with out this change) in my hardware and I had the full range of percentages available to me on both knobs. (the UI showed 0.00 fully CCW and 1.00 fully CW)

I don't think that this fix is necessary.

gamecat69 commented 1 year ago

Thanks for reviewing. Your point sent me on a bit of a testing rampage across all my EuroPis! I found that in some cases neither Bernoulli gate ever gets to 1.00 and in other cases only one gate gets to 1.00.

I then wrote a simple test script (below) and discovered that percent doesn't ever return 1.00, the highest it ever gets to is 99.n. If others experience the same (more testing required...) do you think we may need something updating in the firmware to round up the return from percent() to 1 if it's above 99.6 say? Additionally, I think it may be noise that is preventing percent from ever reading 1.00... if you look at _sample_adc() in the Analogue Reader class it produces a mean average of all samples... therefore the value would always be dragged down to a value lower than 1.00 by noise while reading the samples.

    def main(self):
        while True:
            # get pot values
            self.k1percent = round(k1.percent() * 100,1)
            self.k1readposition = k1.read_position(100)

            self.k2percent = round(k2.percent() * 100,1)
            self.k2readposition = k2.read_position(100)

            # display on screen
            oled.fill(0)
            oled.text('k1: ' + str(self.k1percent) + '% | ' + str(self.k1readposition), 0, 0, 1)
            oled.text('k2: ' + str(self.k2percent) + '% | ' + str(self.k2readposition), 0, 12, 1)
            oled.show()
gamecat69 commented 1 year ago

An updated _sample_sdc() might look something like this:

    def _sample_adc(self, samples=None):
        # Over-samples the ADC and returns the average.
        values = []
        for _ in range(samples or self._samples):
            values.append(self.pin.read_u16())
        val = round(sum(values) / len(values))
        if val < 190:
            return 0
        else:
            return val

Which would be one way to fix the percentage issue across multiple hardware builds. However, it looks like the range() function would never return the highest value because it always deducts 1.... this would need updating as well... unless I am misunderstanding something ...

    def range(self, steps=100, samples=None):
        """Return a value (upper bound excluded) chosen by the current voltage value."""
        if not isinstance(steps, int):
            raise ValueError(f"range expects an int value, got: {steps}")
        percent = self.percent(samples)
        if int(percent) == 1:
            return steps - 1
        return int(percent * steps)
gamecat69 commented 1 year ago

This is one method (I think) to fix the range function

    def range(self, steps=100, samples=None):
        """Return a value (upper bound excluded) chosen by the current voltage value."""
        if not isinstance(steps, int):
            raise ValueError(f"range expects an int value, got: {steps}")
        percent = self.percent(samples)
        if int(percent) == 1:
            return steps
        return int(percent * steps)
gamecat69 commented 1 year ago

Suggest this PR is closed in favour of the raised issue: https://github.com/Allen-Synthesis/EuroPi/issues/209