sine_wave likely to generate value too large for "H" array.array

[kevinjwalters]

When length is even this is likely to try to assign 65536 (depending a little on floating point part of calculation and math functions) to a 16bit unsigned value, i.e. it will overflow it. The multiplier of 2*15 (32768) is too large, value-between-minus1-plus1-at-plus1 32768 + 32768 = 65536.

    b = array.array("H", [0] * length)
    for i in range(length):
        b[i] = int(math.sin(math.pi * 2 * i / length) * (2 ** 15) + 2 ** 15)

The same code is used in various places. I'd suggest a scan across alll Learn guides and adafruit's whole repo.

Reported/discussed on: https://forums.adafruit.com/viewtopic.php?f=60&t=153174

Is https://github.com/adafruit/circuitpython/pull/1860 or something similar the reason why this has gone unnoticed?

[dhalbert]

Yes, it's because #1860 is recent and is more strict. So we need min(<result>, 65535) or similar.

[kevinjwalters]

Thanks for confirmation.

I was thinking about "when length is even" on the way home and for sine this is wrong, it's when it's exactly divisible by four. The example was 440Hz at 16000Hz sample rate which with application of int() gives 36.

[kevinjwalters]

FYI, the sine wave sample generator in circuit_playground library is ok as it uses a 32767 multiplier: https://github.com/adafruit/Adafruit_CircuitPython_CircuitPlayground/blob/master/adafruit_circuitplayground/express.py#L566

[kevinjwalters]

There is also the signed variant of same problem, the python example code in https://github.com/adafruit/circuitpython/blob/4.x/shared-bindings/audioio/RawSample.c#L65 using "h" with 32768 multiplier only works for example values because length comes out as 18 - a 220Hz wave at 8k would blow up too although code has a second fault where 18 is hardcoded into the maths.

[tannewt]

@kevinjwalters would you mind fixing these bugs? They are my mistake from when I implemented it bug I'm currently busy hunting deeper bugs. Thanks!

[kattni]

@kevinjwalters @tannewt Has this been addressed or is it still outstanding?

[tannewt]

@kattni Not sure. I think it needs to be retested. @kevinjwalters may know.

[kevinjwalters]

The code in this project is still broken: https://github.com/adafruit/Adafruit_CircuitPython_Waveform/blob/master/adafruit_waveform/sine.py#L44

My second point was that a good scan (e.g. grep) of all the source code and learn guides is worthwhile as this sample code has been copied all over the place.

[kattni]

@kevinjwalters Do you have suggested a fix for it? Or was your reference to the Circuit Playground library a suggestion to use the same math here?

[sayler]

>>> sine.sine_wave(100,1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/sayler/Projects/Adafruit_CircuitPython_Waveform/adafruit_waveform/sine.py", line 44, in sine_wave
    b[i] = int(math.sin(math.pi * 2 * i / length) * (2 ** 15) + 2 ** 15)
OverflowError: unsigned short is greater than maximum

The easiest thing here is to just slightly reduce the range.