Skip to content

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

Merged
merged 3 commits into from
Oct 27, 2020
Merged

Fix lost board.SPI and board.I2C #3603

merged 3 commits into from
Oct 27, 2020

Conversation

cwalther
Copy link

The first commit brings board.I2C into the same state as board.SPI by fixing something that was clearly wrong, but by itself caused no severe problems. It thereby introduces the problem described in #3581 for board.SPI also for board.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 before reset_board_busses() in cleanup_after_vm(), so I reversed them.

For symmetry, since setting spi_singleton to non-NULL occurs together with common_hal_busio_spi_construct(), setting it to NULL should also occur together with common_hal_busio_spi_deinit() (and likewise for I2C). This makes the pins resettable and allows the swapped reset_port() to do its job. (The symmetry is not complete because common_hal_busio_spi_deinit() can also occur without setting spi_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() calls common_hal_busio_spi_never_reset(), common_hal_displayio_fourwire_deinit() should also call common_hal_busio_spi_reset() (which doesn’t exist, but would do the reverse of …_never_reset()) (and likewise for i2cdisplay). However, this is not necessary, since the never_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 the never_reset flags and reset_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 or board.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:

Adafruit CircuitPython 6.0.0-rc.0-99-g32c2e9354-dirty on 2020-10-25; PewPew M4 CW with samd51G19
>>> import board
>>> bus = board.SPI()
>>> bus.try_lock()
True
>>> bus.write(b'X')
>>> bus.unlock()
>>> bus.deinit()
>>> 
>>> bus = board.SPI()
>>> bus.try_lock()
True
>>> bus.write(b'X')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Object has been deinitialized and can no longer be used. Create a new object.
>>> 

or

# with an SSD1306 display connected to the I2C pins
Adafruit CircuitPython 6.0.0-rc.0-99-g32c2e9354-dirty on 2020-10-25; PewPew M4 CW with samd51G19
>>> import board
>>> bus = board.I2C()
>>> bus.try_lock()
True
>>> bus.writeto(60, b'X')
>>> bus.unlock()
>>> bus.deinit()
>>> 
>>> bus = board.I2C()
>>> bus.try_lock()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Object has been deinitialized and can no longer be used. Create a new object.
>>> 

Testing on non-SAMD ports would be appreciated.

@dhalbert dhalbert requested a review from tannewt October 24, 2020 23:36
Copy link

@jepler jepler left a 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!

dhalbert
dhalbert previously approved these changes Oct 25, 2020
Copy link
Collaborator

@dhalbert dhalbert left a 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!

Copy link
Member

@tannewt tannewt left a 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.

@tannewt tannewt changed the base branch from main to 6.0.x October 26, 2020 21:14
@tannewt tannewt changed the base branch from 6.0.x to main October 26, 2020 21:15
@tannewt
Copy link
Member

tannewt commented Oct 26, 2020

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.
@cwalther cwalther changed the base branch from main to 6.0.x October 26, 2020 21:50
@cwalther
Copy link
Author

Done.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@tannewt tannewt merged commit 563a893 into adafruit:6.0.x Oct 27, 2020
@cwalther
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

board.SPI lost after being used by display
4 participants