8000 Moving Adafruit_CircuitPython_BusDevice to core by gamblor21 · Pull Request #3612 · adafruit/circuitpython · GitHub
[go: up one dir, main page]

Skip to content

Moving Adafruit_CircuitPython_BusDevice to core #3612

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 20 commits into from
Dec 2, 2020

Conversation

gamblor21
Copy link
Member

Moving the Bus Device library into the core. This is a direct translation of the python into C to hopefully achieve some speedup and potential size reduction.

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.

Thank you for doing this! I did a preliminary look, even though this is a draft.

@dhalbert
Copy link
Collaborator

BTW, If your editor has the features or a plug-in, set it up to make sure there's a newline at the end of the file, and also to delete trailing whitespace on each line, either via a command or on save (more convenient). Emacs can do this automatically, and I've seen that some editors can do this too.

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.

Thanks! I have some areas of improvement to suggest.

@gamblor21
Copy link
Member Author

I have a question about the import equivalency of moving this to the core (I2CDevice and SPIDevice are equivalent to their python originals).

In the python library a user may use:
import adafruit_bus_device.spi_device as spi_device self._spi = spi_device.SPIDevice(spi, cs, baudrate=baudrate)

To call it now they would use:

import busdevice self._spi = busdevice.SPIDevice(spi, cs, baudrate=baudrate)

As the spi_device (and i2c_device) .py files no longer exist is there any problem with this? I got these examples from the BME280 library so was thinking the libraries would have to be updated to use the new core module if that's required.

Also wondering how we deal with the case if we cannot include busdevice in the core of the M0 boards you don't want a library to fail.

This may be easier to find time to talk about on discord so feel free to ping me there to set up a time.

@tannewt
Copy link
Member
tannewt commented Nov 3, 2020

Why not just name the core library adafruit_busdevice as well?

@siddacious
Copy link

Why not just name the core library adafruit_busdevice as well?

Do you mean adafruit_bus_device? That will allow existing libs to 'just work' which would make me happy :)

@gamblor21
Copy link
Member Author

Why not just name the core library adafruit_busdevice as well?

Do you mean adafruit_bus_device? That will allow existing libs to 'just work' which would make me happy :)

Yes I do. The last commits I made were to rename everything so it should "just work". But have only tested it with one library (BME280) so far.

@FoamyGuy
Copy link
Collaborator
FoamyGuy commented Nov 6, 2020

I tested this successfully with Feather Sense using some of the on-board sensors:

accel_gyro = LSM6DS(board.I2C())
mag = LIS3MDL(board.I2C())

I took adafruit_bus_device out of lib folder. Confirmed that it crashes on 6.0.0-rc.1 due to missing the library.

Installed the uf2 built from this PR and it's working properly to read values from both sensors. 🎉 This is awesome! Thank you for working on it @gamblor21

@FoamyGuy
Copy link
Collaborator
FoamyGuy commented Nov 7, 2020

Also tested this successfully with MONSTER M4SK build. Deleted adafruit_bus_device from lib folder and I am still able to use the onboard seesaw and the lis3dh accelerometer. Everything is working well.

@FoamyGuy
Copy link
Collaborator
FoamyGuy commented Nov 7, 2020

I tested it successfully with STM32F405 and an MPU-6050 as well. No bus_device in lib folder, am still able to successfully read values from the sensor.

@tannewt
Copy link
Member
tannewt commented Nov 9, 2020

@gamblor21 Want to mark this as ready?

@gamblor21 gamblor21 marked this pull request as ready for review November 9, 2020 23:29
@gamblor21
Copy link
Member Author

Question 1: Should I make all the M0 small builds exclude this? (Basically all the ones that are failing)

Question 2: not 100% sure if I should be running "RUN_BACKGROUND_TASKS and mp_handle_pending" during the I2C enter loop. When I did try it once it was very slow. Not sure why. I could experiment some more too.

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.

Question 1: Should I make all the M0 small builds exclude this? (Basically all the ones that are failing)

Yes, for most of them. Just keep an eye out for the ones that freeze the python version in (like CPX). In those, you should be able to remove the frozen version in favor of the native one.

Question 2: not 100% sure if I should be running "RUN_BACKGROUND_TASKS and mp_handle_pending" during the I2C enter loop. When I did try it once it was very slow. Not sure why. I could experiment some more too.

See my suggestion. It tries to get the lock once before checking for background work. You could do mp_handle_pending after the loop if success is false.

@FoamyGuy
Copy link
Collaborator

I did one more test this morning with Metro ESP32-S2 and a SSD1306 I2C screen. It's working well with those also. No bus_device in lib folder, still able to use the display.

@jerryneedell
Copy link
Collaborator
jerryneedell commented Nov 11, 2020

@gamblor21 just a reminder... the frozen module should be removed from the feather_m0_rfm69 and feather_m0_rfm9x builds.
Thanks!

@gamblor21
Copy link
Member Author

@gamblor21 just a reminder... the frozen module should be removed from the feather_m0_rfm69 and feather_m0_rfm9x builds.
Thanks!

Ah thanks, I knew I missed something else. I'll do a search for all the frozen modules to make sure I get them all the next time.

@tannewt tannewt requested a review from dhalbert November 12, 2020 01:23
@tannewt
Copy link
Member
tannewt commented Dec 1, 2020

Where is this at?

@gamblor21
Copy link
Member Author

There was one question I asked Dan about an optimization, but otherwise as far as I am aware it is ready (minus the couple conflicts I now see which should be easy to fix).

@tannewt
Copy link
Member
tannewt commented Dec 1, 2020

Thanks! I replied to that comment. I think it's ready to go once it's updated.

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.

Sorry, one last thing. I think you'll want to enable BUSDEVICE explicitly on boards that had it frozen in but FULL_BUILD is 0. Otherwise they'll lose it.

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! Looks great!

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.

7 participants
0