Merge pull request #7294 from tannewt/analogbufio_rework

Rework the analogbufio API.

Author: tannewt

locale/circuitpython.pot

@@ -155,6 +155,9 @@ msgid "%q must be >= %d"
 msgstr ""
 
 #: shared-bindings/analogbufio/BufferedIn.c
+msgid "%q must be a bytearray or array of type 'H' or 'B'"
+msgstr ""
+
 #: shared-bindings/audiocore/RawSample.c
 msgid "%q must be a bytearray or array of type 'h', 'H', 'b', or 'B'"
 msgstr ""

ports/raspberrypi/common-hal/analogbufio/BufferedIn.c

@@ -33,6 +33,7 @@
 #include "common-hal/analogbufio/BufferedIn.h"
 #include "shared-bindings/analogbufio/BufferedIn.h"
 #include "shared-bindings/microcontroller/Pin.h"
+#include "shared/runtime/interrupt_char.h"
 #include "py/runtime.h"
 #include "supervisor/shared/translate/translate.h"
 #include "src/rp2_common/hardware_adc/include/hardware/adc.h"
@@ -42,59 +43,32 @@
 #define ADC_FIRST_PIN_NUMBER 26
 #define ADC_PIN_COUNT 4
 
-void common_hal_analogbufio_bufferedin_construct(analogbufio_bufferedin_obj_t *self, const mcu_pin_obj_t *pin, uint8_t *buffer, uint32_t len, uint8_t bytes_per_sample, bool samples_signed, uint32_t sample_rate) {
+#define ADC_CLOCK_INPUT 48000000
+#define ADC_MAX_CLOCK_DIV (1 << (ADC_DIV_INT_MSB - ADC_DIV_INT_LSB + 1))
 
+void common_hal_analogbufio_bufferedin_construct(analogbufio_bufferedin_obj_t *self, const mcu_pin_obj_t *pin, uint32_t sample_rate) {
     // Make sure pin number is in range for ADC
     if (pin->number < ADC_FIRST_PIN_NUMBER || pin->number >= (ADC_FIRST_PIN_NUMBER + ADC_PIN_COUNT)) {
         raise_ValueError_invalid_pins();
     }
 
+    // Validate sample rate here
+    sample_rate = (uint32_t)mp_arg_validate_int_range(sample_rate, ADC_CLOCK_INPUT / ADC_MAX_CLOCK_DIV, ADC_CLOCK_INPUT / 96, MP_QSTR_sample_rate);
+
     // Set pin and channel
     self->pin = pin;
     claim_pin(pin);
 
     // TODO: find a way to accept ADC4 for temperature
     self->chan = pin->number - ADC_FIRST_PIN_NUMBER;
 
-    // Set buffer and length
-    self->buffer = buffer;
-    self->len = len;
-
-    // Set sample rate - used in read
-    self->bytes_per_sample = bytes_per_sample;
-    self->sample_rate = sample_rate;
-
     // Init GPIO for analogue use: hi-Z, no pulls, disable digital input buffer.
+    // TODO: Make sure we share the ADC well. Right now we just assume it is
+    // unused.
     adc_init();
     adc_gpio_init(pin->number);
     adc_select_input(self->chan); // chan = pin - 26 ??
 
-    // RP2040 Implementation Detail
-    // Fills the supplied buffer with ADC values using DMA transfer.
-    // If the buffer is 8-bit, then values are 8-bit shifted and error bit is off.
-    // If buffer is 16-bit, then values are not shifted and error bit is present.
-    // Number of transfers is always the number of samples which is the array
-    // byte length divided by the bytes_per_sample.
-
-    // self->bytes_per_sample == 1
-    uint dma_size = DMA_SIZE_8;
-    bool show_error_bit = false;
-    bool shift_sample_8_bits = true;
-    if (self->bytes_per_sample == 2) {
-        dma_size = DMA_SIZE_16;
-        show_error_bit = true;
-        shift_sample_8_bits = false;
-    }
-
-    // adc_select_input(self->pin->number - ADC_FIRST_PIN_NUMBER);
-    adc_fifo_setup(
-        true,               // Write each completed conversion to the sample FIFO
-        true,               // Enable DMA data request (DREQ)
-        1,                  // DREQ (and IRQ) asserted when at least 1 sample present
-        show_error_bit,     // See the ERR bit
-        shift_sample_8_bits // Shift each sample to 8 bits when pushing to FIFO
-        );
-
     // Divisor of 0 -> full speed. Free-running capture with the divider is
     // equivalent to pressing the ADC_CS_START_ONCE button once per `div + 1`
     // cycles (div not necessarily an integer). Each conversion takes 96
@@ -104,7 +78,8 @@ void common_hal_analogbufio_bufferedin_construct(analogbufio_bufferedin_obj_t *s
     // sample rate determines divisor, not zero.
 
     // sample_rate is forced to be >= 1 in shared-bindings
-    adc_set_clkdiv((float)48000000.0 / (float)self->sample_rate);
+    float clk_div = (float)ADC_CLOCK_INPUT / (float)sample_rate;
+    adc_set_clkdiv(clk_div);
 
     // Set up the DMA to start transferring data as soon as it appears in FIFO
     uint dma_chan = dma_claim_unused_channel(true);
@@ -114,7 +89,6 @@ void common_hal_analogbufio_bufferedin_construct(analogbufio_bufferedin_obj_t *s
     self->cfg = dma_channel_get_default_config(dma_chan);
 
     // Reading from constant address, writing to incrementing byte addresses
-    channel_config_set_transfer_data_size(&(self->cfg), dma_size);
     channel_config_set_read_increment(&(self->cfg), false);
     channel_config_set_write_increment(&(self->cfg), true);
 
@@ -143,14 +117,38 @@ void common_hal_analogbufio_bufferedin_deinit(analogbufio_bufferedin_obj_t *self
     dma_channel_unclaim(self->dma_chan);
 }
 
-void common_hal_analogbufio_bufferedin_read(analogbufio_bufferedin_obj_t *self) {
+uint32_t common_hal_analogbufio_bufferedin_readinto(analogbufio_bufferedin_obj_t *self, uint8_t *buffer, uint32_t len, uint8_t bytes_per_sample) {
+    // RP2040 Implementation Detail
+    // Fills the supplied buffer with ADC values using DMA transfer.
+    // If the buffer is 8-bit, then values are 8-bit shifted and error bit is off.
+    // If buffer is 16-bit, then values are 12-bit and error bit is present. We
+    // stretch the 12-bit value to 16-bits and truncate the number of valid
+    // samples at the first sample with the error bit set.
+    // Number of transfers is always the number of samples which is the array
+    // byte length divided by the bytes_per_sample.
+    uint dma_size = DMA_SIZE_8;
+    bool show_error_bit = false;
+    if (bytes_per_sample == 2) {
+        dma_size = DMA_SIZE_16;
+        show_error_bit = true;
+    }
 
-    uint32_t cdl = self->len / self->bytes_per_sample;
+    adc_fifo_setup(
+        true,                 // Write each completed conversion to the sample FIFO
+        true,                 // Enable DMA data request (DREQ)
+        1,                    // DREQ (and IRQ) asserted when at least 1 sample present
+        show_error_bit,       // See the ERR bit
+        bytes_per_sample == 1 // Shift each sample to 8 bits when pushing to FIFO
+        );
+
+    uint32_t sample_count = len / bytes_per_sample;
+
+    channel_config_set_transfer_data_size(&(self->cfg), dma_size);
 
     dma_channel_configure(self->dma_chan, &(self->cfg),
-        self->buffer,   // dst
+        buffer,   // dst
         &adc_hw->fifo,  // src
-        cdl,            // transfer count
+        sample_count,   // transfer count
         true            // start immediately
         );
 
@@ -159,9 +157,34 @@ void common_hal_analogbufio_bufferedin_read(analogbufio_bufferedin_obj_t *self)
 
     // Once DMA finishes, stop any new conversions from starting, and clean up
     // the FIFO in case the ADC was still mid-conversion.
-    dma_channel_wait_for_finish_blocking(self->dma_chan);
+    uint32_t remaining_transfers = sample_count;
+    while (dma_channel_is_busy(self->dma_chan) &&
+           !mp_hal_is_interrupted()) {
+        RUN_BACKGROUND_TASKS;
+    }
+    remaining_transfers = dma_channel_hw_addr(self->dma_chan)->transfer_count;
 
     //  Clean up
     adc_run(false);
+    // Stopping early so abort.
+    if (dma_channel_is_busy(self->dma_chan)) {
+        dma_channel_abort(self->dma_chan);
+    }
     adc_fifo_drain();
+
+    size_t captured_count = sample_count - remaining_transfers;
+    if (dma_size == DMA_SIZE_16) {
+        uint16_t *buf16 = (uint16_t *)buffer;
+        for (size_t i = 0; i < captured_count; i++) {
+            uint16_t value = buf16[i];
+            // Check the error bit and "truncate" the buffer if there is an error.
+            if ((value & ADC_FIFO_ERR_BITS) != 0) {
+                captured_count = i;
+                break;
+            }
+            // Scale the values to the standard 16 bit range.
+            buf16[i] = (value << 4) | (value >> 8);
+        }
+    }
+    return captured_count;
 }

ports/raspberrypi/common-hal/analogbufio/BufferedIn.h

@@ -41,16 +41,9 @@
 typedef struct {
     mp_obj_base_t base;
     const mcu_pin_obj_t *pin;
-    uint8_t *buffer;
-    uint32_t len;
-    uint8_t bytes_per_sample;
-    bool samples_signed;
-    uint32_t sample_rate;
     uint8_t chan;
     uint dma_chan;
     dma_channel_config cfg;
 } analogbufio_bufferedin_obj_t;
 
-void bufferedin_init(void);
-
 #endif // MICROPY_INCLUDED_RASPBERRYPI_COMMON_HAL_ANALOGBUFIO_BUFFEREDIN_H

shared-bindings/analogbufio/BufferedIn.c

@@ -47,10 +47,10 @@
 //|         import array
 //|
 //|         length = 1000
-//|         mybuffer = array.array("H", 0x0000 for i in range(length))
+//|         mybuffer = array.array("H", [0x0000] * length)
 //|         rate = 500000
-//|         adcbuf = analogbufio.BufferedIn(board.GP26, mybuffer, rate)
-//|         adcbuf.read()
+//|         adcbuf = analogbufio.BufferedIn(board.GP26, sample_rate=rate)
+//|         adcbuf.readinto(mybuffer)
 //|         adcbuf.deinit()
 //|         for i in range(length):
 //|             print(i, mybuffer[i])
@@ -60,64 +60,30 @@
 //|         (TODO) Provide mechanism to read CPU Temperature."""
 //|
 
-//|     def __init__(
-//|         self, pin: microcontroller.Pin, buffer: WriteableBuffer, *, sample_rate: int = 500000
-//|     ) -> None:
-//|         """Create a `BufferedIn` on the given pin. ADC values will be read
-//|            into the given buffer at the supplied sample_rate. Depending on the
-//|            buffer typecode, 'b', 'B', 'h', 'H', samples are 8-bit byte-arrays or
-//|            16-bit half-words and are signed or unsigned.
-//|            The ADC most significant bits of the ADC are kept. (See
-//|            https://docs.circuitpython.org/en/latest/docs/library/array.html)
+//|     def __init__(self, pin: microcontroller.Pin, *, sample_rate: int) -> None:
+//|         """Create a `BufferedIn` on the given pin and given sample rate.
 //|
 //|         :param ~microcontroller.Pin pin: the pin to read from
-//|         :param ~circuitpython_typing.WriteableBuffer buffer: buffer: A buffer for samples
 //|         :param ~int sample_rate: rate: sampling frequency, in samples per second"""
 //|         ...
 STATIC mp_obj_t analogbufio_bufferedin_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *all_args) {
-    enum { ARG_pin, ARG_buffer, ARG_sample_rate };
+    enum { ARG_pin, ARG_sample_rate };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_pin,    MP_ARG_OBJ | MP_ARG_REQUIRED },
-        { MP_QSTR_buffer, MP_ARG_OBJ | MP_ARG_REQUIRED },
-        { MP_QSTR_sample_rate, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 500000} },
+        { MP_QSTR_sample_rate, MP_ARG_KW_ONLY | MP_ARG_REQUIRED | MP_ARG_INT },
     };
     mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)];
     mp_arg_parse_all_kw_array(n_args, n_kw, all_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args);
 
     // Validate Pin
     const mcu_pin_obj_t *pin = validate_obj_is_free_pin(args[ARG_pin].u_obj);
 
-    // Buffer defined and allocated by user
-    mp_buffer_info_t bufinfo;
-    mp_get_buffer_raise(args[ARG_buffer].u_obj, &bufinfo, MP_BUFFER_READ);
-
-    // signed or unsigned, byte per sample
-    bool signed_samples = bufinfo.typecode == 'b' || bufinfo.typecode == 'h';
-    uint8_t bytes_per_sample = 1;
-
-    // Bytes Per Sample
-    if (bufinfo.typecode == 'h' || bufinfo.typecode == 'H') {
-        bytes_per_sample = 2;
-    } else if (bufinfo.typecode != 'b' && bufinfo.typecode != 'B' && bufinfo.typecode != BYTEARRAY_TYPECODE) {
-        mp_raise_ValueError_varg(translate("%q must be a bytearray or array of type 'h', 'H', 'b', or 'B'"), MP_QSTR_buffer);
-    }
-
-    // Validate sample rate here
-    uint32_t sample_rate = (uint32_t)mp_arg_validate_int_range(args[ARG_sample_rate].u_int, 1, 500000, MP_QSTR_sample_rate);
-
     // Create local object
-    analogbufio_bufferedin_obj_t *self = m_new_obj(analogbufio_bufferedin_obj_t);
+    analogbufio_bufferedin_obj_t *self = m_new_obj_with_finaliser(analogbufio_bufferedin_obj_t);
     self->base.type = &analogbufio_bufferedin_type;
 
-    // Call local intereface in ports/common-hal/analogbufio
-    common_hal_analogbufio_bufferedin_construct(self,
-        pin,
-        ((uint8_t *)bufinfo.buf),
-        bufinfo.len,
-        bytes_per_sample,
-        signed_samples,
-        sample_rate
-        );
+    // Call local interface in ports/common-hal/analogbufio
+    common_hal_analogbufio_bufferedin_construct(self, pin, args[ARG_sample_rate].u_int);
 
     return MP_OBJ_FROM_PTR(self);
 }
@@ -153,23 +119,46 @@ STATIC mp_obj_t analogbufio_bufferedin___exit__(size_t n_args, const mp_obj_t *a
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(analogbufio_bufferedin___exit___obj, 4, 4, analogbufio_bufferedin___exit__);
 
-//|     def read(self) -> None:
-//|         """Fills the provided buffer with ADC voltage values."""
+//|     def readinto(self, buffer: WriteableBuffer) -> int:
+//|         """Fills the provided buffer with ADC voltage values.
+//|
+//|         ADC values will be read into the given buffer at the supplied sample_rate.
+//|         Depending on the buffer typecode, 'B', 'H', samples are 8-bit byte-arrays or
+//|         16-bit half-words and are always unsigned.
+//|         The ADC most significant bits of the ADC are kept. (See
+//|         https://docs.circuitpython.org/en/latest/docs/library/array.html)
+//|
+//|         :param ~circuitpython_typing.WriteableBuffer buffer: buffer: A buffer for samples"""
 //|         ...
 //|
-STATIC mp_obj_t analogbufio_bufferedin_obj_read(mp_obj_t self_in) {
+STATIC mp_obj_t analogbufio_bufferedin_obj_readinto(mp_obj_t self_in, mp_obj_t buffer_obj) {
     analogbufio_bufferedin_obj_t *self = MP_OBJ_TO_PTR(self_in);
     check_for_deinit(self);
-    common_hal_analogbufio_bufferedin_read(self);
-    return mp_const_none;
+
+    // Buffer defined and allocated by user
+    mp_buffer_info_t bufinfo;
+    mp_get_buffer_raise(buffer_obj, &bufinfo, MP_BUFFER_READ);
+
+    uint8_t bytes_per_sample = 1;
+
+    // Bytes Per Sample
+    if (bufinfo.typecode == 'H') {
+        bytes_per_sample = 2;
+    } else if (bufinfo.typecode != 'B' && bufinfo.typecode != BYTEARRAY_TYPECODE) {
+        mp_raise_ValueError_varg(translate("%q must be a bytearray or array of type 'H' or 'B'"), MP_QSTR_buffer);
+    }
+
+    mp_uint_t captured = common_hal_analogbufio_bufferedin_readinto(self, bufinfo.buf, bufinfo.len, bytes_per_sample);
+    return MP_OBJ_NEW_SMALL_INT(captured);
 }
-MP_DEFINE_CONST_FUN_OBJ_1(analogbufio_bufferedin_read_obj, analogbufio_bufferedin_obj_read);
+MP_DEFINE_CONST_FUN_OBJ_2(analogbufio_bufferedin_readinto_obj, analogbufio_bufferedin_obj_readinto);
 
 STATIC const mp_rom_map_elem_t analogbufio_bufferedin_locals_dict_table[] = {
+    { MP_ROM_QSTR(MP_QSTR___del__),    MP_ROM_PTR(&analogbufio_bufferedin_deinit_obj) },
     { MP_ROM_QSTR(MP_QSTR_deinit),     MP_ROM_PTR(&analogbufio_bufferedin_deinit_obj) },
     { MP_ROM_QSTR(MP_QSTR___enter__),  MP_ROM_PTR(&default___enter___obj) },
     { MP_ROM_QSTR(MP_QSTR___exit__),   MP_ROM_PTR(&analogbufio_bufferedin___exit___obj) },
-    { MP_ROM_QSTR(MP_QSTR_read),       MP_ROM_PTR(&analogbufio_bufferedin_read_obj)},
+    { MP_ROM_QSTR(MP_QSTR_readinto),       MP_ROM_PTR(&analogbufio_bufferedin_readinto_obj)},
 
 };
 

shared-bindings/analogbufio/BufferedIn.h

@@ -32,9 +32,9 @@
 
 extern const mp_obj_type_t analogbufio_bufferedin_type;
 
-void common_hal_analogbufio_bufferedin_construct(analogbufio_bufferedin_obj_t *self, const mcu_pin_obj_t *pin, uint8_t *buffer, uint32_t len, uint8_t bytes_per_sample, bool samples_signed, uint32_t sample_rate);
+void common_hal_analogbufio_bufferedin_construct(analogbufio_bufferedin_obj_t *self, const mcu_pin_obj_t *pin, uint32_t sample_rate);
 void common_hal_analogbufio_bufferedin_deinit(analogbufio_bufferedin_obj_t *self);
 bool common_hal_analogbufio_bufferedin_deinited(analogbufio_bufferedin_obj_t *self);
-void common_hal_analogbufio_bufferedin_read(analogbufio_bufferedin_obj_t *self);
+uint32_t common_hal_analogbufio_bufferedin_readinto(analogbufio_bufferedin_obj_t *self, uint8_t *buffer, uint32_t len, uint8_t bytes_per_sample);
 
 #endif  // __MICROPY_INCLUDED_SHARED_BINDINGS_ANALOGBUFIO_BUFFEREDIN_H__

shared-bindings/analogbufio/__init__.c

@@ -40,22 +40,6 @@
 //| call :py:meth:`!deinit` or use a context manager. See
 //| :ref:`lifetime-and-contextmanagers` for more info.
 //|
-//| For example::
-//|
-//|   import analogbufio
-//|   import array
-//|   from board import *
-//|
-//|   length = 5000000
-//|   mybuffer = array.array("H", 0x0000 for i in range(length))
-//|   adc_in = analogbufio.BufferedIn(GP26, mybuffer, length)
-//|   analogbufio.read()
-//|   print(*mybuffer)
-//|   adc_in.deinit()
-//|
-//| This example will initialize the the device, read and fill
-//| :py:data:`~analogbufio.BufferedIn` to mybuffer
-//|
 //| TODO: For the essentials of `analogbufio`, see the `CircuitPython Essentials
 //| Learn guide <https://learn.adafruit.com/circuitpython-essentials/circuitpython-analogbufio>`_
 //|