-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
SAMD51 SPI Secondary Mode #9385
base: main
Are you sure you want to change the base?
Conversation
…in and parameter passing has issues
…o adafruit-9.0.x Rebasing onto 9.0.3 (plus 4 commits) from 9.0.0-alpha, to get updates that fix asyncio library
…erted Metro M4 Express board configurations to original
…ld scripts to build firmware for rapid0 boards, updated peripherals submodule
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 the PR! A couple organization suggestions that will slim down this PR.
@@ -59,7 +59,7 @@ | |||
url = https://github.com/adafruit/Adafruit_CircuitPython_DotStar.git | |||
[submodule "ports/atmel-samd/peripherals"] | |||
path = ports/atmel-samd/peripherals | |||
url = https://github.com/adafruit/samd-peripherals.git | |||
url = https://github.com/Bruin-Spacecraft-Group/bruinspace-samd-peripherals.git |
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.
Please make a separate PR to update the Adafruit repo. We'll get that merged in before this PR.
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.
I added and sent in a PR, see adafruit/samd-peripherals#46
shared-bindings/busio/SPI.c
Outdated
//| half_duplex: bool = False, | ||
//| slave_mode: bool = False, |
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.
Please make a new module for this such as spitarget
(to match i2ctarget
). That will greatly reduce the impact of this change because existing constructors won't need to be changed.
We also prefer terminology that isn't historically loaded: https://docs.circuitpython.org/en/latest/docs/design_guide.html#terminology So, please don't use the terms master
or slave
in the interfaces. It is ok to document that they were previously called this.
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.
I've moved the secondary-mode behavior to a new spitarget module and fixed the naming, see https://github.com/Bruin-Spacecraft-Group/bruinspace-circuitpython/tree/main/shared-bindings/spitarget
shared-bindings/busio/SPI.c
Outdated
@@ -485,6 +638,14 @@ STATIC const mp_rom_map_elem_t busio_spi_locals_dict_table[] = { | |||
{ MP_ROM_QSTR(MP_QSTR_write), MP_ROM_PTR(&busio_spi_write_obj) }, | |||
{ MP_ROM_QSTR(MP_QSTR_write_readinto), MP_ROM_PTR(&busio_spi_write_readinto_obj) }, | |||
{ MP_ROM_QSTR(MP_QSTR_frequency), MP_ROM_PTR(&busio_spi_frequency_obj) } | |||
|
|||
#if CIRCUITPY_SAMD |
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.
shared-bindings
shouldn't have conditionalized APIs. Instead, the new module will have the new API and only be implemented on SAMD51.
Adafruit has been using "main" and "secondary" for the "M" and "S" names. I have also seen "sub" or "subnode". There was a proposal for "peripheral" and "controller", with "PICO" and "CIPO" pin names, but that has not been widely adopted. We aren't planning to rename "MOSI" and "MISO", so "main" and "secondary" are probably fine. |
Addresses #2131. Tested on a pair of custom breakout boards which were derived from the Metro M4 Express.