-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix lost board.SPI and board.I2C #3603
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me but I would also like @tannewt to weigh in on the movement of reset_port in particular. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your careful analysis!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good to me! I think we'll want this in 6.0.x though and then we can merge 6.0.x into main
.
Would you mind rebasing onto 6.0.x and then changing the PR's base? Thanks! |
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 adafruit#3581 for SPI. This needs to be solved in the same way for I2C and SPI.
Fixes adafruit#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.
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.
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Thanks! |
The first commit brings
board.I2C
into the same state asboard.SPI
by fixing something that was clearly wrong, but by itself caused no severe problems. It thereby introduces the problem described in #3581 forboard.SPI
also forboard.I2C
. See commit message for details.The second commit fixes #3581 for both SPI and I2C.
I found no reason in the code or in the Git history for
reset_port()
to come beforereset_board_busses()
incleanup_after_vm()
, so I reversed them.For symmetry, since setting
spi_singleton
to non-NULL occurs together withcommon_hal_busio_spi_construct()
, setting it to NULL should also occur together withcommon_hal_busio_spi_deinit()
(and likewise for I2C). This makes the pins resettable and allows the swappedreset_port()
to do its job. (The symmetry is not complete becausecommon_hal_busio_spi_deinit()
can also occur without settingspi_singleton
to NULL, i.e.spi_singleton
can end up pointing at a non-NULL deinited object. This is dealt with in the third commit, see below.)Another desirable symmetry would be that since
common_hal_displayio_fourwire_construct()
callscommon_hal_busio_spi_never_reset()
,common_hal_displayio_fourwire_deinit()
should also callcommon_hal_busio_spi_reset()
(which doesn’t exist, but would do the reverse of…_never_reset()
) (and likewise fori2cdisplay
). However, this is not necessary, since thenever_reset
flags only take effect at the end of the session, and at that time everything is already handled correctly by the changes made here. There is effectively a redundancy between thenever_reset
flags andreset_board_busses()
checking for things still in use by displays. Therefore I avoided adding that symmetry, which would just add more code.Critical review appreciated.
The third commit fixes a tangentially related issue: After one had explicitly deinited the
board.SPI
orboard.I2C
busses (maybe to temporarily use their pins for other purposes), there was no way to get them back in the same session. Calling the function again would just return the unusable deinited object. Reproducible as follows:or
Testing on non-SAMD ports would be appreciated.