address review

Author: dhalbert

lib/tinyusb

@@ -53,7 +53,8 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     uint32_t sda_pinmux, scl_pinmux;
 
     // Ensure the object starts in its deinit state.
-    self->sda_pin = NO_PIN;
+    common_hal_busio_i2c_mark_deinit(self);
+
     Sercom *sercom = samd_i2c_get_sercom(scl, sda, &sercom_index, &sda_pinmux, &scl_pinmux);
     if (sercom == NULL) {
         raise_ValueError_invalid_pins();
@@ -108,7 +109,6 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
         mp_arg_error_invalid(MP_QSTR_frequency);
     }
 
-
     if (i2c_m_sync_enable(&self->i2c_desc) != ERR_NONE) {
         common_hal_busio_i2c_deinit(self);
         mp_raise_OSError(MP_EIO);
@@ -141,8 +141,6 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 
     reset_pin_number(self->sda_pin);
     reset_pin_number(self->scl_pin);
-    self->sda_pin = NO_PIN;
-    self->scl_pin = NO_PIN;
     common_hal_busio_i2c_mark_deinit(self);
 }
 

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

@@ -27,9 +27,14 @@ static bool i2c_in_use[NUM_I2C];
 
 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) {
+
+    // Ensure the object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     size_t instance_index = NUM_I2C;
     uint8_t scl_alt = 0;
     uint8_t sda_alt = 0;
+
     for (scl_alt = 0; scl_alt < 6; scl_alt++) {
         if (scl->functions[scl_alt].type != PIN_FUNCTION_I2C ||
             i2c_in_use[scl->functions[scl_alt].index] ||
@@ -82,8 +87,6 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 
     common_hal_reset_pin(self->sda_pin);
     common_hal_reset_pin(self->scl_pin);
-    self->sda_pin = NULL;
-    self->scl_pin = NULL;
     common_hal_busio_i2c_mark_deinit(self);
 }
 

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

@@ -16,6 +16,10 @@
 
 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) {
+
+    // Ensure the object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     if (frequency != I2C_SPEED_STANDARD && frequency != I2C_SPEED_FAST) {
         mp_arg_error_invalid(MP_QSTR_frequency);
     }

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

@@ -16,6 +16,10 @@
 
 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_us) {
+
+    // Ensure the object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     // Pins 45 and 46 are "strapping" pins that impact start up behavior. They usually need to
     // be pulled-down so pulling them up for I2C is a bad idea. To make this hard, we don't
     // support I2C on these pins.
@@ -125,13 +129,11 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 
     common_hal_reset_pin(self->sda_pin);
     common_hal_reset_pin(self->scl_pin);
-    self->sda_pin = NULL;
-    self->scl_pin = NULL;
     common_hal_busio_i2c_mark_deinit(self);
 }
 
 bool common_hal_busio_i2c_probe(busio_i2c_obj_t *self, uint8_t addr) {
-    esp_err_t result = i2c_master_probe(self->handle, addr, 1);
+    esp_err_t result = i2c_master_probe(self->handle, addr, 10);
     return result == ESP_OK;
 }
 

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

@@ -78,6 +78,10 @@ void common_hal_espcamera_camera_construct(
     self->i2c = i2c;
 
     // These pins might be NULL because they were not specified.
+    // Note that common_hal_mcu_pin_number() returns NO_PIN (- =1) if pass NULL, but as a `uint8_t`,
+    // so it becomes 255. The camera driver expects non-specified pins to be < 0.
+    // So we have to set the pins to NO_PIN explicitly,
+    // instead of relying on the return value from common_hal_mcu_pin_number().
     self->camera_config.pin_pwdn = powerdown_pin ? common_hal_mcu_pin_number(powerdown_pin) : NO_PIN;
     self->camera_config.pin_reset = reset_pin ? common_hal_mcu_pin_number(reset_pin) : NO_PIN;
     self->camera_config.pin_xclk = external_clock_pin ? common_hal_mcu_pin_number(external_clock_pin) : NO_PIN;

ports/espressif/common-hal/espcamera/Camera.c

@@ -66,6 +66,9 @@ static void i2c_check_pin_config(const mcu_pin_obj_t *pin, uint32_t pull) {
 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) {
 
+    // Ensure the object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     #if CIRCUITPY_REQUIRE_I2C_PULLUPS
     // Test that the pins are in a high state. (Hopefully indicating they are pulled up.)
     IOMUXC_SetPinMux(sda->mux_reg, IOMUXC_SW_MUX_CTL_PAD_MUX_MODE_ALT5, 0, 0, 0, 0);
@@ -162,8 +165,6 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     common_hal_reset_pin(self->sda->pin);
     common_hal_reset_pin(self->scl->pin);
 
-    self->sda = NULL;
-    self->scl = NULL;
     common_hal_busio_i2c_mark_deinit(self);
 }
 
@@ -179,6 +180,9 @@ bool common_hal_busio_i2c_probe(busio_i2c_obj_t *self, uint8_t addr) {
 }
 
 bool common_hal_busio_i2c_try_lock(busio_i2c_obj_t *self) {
+    if (common_hal_busio_i2c_deinited(self)) {
+        return false;
+    }
     bool grabbed_lock = false;
 //    CRITICAL_SECTION_ENTER()
     if (!self->has_lock) {

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

@@ -74,6 +74,10 @@ static void twim_event_handler(nrfx_twim_evt_t const *p_event, void *p_context)
 }
 
 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) {
+
+    // Ensure the object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     if (scl->number == sda->number) {
         raise_ValueError_invalid_pins();
     }
@@ -155,8 +159,6 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 
     reset_pin_number(self->sda_pin_number);
     reset_pin_number(self->scl_pin_number);
-    self->sda_pin_number = NO_PIN;
-    self->scl_pin_number = NO_PIN;
 
     self->twim_peripheral->in_use = false;
     common_hal_busio_i2c_mark_deinit(self);

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

@@ -26,6 +26,10 @@ static i2c_inst_t *i2c[2] = {i2c0, i2c1};
 
 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) {
+
+    // Ensure object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     self->peripheral = NULL;
     // I2C pins have a regular pattern. SCL is always odd and SDA is even. They match up in pairs
     // so we can divide by two to get the instance. This pattern repeats.
@@ -109,8 +113,6 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 
     reset_pin_number(self->sda_pin);
     reset_pin_number(self->scl_pin);
-    self->sda_pin = NO_PIN;
-    self->scl_pin = NO_PIN;
     common_hal_busio_i2c_mark_deinit(self);
 }
 

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

@@ -39,6 +39,9 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     const mcu_pin_obj_t *sda,
     uint32_t frequency, uint32_t timeout) {
 
+    // Ensure the object starts in its deinit state.
+    common_hal_busio_i2c_mark_deinit(self);
+
     if ((scl != NULL) && (sda != NULL)) {
         if (scl->function_list[ DEFAULT_I2C_PERIPHERAL == I2C1?
                                 FN_I2C1_SCL : FN_I2C0_SCL] == 1 &&
@@ -88,8 +91,6 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     I2C_Reset(self->i2cspm);
     common_hal_reset_pin(self->sda);
     common_hal_reset_pin(self->scl);
-    self->sda = NULL;
-    self->scl = NULL;
     self->i2cspm = NULL;
     in_used = false;
     common_hal_busio_i2c_mark_deinit(self);