Merge pull request #2252 from dhalbert/cpb-two-i2c

nrf: allocate two I2C on CPB

ports/atmel-samd/common-hal/busio/I2C.c

@@ -116,6 +116,7 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     if (i2c_m_sync_set_baudrate(&self->i2c_desc, 0, frequency / 1000) != ERR_NONE) {
         reset_pin_number(sda->number);
         reset_pin_number(scl->number);
+        common_hal_busio_i2c_deinit(self);
         mp_raise_ValueError(translate("Unsupported baudrate"));
     }
 

ports/nrf/boards/arduino_nano_33_ble/mpconfigboard.mk

@@ -22,3 +22,10 @@ endif
 NRF_DEFINES += -DNRF52840_XXAA -DNRF52840
 
 INTERNAL_FLASH_FILESYSTEM = 1
+
+# Allocate two, not just one I2C peripheral, so that we have both
+# on-board and off-board I2C available.
+# When SPIM3 becomes available we'll be able to have two I2C and two SPI peripherals.
+# We use a CFLAGS define here because there are include order issues
+# if we try to include "mpconfigport.h" into nrfx_config.h .
+CFLAGS += -DCIRCUITPY_NRF_NUM_I2C=2

ports/nrf/boards/circuitplayground_bluefruit/mpconfigboard.mk

@@ -24,3 +24,10 @@ NRF_DEFINES += -DNRF52840_XXAA -DNRF52840
 QSPI_FLASH_FILESYSTEM = 1
 EXTERNAL_FLASH_DEVICE_COUNT = 1
 EXTERNAL_FLASH_DEVICES = "GD25Q16C"
+
+# Allocate two, not just one I2C peripheral for CPB, so that we have both
+# on-board and off-board I2C available.
+# When SPIM3 becomes available we'll be able to have two I2C and two SPI peripherals.
+# We use a CFLAGS define here because there are include order issues
+# if we try to include "mpconfigport.h" into nrfx_config.h .
+CFLAGS += -DCIRCUITPY_NRF_NUM_I2C=2

ports/nrf/common-hal/busio/I2C.c

@@ -28,13 +28,12 @@
  */
 
 #include "shared-bindings/busio/I2C.h"
+#include "shared-bindings/microcontroller/__init__.h"
 #include "py/mperrno.h"
 #include "py/runtime.h"
 #include "supervisor/shared/translate.h"
 
 #include "nrfx_twim.h"
-#include "nrf_gpio.h"
-
 #include "nrfx_spim.h"
 #include "nrf_gpio.h"
 
@@ -107,7 +106,7 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self, const mcu_pin_obj_t *
     for (size_t i = 0 ; i < MP_ARRAY_SIZE(twim_peripherals); i++) {
         if (!twim_peripherals[i].in_use) {
             self->twim_peripheral = &twim_peripherals[i];
-            self->twim_peripheral->in_use = true;
+            // Mark it as in_use later after other validation is finished.
             break;
         }
     }
@@ -116,10 +115,27 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self, const mcu_pin_obj_t *
         mp_raise_ValueError(translate("All I2C peripherals are in use"));
     }
 
+    // Test that the pins are in a high state. (Hopefully indicating they are pulled up.)
+    nrf_gpio_cfg_input(scl->number, NRF_GPIO_PIN_PULLDOWN);
+    nrf_gpio_cfg_input(sda->number, NRF_GPIO_PIN_PULLDOWN);
+
+    common_hal_mcu_delay_us(10);
+
+    nrf_gpio_cfg_input(scl->number, NRF_GPIO_PIN_NOPULL);
+    nrf_gpio_cfg_input(sda->number, NRF_GPIO_PIN_NOPULL);
+
+    // We must pull up within 3us to achieve 400khz.
+    common_hal_mcu_delay_us(3);
+
+    if (!nrf_gpio_pin_read(sda->number) || !nrf_gpio_pin_read(scl->number)) {
+        reset_pin_number(sda->number);
+        reset_pin_number(scl->number);
+        mp_raise_RuntimeError(translate("SDA or SCL needs a pull up"));
+    }
+
     nrfx_twim_config_t config = NRFX_TWIM_DEFAULT_CONFIG;
     config.scl = scl->number;
     config.sda = sda->number;
-
     // change freq. only if it's less than the default 400K
     if (frequency < 100000) {
         config.frequency = NRF_TWIM_FREQ_100K;
@@ -132,6 +148,8 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self, const mcu_pin_obj_t *
     claim_pin(sda);
     claim_pin(scl);
 
+    // About to init. If we fail after this point, common_hal_busio_i2c_deinit() will set in_use to false.
+    self->twim_peripheral->in_use = true;
     nrfx_err_t err = nrfx_twim_init(&self->twim_peripheral->twim, &config, NULL, NULL);
 
     // A soft reset doesn't uninit the driver so we might end up with a invalid state
@@ -152,8 +170,9 @@ bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
 }
 
 void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
-    if (common_hal_busio_i2c_deinited(self))
+    if (common_hal_busio_i2c_deinited(self)) {
         return;
+    }
 
     nrfx_twim_uninit(&self->twim_peripheral->twim);
 

ports/nrf/nrfx_config.h

@@ -5,12 +5,9 @@
 #define NRFX_POWER_ENABLED 1
 #define NRFX_POWER_CONFIG_IRQ_PRIORITY 7
 
-// Turn on nrfx supported workarounds for errata in Rev1/Rev2 of nRF52832
-#ifdef NRF52832_XXAA
-    #define NRFX_SPIS_NRF52_ANOMALY_109_WORKAROUND_ENABLED 1
-#endif
-
-// NOTE: THIS WORKAROUND CAUSES BLE CODE TO CRASH; tested on 2019-03-11.
+// NOTE: THIS WORKAROUND CAUSES BLE CODE TO CRASH.
+// It doesn't work with the SoftDevice.
+// See https://devzone.nordicsemi.com/f/nordic-q-a/33982/sdk-15-software-crash-during-spi-session
 // Turn on nrfx supported workarounds for errata in Rev1 of nRF52840
 #ifdef NRF52840_XXAA
 //    #define NRFX_SPIM3_NRF52840_ANOMALY_198_WORKAROUND_ENABLED 1
@@ -24,11 +21,26 @@
 // so out of the box TWIM0/SPIM0 and TWIM1/SPIM1 cannot be shared
 // between common-hal/busio/I2C.c and SPI.c.
 // We could write an interrupt handler that checks whether it's
-// being used for SPI or I2C, but perhaps two I2C's and 1-2 SPI's are good enough for now.
+// being used for SPI or I2C, but perhaps one I2C and two SPI or two I2C and one SPI
+// are good enough for now.
+
+// CIRCUITPY_NRF_NUM_I2C is 1 or 2 to choose how many I2C (TWIM) peripherals
+// to provide.
+// This can go away once we have SPIM3 working: then we can have two
+// I2C and two SPI.
+#ifndef CIRCUITPY_NRF_NUM_I2C
+#define CIRCUITPY_NRF_NUM_I2C 1
+#endif
+
+#if CIRCUITPY_NRF_NUM_I2C != 1 && CIRCUITPY_NRF_NUM_I2C != 2
+# error CIRCUITPY_NRF_NUM_I2C must be 1 or 2
+#endif
 
 // Enable SPIM1, SPIM2 and SPIM3 (if available)
 // No conflict with TWIM0.
+#if CIRCUITPY_NRF_NUM_I2C == 1
 #define NRFX_SPIM1_ENABLED 1
+#endif
 #define NRFX_SPIM2_ENABLED 1
 // DON'T ENABLE SPIM3 DUE TO ANOMALY WORKAROUND FAILURE (SEE ABOVE).
 // #ifdef NRF52840_XXAA
@@ -45,10 +57,13 @@
 // QSPI
 #define NRFX_QSPI_ENABLED                          1
 
-// TWI aka. I2C; enable a single bus: TWIM0 (no conflict with SPIM1 and SPIM2)
+// TWI aka. I2C; always enable TWIM0 (no conflict with SPIM1 and SPIM2)
 #define NRFX_TWIM_ENABLED 1
 #define NRFX_TWIM0_ENABLED 1
-//#define NRFX_TWIM1_ENABLED 1
+
+#if CIRCUITPY_NRF_NUM_I2C == 2
+#define NRFX_TWIM1_ENABLED 1
+#endif
 
 #define NRFX_TWIM_DEFAULT_CONFIG_IRQ_PRIORITY 7
 #define NRFX_TWIM_DEFAULT_CONFIG_FREQUENCY NRF_TWIM_FREQ_400K

shared-bindings/busio/I2C.c

@@ -60,6 +60,10 @@
 //|   :param int frequency: The clock frequency in Hertz
 //|   :param int timeout: The maximum clock stretching timeut - (used only for bitbangio.I2C; ignored for busio.I2C)
 //|
+//|   .. note:: On the nRF52840, only one I2C object may be created,
+//|      except on the Circuit Playground Bluefruit, which allows two,
+//|      one for the onboard accelerometer, and one for offboard use.
+//|
 STATIC mp_obj_t busio_i2c_make_new(const mp_obj_type_t *type, size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     busio_i2c_obj_t *self = m_new_obj(busio_i2c_obj_t);
     self->base.type = &busio_i2c_type;

shared-bindings/busio/SPI.c

@@ -154,11 +154,12 @@ STATIC void check_for_deinit(busio_spi_obj_t *self) {
 //|      within spec for the SAMD21.
 //|
 //|   .. note:: On the nRF52840, these baudrates are available: 125kHz, 250kHz, 1MHz, 2MHz, 4MHz,
-//|      and 8MHz. 16MHz and 32MHz are also available, but only on the first
-//|      `busio.SPI` object you create. Two more ``busio.SPI`` objects can be created, but they are restricted
-//|      to 8MHz maximum. This is a hardware restriction: there is only one high-speed SPI peripheral.
+//|      and 8MHz.
 //|      If you pick a a baudrate other than one of these, the nearest lower
 //|      baudrate will be chosen, with a minimum of 125kHz.
+//|      Two SPI objects may be created, except on the Circuit Playground Bluefruit,
+//|      which allows only one (to allow for an additional I2C object).
+//|
 STATIC mp_obj_t busio_spi_configure(size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     enum { ARG_baudrate, ARG_polarity, ARG_phase, ARG_bits };
     static const mp_arg_t allowed_args[] = {