Merge pull request #2463 from hierophect/stm32-i2c-rework

STM32: I2C fix & general busio cleanup

4 files changed, 353 insertions, 252 deletions

diff --git a/ports/stm32f4/common-hal/busio/I2C.c b/ports/stm32f4/common-hal/busio/I2C.c
index d2292845e..1437e5e78 100644
--- a/ports/stm32f4/common-hal/busio/I2C.c
+++ b/ports/stm32f4/common-hal/busio/I2C.c
@@ -35,51 +35,46 @@
 #include "supervisor/shared/translate.h"
 #include "common-hal/microcontroller/Pin.h"
 
-STATIC bool reserved_i2c[3];
-STATIC bool never_reset[3];
+//arrays use 0 based numbering: I2C1 is stored at index 0
+#define MAX_I2C 3
+STATIC bool reserved_i2c[MAX_I2C];
+STATIC bool never_reset_i2c[MAX_I2C];
 
-void i2c_reset(void) {
-    //Note: I2Cs are also forcibly reset in construct, due to silicon error
-    #ifdef I2C1
-        reserved_i2c[0] = false;
-        __HAL_RCC_I2C1_CLK_DISABLE(); 
-    #endif
-    #ifdef I2C2
-        reserved_i2c[1] = false;
-        __HAL_RCC_I2C2_CLK_DISABLE(); 
-    #endif
-    #ifdef I2C3
-        reserved_i2c[2] = false;
-        __HAL_RCC_I2C3_CLK_DISABLE(); 
-    #endif
-}
-
-void common_hal_busio_i2c_never_reset(busio_i2c_obj_t *self) {
-    for (size_t i = 0 ; i < MP_ARRAY_SIZE(mcu_i2c_banks); i++) {
-        if (self->handle.Instance == mcu_i2c_banks[i]) {
-            never_reset[i] = true;
+#define ALL_CLOCKS 0xFF
+STATIC void i2c_clock_enable(uint8_t mask);
+STATIC void i2c_clock_disable(uint8_t mask);
 
-            never_reset_pin_number(self->scl->pin->port, self->scl->pin->number);
-            never_reset_pin_number(self->sda->pin->port, self->scl->pin->number);
-            break;
+void i2c_reset(void) {
+    uint16_t never_reset_mask = 0x00;
+    for(int i = 0; i < MAX_I2C; i++) {
+        if (!never_reset_i2c[i]) {
+            reserved_i2c[i] = false;
+        } else {
+            never_reset_mask |= 1 << i;
         }
     }
+    i2c_clock_disable(ALL_CLOCKS & ~(never_reset_mask));
 }
 
-
 void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
         const mcu_pin_obj_t* scl, const mcu_pin_obj_t* sda, uint32_t frequency, uint32_t timeout) {
 
     //match pins to I2C objects
     I2C_TypeDef * I2Cx;
+    uint8_t sda_len = MP_ARRAY_SIZE(mcu_i2c_sda_list);
+    uint8_t scl_len = MP_ARRAY_SIZE(mcu_i2c_scl_list);
+    bool i2c_taken = false;
 
-    uint8_t sda_len = sizeof(mcu_i2c_sda_list)/sizeof(*mcu_i2c_sda_list);
-    uint8_t scl_len = sizeof(mcu_i2c_scl_list)/sizeof(*mcu_i2c_scl_list);
-    for(uint i=0; i<sda_len;i++) {
+    for (uint i = 0; i < sda_len; i++) {
         if (mcu_i2c_sda_list[i].pin == sda) {
-            for(uint j=0; j<scl_len;j++) {
+            for (uint j = 0; j < scl_len; j++) {
                 if ((mcu_i2c_scl_list[j].pin == scl)
                     && (mcu_i2c_scl_list[j].i2c_index == mcu_i2c_sda_list[i].i2c_index)) {
+                    //keep looking if the I2C is taken, could be another SCL that works
+                    if (reserved_i2c[mcu_i2c_scl_list[i].i2c_index - 1]) {
+                        i2c_taken = true;
+                        continue;
+                    }
                     self->scl = &mcu_i2c_scl_list[j];
                     self->sda = &mcu_i2c_sda_list[i];
                     break;
@@ -89,14 +84,14 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     }
 
     //handle typedef selection, errors
-    if(self->sda!=NULL && self->scl!=NULL ) {
-        I2Cx = mcu_i2c_banks[self->sda->i2c_index-1];
+    if (self->sda != NULL && self->scl != NULL ) {
+        I2Cx = mcu_i2c_banks[self->sda->i2c_index - 1];
     } else {
-        mp_raise_RuntimeError(translate("Invalid I2C pin selection"));
-    }
-
-    if(reserved_i2c[self->sda->i2c_index-1]) {
-        mp_raise_RuntimeError(translate("Hardware busy, try alternative pins"));
+        if (i2c_taken) {
+            mp_raise_ValueError(translate("Hardware busy, try alternative pins"));
+        } else {
+            mp_raise_ValueError(translate("Invalid I2C pin selection"));
+        }
     }
 
     //Start GPIO for each pin
@@ -115,44 +110,9 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     GPIO_InitStruct.Alternate = self->scl->altfn_index; 
     HAL_GPIO_Init(pin_port(scl->port), &GPIO_InitStruct);
 
-    //Fix for HAL error caused by soft reboot GPIO init SDA pin voltage drop. See Eratta. 
-    //Must be in this exact spot or I2C will get stuck in infinite loop. 
-    //TODO: See git issue #2172
-    #ifdef I2C1
-    __HAL_RCC_I2C1_FORCE_RESET();
-    HAL_Delay(2);
-    __HAL_RCC_I2C1_RELEASE_RESET();
-    #endif
-    #ifdef I2C2
-    __HAL_RCC_I2C2_FORCE_RESET();
-    HAL_Delay(2);
-    __HAL_RCC_I2C2_RELEASE_RESET();
-    #endif
-    #ifdef I2C3
-    __HAL_RCC_I2C3_FORCE_RESET();
-    HAL_Delay(2);
-    __HAL_RCC_I2C3_RELEASE_RESET();
-    #endif
-
-    //Keep separate so above hack can be cleanly replaced
-    #ifdef I2C1
-    if(I2Cx==I2C1) { 
-        reserved_i2c[0] = true;
-        __HAL_RCC_I2C1_CLK_ENABLE();
-    } 
-    #endif
-    #ifdef I2C2
-    if(I2Cx==I2C2) {
-        reserved_i2c[1] = true;
-        __HAL_RCC_I2C2_CLK_ENABLE(); 
-    }
-    #endif
-    #ifdef I2C3
-    if(I2Cx==I2C3) {
-        reserved_i2c[2] = true;
-        __HAL_RCC_I2C3_CLK_ENABLE(); 
-    }
-    #endif
+    //Note: due to I2C soft reboot issue, do not relocate clock init.
+    i2c_clock_enable(1 << (self->sda->i2c_index - 1));
+    reserved_i2c[self->sda->i2c_index - 1] = true;
 
     self->handle.Instance = I2Cx;
     self->handle.Init.ClockSpeed = 100000;
@@ -163,13 +123,26 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     self->handle.Init.OwnAddress2 = 0;
     self->handle.Init.GeneralCallMode = I2C_GENERALCALL_DISABLE;
     self->handle.Init.NoStretchMode = I2C_NOSTRETCH_DISABLE;
-    if(HAL_I2C_Init(&(self->handle)) != HAL_OK) {
+    self->handle.State = HAL_I2C_STATE_RESET;
+    if (HAL_I2C_Init(&(self->handle)) != HAL_OK) {
         mp_raise_RuntimeError(translate("I2C Init Error"));
     }
     claim_pin(sda);
     claim_pin(scl);
 }
 
+void common_hal_busio_i2c_never_reset(busio_i2c_obj_t *self) {
+    for (size_t i = 0; i < MP_ARRAY_SIZE(mcu_i2c_banks); i++) {
+        if (self->handle.Instance == mcu_i2c_banks[i]) {
+            never_reset_i2c[i] = true;
+
+            never_reset_pin_number(self->scl->pin->port, self->scl->pin->number);
+            never_reset_pin_number(self->sda->pin->port, self->sda->pin->number);
+            break;
+        }
+    }
+}
+
 bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
     return self->sda->pin == mp_const_none;
 }
@@ -178,27 +151,11 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     if (common_hal_busio_i2c_deinited(self)) {
         return;
     }
-    #ifdef I2C1
-    if(self->handle.Instance==I2C1) {
-        never_reset[0] = false;
-        reserved_i2c[0] = false;
-        __HAL_RCC_I2C1_CLK_DISABLE(); 
-    }
-    #endif
-    #ifdef I2C2
-    if(self->handle.Instance==I2C2) {
-        never_reset[1] = false;
-        reserved_i2c[1] = false;
-        __HAL_RCC_I2C2_CLK_DISABLE(); 
-    }
-    #endif
-    #ifdef I2C3
-    if(self->handle.Instance==I2C3) {
-        never_reset[2] = false;
-        reserved_i2c[2] = false;
-        __HAL_RCC_I2C3_CLK_DISABLE(); 
-    }
-    #endif
+
+    i2c_clock_disable(1 << (self->sda->i2c_index - 1));
+    reserved_i2c[self->sda->i2c_index - 1] = false;
+    never_reset_i2c[self->sda->i2c_index - 1] = false;
+
     reset_pin_number(self->sda->pin->port,self->sda->pin->number);
     reset_pin_number(self->scl->pin->port,self->scl->pin->number);
     self->sda = mp_const_none;
@@ -206,7 +163,7 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 }
 
 bool common_hal_busio_i2c_probe(busio_i2c_obj_t *self, uint8_t addr) {
-    return HAL_I2C_IsDeviceReady(&(self->handle), (uint16_t)(addr<<1),2,2) == HAL_OK;
+    return HAL_I2C_IsDeviceReady(&(self->handle), (uint16_t)(addr << 1), 2, 2) == HAL_OK;
 }
 
 bool common_hal_busio_i2c_try_lock(busio_i2c_obj_t *self) {
@@ -238,11 +195,56 @@ void common_hal_busio_i2c_unlock(busio_i2c_obj_t *self) {
 
 uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr,
                                    const uint8_t *data, size_t len, bool transmit_stop_bit) {
-    HAL_StatusTypeDef result = HAL_I2C_Master_Transmit(&(self->handle), (uint16_t)(addr<<1), (uint8_t *)data, (uint16_t)len, 500);
+    HAL_StatusTypeDef result = HAL_I2C_Master_Transmit(&(self->handle), (uint16_t)(addr << 1), 
+                                                        (uint8_t *)data, (uint16_t)len, 500);
     return result == HAL_OK ? 0 : MP_EIO;
 }
 
 uint8_t common_hal_busio_i2c_read(busio_i2c_obj_t *self, uint16_t addr,
         uint8_t *data, size_t len) {
-    return HAL_I2C_Master_Receive(&(self->handle), (uint16_t)(addr<<1), data, (uint16_t)len, 500) == HAL_OK ? 0 : MP_EIO;
+    return HAL_I2C_Master_Receive(&(self->handle), (uint16_t)(addr<<1), data, (uint16_t)len, 500) 
+                                    == HAL_OK ? 0 : MP_EIO;
+}
+
+STATIC void i2c_clock_enable(uint8_t mask) {
+    //Note: hard reset required due to soft reboot issue. 
+    #ifdef I2C1
+    if (mask & (1 << 0)) {
+        __HAL_RCC_I2C1_CLK_ENABLE();
+        __HAL_RCC_I2C1_FORCE_RESET();
+        __HAL_RCC_I2C1_RELEASE_RESET();
+    }
+    #endif
+    #ifdef I2C2
+    if (mask & (1 << 1)) {
+        __HAL_RCC_I2C2_CLK_ENABLE();
+        __HAL_RCC_I2C2_FORCE_RESET();
+        __HAL_RCC_I2C2_RELEASE_RESET();
+    }
+    #endif
+    #ifdef I2C3
+    if (mask & (1 << 2)) {
+        __HAL_RCC_I2C3_CLK_ENABLE();
+        __HAL_RCC_I2C3_FORCE_RESET();
+        __HAL_RCC_I2C3_RELEASE_RESET();
+    }
+    #endif
+}
+
+STATIC void i2c_clock_disable(uint8_t mask) {
+    #ifdef I2C1
+    if (mask & (1 << 0)) {
+        __HAL_RCC_I2C1_CLK_DISABLE();
+    }
+    #endif
+    #ifdef I2C2
+    if (mask & (1 << 1)) {
+        __HAL_RCC_I2C2_CLK_DISABLE();
+    }
+    #endif
+    #ifdef I2C3
+    if (mask & (1 << 2)) {
+        __HAL_RCC_I2C3_CLK_DISABLE();
+    }
+    #endif
 }
diff --git a/ports/stm32f4/common-hal/busio/SPI.c b/ports/stm32f4/common-hal/busio/SPI.c
index deacedb50..afc6cfcbd 100644
--- a/ports/stm32f4/common-hal/busio/SPI.c
+++ b/ports/stm32f4/common-hal/busio/SPI.c
@@ -39,13 +39,12 @@
 // Note that any bugs introduced in this file can cause crashes at startup
 // for chips using external SPI flash.
 
-#define MAX_SPI 6 //TODO; replace this as part of periph cleanup
-#define ALL_CLOCKS 0xFF
-
 //arrays use 0 based numbering: SPI1 is stored at index 0
+#define MAX_SPI 6
 STATIC bool reserved_spi[MAX_SPI];
 STATIC bool never_reset_spi[MAX_SPI];
 
+#define ALL_CLOCKS 0xFF
 STATIC void spi_clock_enable(uint8_t mask);
 STATIC void spi_clock_disable(uint8_t mask);
 
@@ -60,13 +59,39 @@ STATIC uint32_t get_busclock(SPI_TypeDef * instance) {
     return HAL_RCC_GetPCLK2Freq();
 }
 
+STATIC uint32_t stm32_baud_to_spi_div(uint32_t baudrate, uint16_t * prescaler, uint32_t busclock) {
+    static const uint32_t baud_map[8][2] = {
+        {2,SPI_BAUDRATEPRESCALER_2},
+        {4,SPI_BAUDRATEPRESCALER_4},
+        {8,SPI_BAUDRATEPRESCALER_8},
+        {16,SPI_BAUDRATEPRESCALER_16},
+        {32,SPI_BAUDRATEPRESCALER_32},
+        {64,SPI_BAUDRATEPRESCALER_64},
+        {128,SPI_BAUDRATEPRESCALER_128},
+        {256,SPI_BAUDRATEPRESCALER_256}
+    };
+    size_t i = 0;
+    uint16_t divisor;
+    do {
+        divisor = baud_map[i][0];
+        if (baudrate >= (busclock/divisor)) {
+            *prescaler = divisor;
+            return baud_map[i][1];
+        }
+        i++;
+    } while (divisor != 256);
+    //only gets here if requested baud is lower than minimum
+    *prescaler = 256;
+    return SPI_BAUDRATEPRESCALER_256;
+}
+
 void spi_reset(void) {
     uint16_t never_reset_mask = 0x00;
-    for(int i=0;i<MAX_SPI;i++) {
+    for (int i = 0; i < MAX_SPI; i++) {
         if (!never_reset_spi[i]) {
-            reserved_spi[i] = 0x00;
+            reserved_spi[i] = false;
         } else {
-            never_reset_mask |= 1<<i;
+            never_reset_mask |= 1 << i;
         }
     }
     spi_clock_disable(ALL_CLOCKS & ~(never_reset_mask));
@@ -79,26 +104,26 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
     //match pins to SPI objects
     SPI_TypeDef * SPIx;
 
-    uint8_t sck_len = sizeof(mcu_spi_sck_list)/sizeof(*mcu_spi_sck_list);
-    uint8_t mosi_len = sizeof(mcu_spi_mosi_list)/sizeof(*mcu_spi_mosi_list);
-    uint8_t miso_len = sizeof(mcu_spi_miso_list)/sizeof(*mcu_spi_miso_list);
+    uint8_t sck_len = MP_ARRAY_SIZE(mcu_spi_sck_list);
+    uint8_t mosi_len = MP_ARRAY_SIZE(mcu_spi_mosi_list);
+    uint8_t miso_len = MP_ARRAY_SIZE(mcu_spi_miso_list);
     bool spi_taken = false;
 
     //SCK is not optional. MOSI and MISO are
-    for (uint i=0; i<sck_len;i++) {
+    for (uint i = 0; i < sck_len; i++) {
         if (mcu_spi_sck_list[i].pin == sck) {
             //if both MOSI and MISO exist, loop search normally
             if ((mosi != mp_const_none) && (miso != mp_const_none)) {
                 //MOSI
-                for (uint j=0; j<mosi_len;j++) {
+                for (uint j = 0; j < mosi_len; j++) {
                     if (mcu_spi_mosi_list[j].pin == mosi) {
                         //MISO
-                        for (uint k=0; k<miso_len;k++) {
+                        for (uint k = 0; k < miso_len; k++) {
                             if ((mcu_spi_miso_list[k].pin == miso) //everything needs the same index
                                 && (mcu_spi_sck_list[i].spi_index == mcu_spi_mosi_list[j].spi_index)
                                 && (mcu_spi_sck_list[i].spi_index == mcu_spi_miso_list[k].spi_index)) {
                                 //keep looking if the SPI is taken, edge case
-                                if (reserved_spi[mcu_spi_sck_list[i].spi_index-1]) {
+                                if (reserved_spi[mcu_spi_sck_list[i].spi_index - 1]) {
                                     spi_taken = true;
                                     continue;
                                 }
@@ -113,11 +138,11 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
                 }
             // if just MISO, reduce search
             } else if (miso != mp_const_none) {
-                for (uint j=0; j<miso_len;j++) {
+                for (uint j = 0; j < miso_len; j++) {
                     if ((mcu_spi_miso_list[j].pin == miso) //only SCK and MISO need the same index
                         && (mcu_spi_sck_list[i].spi_index == mcu_spi_miso_list[j].spi_index)) {
                         //keep looking if the SPI is taken, edge case
-                        if (reserved_spi[mcu_spi_sck_list[i].spi_index-1]) {
+                        if (reserved_spi[mcu_spi_sck_list[i].spi_index - 1]) {
                             spi_taken = true;
                             continue;
                         }
@@ -130,11 +155,11 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
                 }  
             // if just MOSI, reduce search
             } else if (mosi != mp_const_none) {
-                for (uint j=0; j<mosi_len;j++) {
+                for (uint j = 0; j < mosi_len; j++) {
                     if ((mcu_spi_mosi_list[j].pin == mosi) //only SCK and MOSI need the same index
                         && (mcu_spi_sck_list[i].spi_index == mcu_spi_mosi_list[j].spi_index)) {
                         //keep looking if the SPI is taken, edge case
-                        if (reserved_spi[mcu_spi_sck_list[i].spi_index-1]) {
+                        if (reserved_spi[mcu_spi_sck_list[i].spi_index - 1]) {
                             spi_taken = true;
                             continue;
                         }
@@ -153,10 +178,10 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
     }
 
     //handle typedef selection, errors
-    if ( (self->sck!=NULL && self->mosi!=NULL && self->miso != NULL) ||
-        (self->sck!=NULL && self->mosi!=NULL && miso == mp_const_none) ||
-        (self->sck!=NULL && self->miso!=NULL && mosi == mp_const_none)) {
-        SPIx = mcu_spi_banks[self->sck->spi_index-1];
+    if ( (self->sck != NULL && self->mosi != NULL && self->miso != NULL) ||
+        (self->sck != NULL && self->mosi != NULL && miso == mp_const_none) ||
+        (self->sck != NULL && self->miso != NULL && mosi == mp_const_none)) {
+        SPIx = mcu_spi_banks[self->sck->spi_index - 1];
     } else {
         if (spi_taken) {
             mp_raise_ValueError(translate("Hardware busy, try alternative pins"));
@@ -192,7 +217,7 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
         HAL_GPIO_Init(pin_port(miso->port), &GPIO_InitStruct);
     }
 
-    spi_clock_enable(1<<(self->sck->spi_index - 1));
+    spi_clock_enable(1 << (self->sck->spi_index - 1));
     reserved_spi[self->sck->spi_index - 1] = true;
     
     self->handle.Instance = SPIx; 
@@ -212,7 +237,7 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
     {
         mp_raise_ValueError(translate("SPI Init Error"));
     }
-    self->baudrate = (get_busclock(SPIx)/16);
+    self->baudrate = (get_busclock(SPIx) / 16);
     self->prescaler = 16;
     self->polarity = 0;
     self->phase = 0;
@@ -228,7 +253,7 @@ void common_hal_busio_spi_construct(busio_spi_obj_t *self,
 }
 
 void common_hal_busio_spi_never_reset(busio_spi_obj_t *self) {
-    for (size_t i = 0 ; i < MP_ARRAY_SIZE(mcu_spi_banks); i++) {
+    for (size_t i = 0; i < MP_ARRAY_SIZE(mcu_spi_banks); i++) {
         if (mcu_spi_banks[i] == self->handle.Instance) {
             never_reset_spi[i] = true;
             never_reset_pin_number(self->sck->pin->port, self->sck->pin->number);
@@ -248,6 +273,9 @@ bool common_hal_busio_spi_deinited(busio_spi_obj_t *self) {
 }
 
 void common_hal_busio_spi_deinit(busio_spi_obj_t *self) {
+    if (common_hal_busio_spi_deinited(self)) {
+        return;
+    }
     spi_clock_disable(1<<(self->sck->spi_index - 1));
     reserved_spi[self->sck->spi_index - 1] = false;
     never_reset_spi[self->sck->spi_index - 1] = false;
@@ -264,32 +292,6 @@ void common_hal_busio_spi_deinit(busio_spi_obj_t *self) {
     self->miso = mp_const_none;
 }
 
-STATIC uint32_t stm32_baud_to_spi_div(uint32_t baudrate, uint16_t * prescaler, uint32_t busclock) {
-    static const uint32_t baud_map[8][2] = {
-        {2,SPI_BAUDRATEPRESCALER_2},
-        {4,SPI_BAUDRATEPRESCALER_4},
-        {8,SPI_BAUDRATEPRESCALER_8},
-        {16,SPI_BAUDRATEPRESCALER_16},
-        {32,SPI_BAUDRATEPRESCALER_32},
-        {64,SPI_BAUDRATEPRESCALER_64},
-        {128,SPI_BAUDRATEPRESCALER_128},
-        {256,SPI_BAUDRATEPRESCALER_256}
-    };
-    size_t i = 0;
-    uint16_t divisor;
-    do {
-        divisor = baud_map[i][0];
-        if (baudrate >= (busclock/divisor)) {
-            *prescaler = divisor;
-            return baud_map[i][1];
-        }
-        i++;
-    } while (divisor != 256);
-    //only gets here if requested baud is lower than minimum
-    *prescaler = 256;
-    return SPI_BAUDRATEPRESCALER_256;
-}
-
 bool common_hal_busio_spi_configure(busio_spi_obj_t *self,
         uint32_t baudrate, uint8_t polarity, uint8_t phase, uint8_t bits) {
     //This resets the SPI, so check before updating it redundantly
@@ -391,42 +393,66 @@ uint8_t common_hal_busio_spi_get_polarity(busio_spi_obj_t* self) {
 
 STATIC void spi_clock_enable(uint8_t mask) {
     #ifdef SPI1
-    if (mask & 1<<0) __HAL_RCC_SPI1_CLK_ENABLE();
+    if (mask & (1 << 0)) {
+        __HAL_RCC_SPI1_CLK_ENABLE();
+    }
     #endif
     #ifdef SPI2
-    if (mask & 1<<1) __HAL_RCC_SPI2_CLK_ENABLE();
+    if (mask & (1 << 1)) {
+        __HAL_RCC_SPI2_CLK_ENABLE();
+    }
     #endif
     #ifdef SPI3
-    if (mask & 1<<2) __HAL_RCC_SPI3_CLK_ENABLE();
+    if (mask & (1 << 2)) {
+        __HAL_RCC_SPI3_CLK_ENABLE();
+    }
     #endif
     #ifdef SPI4
-    if (mask & 1<<3) __HAL_RCC_SPI4_CLK_ENABLE();
+    if (mask & (1 << 3)) {
+        __HAL_RCC_SPI4_CLK_ENABLE();
+    }
     #endif
     #ifdef SPI5
-    if (mask & 1<<4) __HAL_RCC_SPI5_CLK_ENABLE();
+    if (mask & (1 << 4)) {
+        __HAL_RCC_SPI5_CLK_ENABLE();
+    }
     #endif
     #ifdef SPI6
-    if (mask & 1<<5) __HAL_RCC_SPI6_CLK_ENABLE();
+    if (mask & (1 << 5)) {
+        __HAL_RCC_SPI6_CLK_ENABLE();
+    }
     #endif
 }
 
 STATIC void spi_clock_disable(uint8_t mask) {
     #ifdef SPI1
-    if (mask & 1<<0) __HAL_RCC_SPI1_CLK_DISABLE();
+    if (mask & (1 << 0)) {
+        __HAL_RCC_SPI1_CLK_DISABLE();
+    }
     #endif
     #ifdef SPI2
-    if (mask & 1<<1) __HAL_RCC_SPI2_CLK_DISABLE();
+    if (mask & (1 << 1)) {
+        __HAL_RCC_SPI2_CLK_DISABLE();
+    }
     #endif
     #ifdef SPI3
-    if (mask & 1<<2) __HAL_RCC_SPI3_CLK_DISABLE();
+    if (mask & (1 << 2)) {
+        __HAL_RCC_SPI3_CLK_DISABLE();
+    }
     #endif
     #ifdef SPI4
-    if (mask & 1<<3) __HAL_RCC_SPI4_CLK_DISABLE();
+    if (mask & (1 << 3)) {
+        __HAL_RCC_SPI4_CLK_DISABLE();
+    }
     #endif
     #ifdef SPI5
-    if (mask & 1<<4) __HAL_RCC_SPI5_CLK_DISABLE();
+    if (mask & (1 << 4)) {
+        __HAL_RCC_SPI5_CLK_DISABLE();
+    }
     #endif
     #ifdef SPI6
-    if (mask & 1<<5) __HAL_RCC_SPI6_CLK_DISABLE();
+    if (mask & (1 << 5)) {
+        __HAL_RCC_SPI6_CLK_DISABLE();
+    }
     #endif
 }
diff --git a/ports/stm32f4/common-hal/busio/UART.c b/ports/stm32f4/common-hal/busio/UART.c
index aa792dae1..bfa541c17 100644
--- a/ports/stm32f4/common-hal/busio/UART.c
+++ b/ports/stm32f4/common-hal/busio/UART.c
@@ -40,6 +40,7 @@
 
 #define ALL_UARTS 0xFFFF
 
+//arrays use 0 based numbering: UART1 is stored at index 0
 STATIC bool reserved_uart[MAX_UART];
 int errflag; //Used to restart read halts
 
@@ -47,14 +48,6 @@ STATIC void uart_clock_enable(uint16_t mask);
 STATIC void uart_clock_disable(uint16_t mask);
 STATIC void uart_assign_irq(busio_uart_obj_t* self, USART_TypeDef* USARTx);
 
-void uart_reset(void) {
-    for (uint8_t i = 0; i < MAX_UART; i++) {
-        reserved_uart[i] = false;
-        MP_STATE_PORT(cpy_uart_obj_all)[i] = NULL;
-    }
-    uart_clock_disable(ALL_UARTS);
-}
-
 STATIC USART_TypeDef * assign_uart_or_throw(busio_uart_obj_t* self, bool pin_eval,
                                             int uart_index, bool uart_taken) {
     if (pin_eval) {
@@ -70,6 +63,14 @@ STATIC USART_TypeDef * assign_uart_or_throw(busio_uart_obj_t* self, bool pin_eva
     }
 }
 
+void uart_reset(void) {
+    for (uint8_t i = 0; i < MAX_UART; i++) {
+        reserved_uart[i] = false;
+        MP_STATE_PORT(cpy_uart_obj_all)[i] = NULL;
+    }
+    uart_clock_disable(ALL_UARTS);
+}
+
 void common_hal_busio_uart_construct(busio_uart_obj_t* self,
         const mcu_pin_obj_t* tx, const mcu_pin_obj_t* rx, uint32_t baudrate,
         uint8_t bits, uart_parity_t parity, uint8_t stop, mp_float_t timeout,
@@ -78,8 +79,8 @@ void common_hal_busio_uart_construct(busio_uart_obj_t* self,
     //match pins to UART objects
     USART_TypeDef * USARTx;
 
-    uint8_t tx_len = sizeof(mcu_uart_tx_list)/sizeof(*mcu_uart_tx_list);
-    uint8_t rx_len = sizeof(mcu_uart_rx_list)/sizeof(*mcu_uart_rx_list);
+    uint8_t tx_len = MP_ARRAY_SIZE(mcu_uart_tx_list);
+    uint8_t rx_len = MP_ARRAY_SIZE(mcu_uart_rx_list);
     bool uart_taken = false;
     uint8_t uart_index = 0; //origin 0 corrected
 
@@ -567,33 +568,53 @@ STATIC void uart_clock_disable(uint16_t mask) {
 
 STATIC void uart_assign_irq(busio_uart_obj_t *self, USART_TypeDef * USARTx) {
     #ifdef USART1
-    if (USARTx == USART1) self->irq = USART1_IRQn;
+    if (USARTx == USART1) {
+        self->irq = USART1_IRQn;
+    }
     #endif
     #ifdef USART2
-    if (USARTx == USART2) self->irq = USART2_IRQn;
+    if (USARTx == USART2) {
+        self->irq = USART2_IRQn;
+    }
     #endif
     #ifdef USART3
-    if (USARTx == USART3) self->irq = USART3_IRQn;
+    if (USARTx == USART3) {
+        self->irq = USART3_IRQn;
+    }
     #endif
     #ifdef UART4
-    if (USARTx == UART4) self->irq = UART4_IRQn;
+    if (USARTx == UART4) {
+        self->irq = UART4_IRQn;
+    }
     #endif
     #ifdef UART5
-    if (USARTx == UART5) self->irq = UART5_IRQn;
+    if (USARTx == UART5) {
+        self->irq = UART5_IRQn;
+    }
     #endif
     #ifdef USART6
-    if (USARTx == USART6) self->irq = USART6_IRQn;
+    if (USARTx == USART6) {
+        self->irq = USART6_IRQn;
+    }
     #endif
     #ifdef UART7
-    if (USARTx == UART7) self->irq = UART7_IRQn;
+    if (USARTx == UART7) {
+        self->irq = UART7_IRQn;
+    }
     #endif
     #ifdef UART8
-    if (USARTx == UART8) self->irq = UART8_IRQn;
+    if (USARTx == UART8) {
+        self->irq = UART8_IRQn;
+    }
     #endif
     #ifdef UART9
-    if (USARTx == UART9) self->irq = UART9_IRQn;
+    if (USARTx == UART9) {
+        self->irq = UART9_IRQn;
+    }
     #endif
     #ifdef UART10
-    if (USARTx == UART10) self->irq = UART10_IRQn;
+    if (USARTx == UART10) {
+        self->irq = UART10_IRQn;
+    }
     #endif
 }
diff --git a/ports/stm32f4/common-hal/pulseio/PWMOut.c b/ports/stm32f4/common-hal/pulseio/PWMOut.c
index 16b1b5b89..a750314e1 100644
--- a/ports/stm32f4/common-hal/pulseio/PWMOut.c
+++ b/ports/stm32f4/common-hal/pulseio/PWMOut.c
@@ -69,15 +69,15 @@ STATIC uint32_t timer_get_source_freq(uint32_t tim_id) {
 
 STATIC uint32_t timer_get_internal_duty(uint16_t duty, uint32_t period) {
     //duty cycle is duty/0xFFFF fraction x (number of pulses per period)
-    return (duty*period)/((1<<16)-1);
+    return (duty*period) / ((1 << 16) - 1);
 }
 
 STATIC void timer_get_optimal_divisors(uint32_t*period, uint32_t*prescaler, 
                                        uint32_t frequency, uint32_t source_freq) {
     //Find the largest possible period supported by this frequency
-    for (int i=0; i<(1 << 16);i++) {
-        *period = source_freq/(i*frequency);
-        if (*period < (1 << 16) && *period>=2) {
+    for (int i = 0; i < (1 << 16); i++) {
+        *period = source_freq / (i * frequency);
+        if (*period < (1 << 16) && *period >= 2) {
             *prescaler = i;
             break;
         }
@@ -89,59 +89,40 @@ STATIC void timer_get_optimal_divisors(uint32_t*period, uint32_t*prescaler,
 
 void pwmout_reset(void) {
     uint16_t never_reset_mask = 0x00;
-    for(int i=0;i<TIM_BANK_ARRAY_LEN;i++) {
+    for (int i = 0; i < TIM_BANK_ARRAY_LEN; i++) {
         if (!never_reset_tim[i]) {
             reserved_tim[i] = 0x00;
             tim_frequencies[i] = 0x00;
         } else {
-            never_reset_mask |= 1<<i;
+            never_reset_mask |= 1 << i;
         }
     }
     tim_clock_disable(ALL_CLOCKS & ~(never_reset_mask));
 }
 
-void common_hal_pulseio_pwmout_never_reset(pulseio_pwmout_obj_t *self) {
-    for(size_t i = 0 ; i < TIM_BANK_ARRAY_LEN; i++) {
-        if (mcu_tim_banks[i] == self->handle.Instance) {
-            never_reset_tim[i] = true;
-            never_reset_pin_number(self->tim->pin->port, self->tim->pin->number);
-            break;
-        }
-    }
-}
-
-void common_hal_pulseio_pwmout_reset_ok(pulseio_pwmout_obj_t *self) {
-    for(size_t i = 0 ; i < TIM_BANK_ARRAY_LEN; i++) {
-        if (mcu_tim_banks[i] == self->handle.Instance) {
-            never_reset_tim[i] = false;
-            break;
-        }
-    }
-}
-
 pwmout_result_t common_hal_pulseio_pwmout_construct(pulseio_pwmout_obj_t* self,
                                                     const mcu_pin_obj_t* pin,
                                                     uint16_t duty,
                                                     uint32_t frequency,
                                                     bool variable_frequency) {
     TIM_TypeDef * TIMx;
-    uint8_t tim_num = sizeof(mcu_tim_pin_list)/sizeof(*mcu_tim_pin_list);
+    uint8_t tim_num = MP_ARRAY_SIZE(mcu_tim_pin_list);
     bool tim_chan_taken = false;
     bool tim_taken_f_mismatch = false;
     bool var_freq_mismatch = false;
     bool first_time_setup = true;
 
-    for(uint i = 0; i < tim_num; i++) {
+    for (uint i = 0; i < tim_num; i++) {
         mcu_tim_pin_obj_t l_tim = mcu_tim_pin_list[i];
-        uint8_t l_tim_index = l_tim.tim_index-1;
-        uint8_t l_tim_channel = l_tim.channel_index-1;
+        uint8_t l_tim_index = l_tim.tim_index - 1;
+        uint8_t l_tim_channel = l_tim.channel_index - 1;
 
         //if pin is same
         if (l_tim.pin == pin) {
             //check if the timer has a channel active
             if (reserved_tim[l_tim_index] != 0) {
                 //is it the same channel? (or all channels reserved by a var-freq)
-                if (reserved_tim[l_tim_index] & 1<<(l_tim_channel)) {
+                if (reserved_tim[l_tim_index] & 1 << (l_tim_channel)) {
                     tim_chan_taken = true;
                     continue; //keep looking, might be another viable option
                 }
@@ -164,21 +145,21 @@ pwmout_result_t common_hal_pulseio_pwmout_construct(pulseio_pwmout_obj_t* self,
     }
 
     //handle valid/invalid timer instance
-    if (self->tim!=NULL) {
+    if (self->tim != NULL) {
         //create instance
-        TIMx = mcu_tim_banks[self->tim->tim_index-1];
+        TIMx = mcu_tim_banks[self->tim->tim_index - 1];
         //reserve timer/channel
         if (variable_frequency) {
-            reserved_tim[self->tim->tim_index-1] = 0x0F;
+            reserved_tim[self->tim->tim_index - 1] = 0x0F;
         } else {
-            reserved_tim[self->tim->tim_index-1] |= 1<<(self->tim->channel_index-1);
+            reserved_tim[self->tim->tim_index - 1] |= 1 << (self->tim->channel_index - 1);
         }
-        tim_frequencies[self->tim->tim_index-1] = frequency;
+        tim_frequencies[self->tim->tim_index - 1] = frequency;
     } else { //no match found
         if (tim_chan_taken) {
             mp_raise_ValueError(translate("No more timers available on this pin."));
         } else if (tim_taken_f_mismatch) {
-            mp_raise_ValueError(translate("Frequency must be the same as as the existing PWMOut using this timer"));
+            mp_raise_ValueError(translate("Frequency must match existing PWMOut using this timer"));
         } else if (var_freq_mismatch) {
             mp_raise_ValueError(translate("Cannot vary frequency on a timer that is already in use"));
         } else {
@@ -194,14 +175,15 @@ pwmout_result_t common_hal_pulseio_pwmout_construct(pulseio_pwmout_obj_t* self,
     GPIO_InitStruct.Alternate = self->tim->altfn_index;
     HAL_GPIO_Init(pin_port(pin->port), &GPIO_InitStruct);
 
-    tim_clock_enable(1<<(self->tim->tim_index - 1));
+    tim_clock_enable(1 << (self->tim->tim_index - 1));
 
     //translate channel into handle value
     self->channel = 4 * (self->tim->channel_index - 1);
 
     uint32_t prescaler = 0; //prescaler is 15 bit
     uint32_t period = 0; //period is 16 bit
-    timer_get_optimal_divisors(&period, &prescaler,frequency,timer_get_source_freq(self->tim->tim_index));
+    timer_get_optimal_divisors(&period, &prescaler, frequency, 
+                            timer_get_source_freq(self->tim->tim_index));
 
     //Timer init
     self->handle.Instance = TIMx;
@@ -241,6 +223,25 @@ pwmout_result_t common_hal_pulseio_pwmout_construct(pulseio_pwmout_obj_t* self,
     return PWMOUT_OK;
 }
 
+void common_hal_pulseio_pwmout_never_reset(pulseio_pwmout_obj_t *self) {
+    for (size_t i = 0; i < TIM_BANK_ARRAY_LEN; i++) {
+        if (mcu_tim_banks[i] == self->handle.Instance) {
+            never_reset_tim[i] = true;
+            never_reset_pin_number(self->tim->pin->port, self->tim->pin->number);
+            break;
+        }
+    }
+}
+
+void common_hal_pulseio_pwmout_reset_ok(pulseio_pwmout_obj_t *self) {
+    for(size_t i = 0; i < TIM_BANK_ARRAY_LEN; i++) {
+        if (mcu_tim_banks[i] == self->handle.Instance) {
+            never_reset_tim[i] = false;
+            break;
+        }
+    }
+}
+
 bool common_hal_pulseio_pwmout_deinited(pulseio_pwmout_obj_t* self) {
     return self->tim == mp_const_none;
 }
@@ -251,18 +252,18 @@ void common_hal_pulseio_pwmout_deinit(pulseio_pwmout_obj_t* self) {
     }
     //var freq shuts down entire timer, others just their channel
     if (self->variable_frequency) {
-        reserved_tim[self->tim->tim_index-1] = 0x00;
+        reserved_tim[self->tim->tim_index - 1] = 0x00;
     } else {
-        reserved_tim[self->tim->tim_index-1] &= ~(1<<self->tim->channel_index);
+        reserved_tim[self->tim->tim_index - 1] &= ~(1 << self->tim->channel_index);
         HAL_TIM_PWM_Stop(&self->handle, self->channel);
     }
     reset_pin_number(self->tim->pin->port,self->tim->pin->number);
     self->tim = mp_const_none;
 
     //if reserved timer has no active channels, we can disable it
-    if (!reserved_tim[self->tim->tim_index-1]) {
-        tim_frequencies[self->tim->tim_index-1] = 0x00;
-        tim_clock_disable(1<<(self->tim->tim_index-1));
+    if (!reserved_tim[self->tim->tim_index - 1]) {
+        tim_frequencies[self->tim->tim_index - 1] = 0x00;
+        tim_clock_disable(1 << (self->tim->tim_index - 1));
     }
 }
 
@@ -278,11 +279,14 @@ uint16_t common_hal_pulseio_pwmout_get_duty_cycle(pulseio_pwmout_obj_t* self) {
 
 void common_hal_pulseio_pwmout_set_frequency(pulseio_pwmout_obj_t* self, uint32_t frequency) {
     //don't halt setup for the same frequency
-    if (frequency == self->frequency) return;
+    if (frequency == self->frequency) { 
+        return;
+    }
 
     uint32_t prescaler = 0;
     uint32_t period = 0;
-    timer_get_optimal_divisors(&period, &prescaler,frequency,timer_get_source_freq(self->tim->tim_index));
+    timer_get_optimal_divisors(&period, &prescaler, frequency, 
+                                timer_get_source_freq(self->tim->tim_index));
 
     //shut down
     HAL_TIM_PWM_Stop(&self->handle, self->channel);
@@ -305,7 +309,7 @@ void common_hal_pulseio_pwmout_set_frequency(pulseio_pwmout_obj_t* self, uint32_
         mp_raise_ValueError(translate("Could not restart PWM"));
     }
 
-    tim_frequencies[self->tim->tim_index-1] = frequency;
+    tim_frequencies[self->tim->tim_index - 1] = frequency;
     self->frequency = frequency;
     self->period = period;
 }
@@ -320,80 +324,128 @@ bool common_hal_pulseio_pwmout_get_variable_frequency(pulseio_pwmout_obj_t* self
 
 STATIC void tim_clock_enable(uint16_t mask) {
     #ifdef TIM1
-    if (mask & 1<<0) __HAL_RCC_TIM1_CLK_ENABLE();
+    if (mask & (1 << 0)) {
+        __HAL_RCC_TIM1_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM2
-    if (mask & 1<<1) __HAL_RCC_TIM2_CLK_ENABLE();
+    if (mask & (1 << 1)) {
+        __HAL_RCC_TIM2_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM3
-    if (mask & 1<<2) __HAL_RCC_TIM3_CLK_ENABLE();
+    if (mask & (1 << 2)) {
+        __HAL_RCC_TIM3_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM4
-    if (mask & 1<<3) __HAL_RCC_TIM4_CLK_ENABLE();
+    if (mask & (1 << 3)) {
+        __HAL_RCC_TIM4_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM5
-    if (mask & 1<<4) __HAL_RCC_TIM5_CLK_ENABLE();
+    if (mask & (1 << 4)) {
+        __HAL_RCC_TIM5_CLK_ENABLE();
+    }
     #endif
     //6 and 7 are reserved ADC timers
     #ifdef TIM8
-    if (mask & 1<<7) __HAL_RCC_TIM8_CLK_ENABLE();
+    if (mask & (1 << 7)) {
+        __HAL_RCC_TIM8_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM9
-    if (mask & 1<<8) __HAL_RCC_TIM9_CLK_ENABLE();
+    if (mask & (1 << 8)) {
+        __HAL_RCC_TIM9_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM10
-    if (mask & 1<<9) __HAL_RCC_TIM10_CLK_ENABLE();
+    if (mask & (1 << 9)) {
+        __HAL_RCC_TIM10_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM11
-    if (mask & 1<<10) __HAL_RCC_TIM11_CLK_ENABLE();
+    if (mask & (1 << 10)) {
+        __HAL_RCC_TIM11_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM12
-    if (mask & 1<<11) __HAL_RCC_TIM12_CLK_ENABLE();
+    if (mask & (1 << 11)) {
+        __HAL_RCC_TIM12_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM13
-    if (mask & 1<<12) __HAL_RCC_TIM13_CLK_ENABLE();
+    if (mask & (1 << 12)) {
+        __HAL_RCC_TIM13_CLK_ENABLE();
+    }
     #endif
     #ifdef TIM14
-    if (mask & 1<<13) __HAL_RCC_TIM14_CLK_ENABLE();
+    if (mask & (1 << 13)) {
+        __HAL_RCC_TIM14_CLK_ENABLE();
+    }
     #endif
 }
 
 STATIC void tim_clock_disable(uint16_t mask) {
     #ifdef TIM1
-    if (mask & 1<<0) __HAL_RCC_TIM1_CLK_DISABLE();
+    if (mask & (1 << 0)) {
+        __HAL_RCC_TIM1_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM2
-    if (mask & 1<<1) __HAL_RCC_TIM2_CLK_DISABLE();
+    if (mask & (1 << 1)) {
+        __HAL_RCC_TIM2_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM3
-    if (mask & 1<<2) __HAL_RCC_TIM3_CLK_DISABLE();
+    if (mask & (1 << 2)) {
+        __HAL_RCC_TIM3_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM4
-    if (mask & 1<<3) __HAL_RCC_TIM4_CLK_DISABLE();
+    if (mask & (1 << 3)) {
+        __HAL_RCC_TIM4_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM5
-    if (mask & 1<<4) __HAL_RCC_TIM5_CLK_DISABLE();
+    if (mask & (1 << 4)) {
+        __HAL_RCC_TIM5_CLK_DISABLE();
+    }
     #endif
     //6 and 7 are reserved ADC timers
     #ifdef TIM8
-    if (mask & 1<<7) __HAL_RCC_TIM8_CLK_DISABLE();
+    if (mask & (1 << 7)) {
+        __HAL_RCC_TIM8_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM9
-    if (mask & 1<<8) __HAL_RCC_TIM9_CLK_DISABLE();
+    if (mask & (1 << 8)) {
+        __HAL_RCC_TIM9_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM10
-    if (mask & 1<<9) __HAL_RCC_TIM10_CLK_DISABLE();
+    if (mask & (1 << 9)) {
+        __HAL_RCC_TIM10_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM11
-    if (mask & 1<<10) __HAL_RCC_TIM11_CLK_DISABLE();
+    if (mask & (1 << 10)) {
+        __HAL_RCC_TIM11_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM12
-    if (mask & 1<<11) __HAL_RCC_TIM12_CLK_DISABLE();
+    if (mask & (1 << 11)) {
+        __HAL_RCC_TIM12_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM13
-    if (mask & 1<<12) __HAL_RCC_TIM13_CLK_DISABLE();
+    if (mask & (1 << 12)) {
+        __HAL_RCC_TIM13_CLK_DISABLE();
+    }
     #endif
     #ifdef TIM14
-    if (mask & 1<<13) __HAL_RCC_TIM14_CLK_DISABLE();
+    if (mask & (1 << 13)) {
+        __HAL_RCC_TIM14_CLK_DISABLE();
+    }
     #endif
 }