From 1117edae63df7b66f1575980bf54d6836f9a8aa8 Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Sat, 24 Oct 2020 17:13:10 +0200 Subject: [PATCH 1/3] Fix erroneous claim of board.I2C() by deinited display. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the end of a session that called displayio.release_displays() (and did not initialize a new display), a board.I2C() bus that was previously used by a display would wrongly be considered still in use. While I can’t think of any unrecoverable problem this would cause in the next session, it violates the assumption that a soft reboot resets everything not needed by displays, potentially leading to confusion. By itself, this change does not fix the problem yet - rather, it introduces the same issue as in #3581 for SPI. This needs to be solved in the same way for I2C and SPI. --- shared-module/board/__init__.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared-module/board/__init__.c b/shared-module/board/__init__.c index 39b68a0f11..856103b7b6 100644 --- a/shared-module/board/__init__.c +++ b/shared-module/board/__init__.c @@ -139,7 +139,7 @@ void reset_board_busses(void) { bool display_using_i2c = false; #if CIRCUITPY_DISPLAYIO for (uint8_t i = 0; i < CIRCUITPY_DISPLAY_LIMIT; i++) { - if (displays[i].i2cdisplay_bus.bus == i2c_singleton) { + if (displays[i].bus_base.type == &displayio_i2cdisplay_type && displays[i].i2cdisplay_bus.bus == i2c_singleton) { display_using_i2c = true; break; } From f4f80e07ca3dc2f5a93484e5342540837a94a505 Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Sat, 24 Oct 2020 17:23:17 +0200 Subject: [PATCH 2/3] Fix lost board.SPI and board.I2C after being used by display. Fixes #3581. Pins were marked as never_reset by common_hal_displayio_fourwire_construct() and common_hal_sharpdisplay_framebuffer_construct(), but these marks were never removed, so at the end of a session after displayio.release_displays(), {spi|i2c}_singleton would be set to NULL but the pins would not be reset. In the next session, board.SPI() and board.I2C() were unable to reconstruct the object because the pins were still in use. For symmetry with creation of the singleton, add deinitialization before setting it to NULL in reset_board_busses(). This makes the pins resettable, so that reset_port(), moved behind it, then resets them. --- main.c | 4 +++- shared-module/board/__init__.c | 14 +++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/main.c b/main.c index b43b3b8c80..80b163f607 100755 --- a/main.c +++ b/main.c @@ -234,10 +234,12 @@ void cleanup_after_vm(supervisor_allocation* heap) { common_hal_canio_reset(); #endif - reset_port(); + // reset_board_busses() first because it may release pins from the never_reset state, so that + // reset_port() can reset them. #if CIRCUITPY_BOARD reset_board_busses(); #endif + reset_port(); reset_board(); reset_status_led(); } diff --git a/shared-module/board/__init__.c b/shared-module/board/__init__.c index 856103b7b6..967b430d2b 100644 --- a/shared-module/board/__init__.c +++ b/shared-module/board/__init__.c @@ -145,8 +145,11 @@ void reset_board_busses(void) { } } #endif - if (!display_using_i2c) { - i2c_singleton = NULL; + if (i2c_singleton != NULL) { + if (!display_using_i2c) { + common_hal_busio_i2c_deinit(i2c_singleton); + i2c_singleton = NULL; + } } #endif #if BOARD_SPI @@ -169,9 +172,10 @@ void reset_board_busses(void) { // make sure SPI lock is not held over a soft reset if (spi_singleton != NULL) { common_hal_busio_spi_unlock(spi_singleton); - } - if (!display_using_spi) { - spi_singleton = NULL; + if (!display_using_spi) { + common_hal_busio_spi_deinit(spi_singleton); + spi_singleton = NULL; + } } #endif #if BOARD_UART From 99a3750a2cfd1da8cdd266ca9a0367f0c3a54092 Mon Sep 17 00:00:00 2001 From: Christian Walther Date: Sat, 24 Oct 2020 17:50:11 +0200 Subject: [PATCH 3/3] Fix lost board.SPI and board.I2C after explicitly deiniting them. After calling board.SPI().deinit(), calling board.SPI() again would return the unusable deinited object and there was no way of getting it back into an initialized state until the end of the session. --- shared-bindings/board/__init__.c | 10 ++++++++-- shared-module/board/__init__.c | 10 ++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/shared-bindings/board/__init__.c b/shared-bindings/board/__init__.c index 6837fc41c2..717698408a 100644 --- a/shared-bindings/board/__init__.c +++ b/shared-bindings/board/__init__.c @@ -28,6 +28,12 @@ #include "py/runtime.h" #include "shared-bindings/board/__init__.h" +#if BOARD_I2C +#include "shared-bindings/busio/I2C.h" +#endif +#if BOARD_SPI +#include "shared-bindings/busio/SPI.h" +#endif //| """Board specific pin names //| @@ -45,7 +51,7 @@ #if BOARD_I2C mp_obj_t board_i2c(void) { mp_obj_t singleton = common_hal_board_get_i2c(); - if (singleton != NULL) { + if (singleton != NULL && !common_hal_busio_i2c_deinited(singleton)) { return singleton; } assert_pin_free(DEFAULT_I2C_BUS_SDA); @@ -69,7 +75,7 @@ MP_DEFINE_CONST_FUN_OBJ_0(board_i2c_obj, board_i2c); #if BOARD_SPI mp_obj_t board_spi(void) { mp_obj_t singleton = common_hal_board_get_spi(); - if (singleton != NULL) { + if (singleton != NULL && !common_hal_busio_spi_deinited(singleton)) { return singleton; } assert_pin_free(DEFAULT_SPI_BUS_SCK); diff --git a/shared-module/board/__init__.c b/shared-module/board/__init__.c index 967b430d2b..2f1c34e565 100644 --- a/shared-module/board/__init__.c +++ b/shared-module/board/__init__.c @@ -55,9 +55,8 @@ mp_obj_t common_hal_board_get_i2c(void) { } mp_obj_t common_hal_board_create_i2c(void) { - if (i2c_singleton != NULL) { - return i2c_singleton; - } + // All callers have either already verified this or come so early that it can't be otherwise. + assert(i2c_singleton == NULL || common_hal_busio_i2c_deinited(i2c_singleton)); busio_i2c_obj_t *self = &i2c_obj; self->base.type = &busio_i2c_type; @@ -79,9 +78,8 @@ mp_obj_t common_hal_board_get_spi(void) { } mp_obj_t common_hal_board_create_spi(void) { - if (spi_singleton != NULL) { - return spi_singleton; - } + // All callers have either already verified this or come so early that it can't be otherwise. + assert(spi_singleton == NULL || common_hal_busio_spi_deinited(spi_singleton)); busio_spi_obj_t *self = &spi_obj; self->base.type = &busio_spi_type;