Skip to content

Provide proxies for I2C and SPI #62

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 4 commits into from
Sep 26, 2022
Merged

Conversation

rsbohn
Copy link
Contributor

@rsbohn rsbohn commented Aug 20, 2022

basic.Adafruit_BME280 provides a simplified interface to the device.
advanced.Adafruit_BME280 provides additional functionality.

Each of the above classes has subclasses for _I2C and _SPI busses.
The user will instantiate the appropriate subclass based on their connection.
basic.Adafruit_BME280_I2C or basic.Adafruit_BME280_SPI for example.

pylint complains that there is duplicate code between the basic and advanced implementations. The code to instantiate, read, and write to the device
is duplicated in both implementations.

This pull request provides a single implementation for I2C and for SPI.
To use *_Impl I added a parameter to the init method
to set the selected implementation class.

Questions:

  • Is 'protocol.py' the right name for the file containing the different bus-specific implementations?
  • Is there a better name than *_Impl?
  • Is this a proxy or perhaps a delegate? The data attribute in Adafruit_BME280 should be named accordingly.

This work was part of the CircuitPythonDay2022 Sprint.

@tekktrik tekktrik requested a review from KeithTheEE August 20, 2022 17:41
@tekktrik tekktrik linked an issue Aug 20, 2022 that may be closed by this pull request
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.

This is looking pretty good to me. Looked over the code, but don't have a sensor to use for testing.

Is 'protocol.py' the right name for the file containing the different bus-specific implementations?

I think protocol.py is a good name for it. It could perhaps be implementations.py but I'm not really sure that is any more clear so not sure if it's better really. I can't think of anything that seems definitely better than protocol.py

Is there a better name than *_Impl?

I can't think of anything better than Impl, I think implementation conveys the idea of what it's doing well. Open to input from others though.

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 had another look over this, and does look good to me.

I'll wait and see if we can get someone to test it with hardware. I can try to get a sensor after a bit if no one tries it out first.

@FoamyGuy
Copy link
Contributor

I tested this successfully with

Adafruit CircuitPython 8.0.0-beta.0 on 2022-08-18; Adafruit Feather ESP32S2 with ESP32S2
Board ID:adafruit_feather_esp32s2

Using the simpletest script in the examples directory of this repo to connect via I2C and get readings from the sensor.

Thanks again for working on this Dexter.

@FoamyGuy FoamyGuy merged commit c7d4e1c into adafruit:main Sep 26, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Sep 27, 2022
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.

Code fails duplicate-code check, consider refactor
2 participants