adafruit / Adafruit_CircuitPython_BLE_Adafruit

Support for the BLE Adafruit Service, which provides access to on-board sensors and components
MIT License
9 stars 6 forks source link

fix mic on Clue demo #22

Closed tlyu closed 2 years ago

tlyu commented 2 years ago

The numpy in ulab doesn't seem to convert the result types the same way as regular numpy, so the result of the mic_samples subtraction had dtype=float, which was too big for that characteristic and caused an error once an app connected.

dhalbert commented 2 years ago

@tlyu Could you describe the ulab problem in more detail? There may be a bug to fix in numpy. Tagging @v923z

tlyu commented 2 years ago

Quick example: (all result types should be np.uint16, from what I understand)

ulab (on CircuitPython 7.3 on a Clue):

>>> a = np.zeros(8, dtype=np.uint16)
>>> a
array([0, 0, 0, 0, 0, 0, 0, 0], dtype=uint16)
>>> a - 32767
array([32769, 32769, 32769, 32769, 32769, 32769, 32769, 32769], dtype=uint16)
>>> a - 32768
array([-32768.0, -32768.0, -32768.0, -32768.0, -32768.0, -32768.0, -32768.0, -32768.0], dtype=float32)

regular numpy:

In [18]: a
Out[18]: array([0, 0, 0, 0, 0, 0, 0, 0], dtype=uint16)

In [19]: a-32767
Out[19]:
array([32769, 32769, 32769, 32769, 32769, 32769, 32769, 32769],
      dtype=uint16)

In [20]: a-32768
Out[20]:
array([32768, 32768, 32768, 32768, 32768, 32768, 32768, 32768],
      dtype=uint16)
v923z commented 2 years ago

@tlyu I think the problem is in https://github.com/v923z/micropython-ulab/blob/d438344943ab4383dae86b4620075ea877dec535/code/ndarray.c#L1683, and it is the consequence of how dtypes are promoted in ulab: https://micropython-ulab.readthedocs.io/en/latest/ulab-ndarray.html?highlight=upcasting#upcasting

We could definitely change the rules, if you judge compatibility to be important. As I explained there, we cannot possibly support full compatibility, because there are only 5 dtypes in ulab, whilte numpy supports 13.

I would suggest that you open a ticket at https://github.com/v923z/micropython-ulab/issues, so that we can track this.

tlyu commented 2 years ago

@dhalbert I would like to suggest merging this workaround patch for now, because (last time I checked) the demo crashes as soon as it receives a connection from a mobile client. It's not completely clear what the right upstream change to ulab would be, and it might take a while to work that out.