Skip to content
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

Avoid AttributeError when calling start_wifi() after stop_bluetooth() #8

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

glenrobertson
Copy link
Contributor

stop_bluetooth() sets _uart to None, so when calling start_wifi(), the call to reset that tries to read self._uart raises a Value Error. Check that _uart is not None

`stop_bluetooth()` sets _uart to None, so when calling `start_wifi()`, the call to reset that tries to read self._uart raises a Value Error.
Check that _uart is not None
@glenrobertson
Copy link
Contributor Author

@tekktrik

@tekktrik
Copy link
Member

tekktrik commented Jan 2, 2023

Hi, unless there's a specific reason you tagged me, I'll mark this as ready for review and someone will take a look at the earliest opportunity :)

@tekktrik tekktrik requested a review from a team January 2, 2023 21:41
@glenrobertson
Copy link
Contributor Author

@tekktrik thanks! Wasn't sure what the process was and saw you were a committer. Sorry to bother you

Copy link
Member

@tekktrik tekktrik left a comment

Choose a reason for hiding this comment

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

Taking a look at this, I think preventing the AttributeError is good, but the fix may be slightly more extensive since looking at this now, it seems like this will now just raise the RuntimeError if the Bluetooth is off at all. I think the fix is to also only attempt to check if startup_message is empty (and print it if debug is true) if self._uart is not None. Was that an issue you experienced with this modified code?

@glenrobertson
Copy link
Contributor Author

glenrobertson commented Jan 30, 2023

@tekktrik thank you for taking an initial look and providing comments.
I updated what I have to only print the startup_message if it is set. I changed it slightly to avoid the negated logic, as I thought it might be more readable.

Since I got back to my project that uses this library, I realized an issue where toggling between bluetooth/wifi a few times, subsequent calls to start_bluetooth() can cause RuntimeError: Too many adapters, due to the subsequent calls to the constructor _bleio.Adapter(.....)

My use case is that I use bluetooth for pairing so an app can send the wifi credentials, and once they're received, it switches over to wifi to use them. If they are revoked for some reason, I needed to switch back to bluetooth to pair again.

I resolved this in 912f82f by avoiding reinitialization of _bleio_adapter. I tested it in my own code by using the following to start bluetooth after it had been started and stopped in the past:

if self.esp32._bleio_adapter is None:
    adapter = self.esp32.start_bluetooth()
    _bleio.set_adapter(adapter)
else:
    # avoid "RuntimeError: Too many adapters"
    self.esp32.reset(ESP32.BLUETOOTH)
    self.esp32._busy_cts.switch_to_input()
    self.esp32._gpio0_rts.switch_to_output()
    self.esp32._bleio_adapter.enabled = True
    adapter = self.esp32._bleio_adapter

Let me know what you think! Thanks in advance and apologies for the delay in responding.

@tekktrik tekktrik self-requested a review January 30, 2023 21:36
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

I pushed a commit with code format / black changes to make the actions check happy.

This looks good to me now. If I'm following the logic correctly it now checks for existence of the bus or adapter internally rather than always initing a new one and shouldn't try to access a field that doesn't exist yet.

I tested this version successfully with this code intended to try to reproduce the issue described by switching back and forth from bluetooth to wifi and then back.

import time
import _bleio
from adafruit_airlift.esp32 import ESP32

esp32 = ESP32()
adapter = esp32.start_bluetooth(debug=True)
_bleio.set_adapter(adapter)  # pylint: disable=no-member

print(_bleio.adapter.address)

time.sleep(5)

esp32.stop_bluetooth()
print("bluetooth stopped")

time.sleep(3)

esp32.start_wifi(debug=True)
print("wifi started")

time.sleep(3)

esp32.stop_wifi()
print("wifi stopped")

adapter = esp32.start_bluetooth(debug=True)
_bleio.set_adapter(adapter)  # pylint: disable=no-member

print(_bleio.adapter.address)

With the version from this PR the code seems to run and function as intended.

@FoamyGuy FoamyGuy merged commit 83ad08a into adafruit:main Feb 27, 2023
@tekktrik tekktrik removed their request for review February 27, 2023 17:35
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Feb 28, 2023
Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306 to 1.6.0 from 1.5.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SSD1306#31 from dglaude/0.49''-64-x-32
  > Add upload url to release action
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731 to 3.3.7 from 3.3.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3731#53 from EAGrahamJr/fade_52
  > Add upload url to release action
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_PN532 to 2.3.16 from 2.3.14:
  > Merge pull request adafruit/Adafruit_CircuitPython_PN532#63 from caternuson/iss44
  > Merge pull request adafruit/Adafruit_CircuitPython_PN532#62 from caternuson/iss49
  > Add upload url to release action
  > Add .venv to .gitignore

Updating https://github.com/adafruit/Adafruit_CircuitPython_RFM69 to 2.1.14 from 2.1.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_RFM69#44 from jerryneedell/jerryn_size
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1675 to 1.1.16 from 1.1.15:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1675#14 from jposada202020/updating_example
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_SSD1680 to 1.0.14 from 1.0.12:
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1680#12 from jposada202020/adding_breakout_example
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1680#11 from jposada202020/main
  > Merge pull request adafruit/Adafruit_CircuitPython_SSD1680#9 from dave-ct/128
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_Touchscreen to 1.2.0 from 1.1.17:
  > Merge pull request adafruit/Adafruit_CircuitPython_Touchscreen#23 from rtwfroody/invert
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_AirLift to 1.0.9 from 1.0.8:
  > Merge pull request adafruit/Adafruit_CircuitPython_AirLift#8 from glenrobertson/patch-1
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.7.0 from 2.6.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#105 from FoamyGuy/multicolor_comet

Updating https://github.com/adafruit/Adafruit_CircuitPython_MatrixPortal to 3.0.11 from 3.0.10:
  > Merge pull request adafruit/Adafruit_CircuitPython_MatrixPortal#88 from Lnk2past/master
  > Add upload url to release action
  > Add .venv to .gitignore
  > Update .pylintrc for v2.15.5
  > Fix release CI files
  > Update pylint to 2.15.5
  > Updated pylint version to 2.13.0
  > Switching to composite actions

Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.3.1 from 7.3.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#156 from vladak/suback_var_payload
  > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#153 from vladak/back_off_tests

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
@glenrobertson
Copy link
Contributor Author

@FoamyGuy thank you for helping with this and merging! And thank you @tekktrik for taking a look and providing comments too.

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.

None yet

3 participants