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

Rework SPI to detach it allowing reuse by several devices #1637

Merged
merged 36 commits into from
Sep 11, 2020
Merged

Rework SPI to detach it allowing reuse by several devices #1637

merged 36 commits into from
Sep 11, 2020

Conversation

AdrianSoundy
Copy link
Member

@AdrianSoundy AdrianSoundy commented May 18, 2020

Description, Motivation and Context

Changes to split out the SPI device layer to separate files to allow for shared use from multiple internal devices like Display, Spi Ethernet

This code needs to be reviewed before before any other changes are made in case things need to be changed.

The Windows.device.Spi is now common to all devices and moved to /src tree
The device layer has been split into the PAL & HAL files where the HAL only needs to ported to new devices.

PAL - nanoSpi.cpp / .h
HAL - CPU_SPI.cpp / CPU_SPI_decl.h

TI-SimpleLink - Changes done but noticed the SPI is a slave implementation and doesn't have any support for chip select. Not sure if it was working before.

Following nanoframework/Windows.Devices.Spi#77.

Other things to do

Modify the cmake config so you can enable the SPI without including the Windows.device.Spi
So that other devices can enable it.

How Has This Been Tested?

Some testing done with ESP32 but other platform untested, not sure if they even build

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Signed-off-by: adriansoundy adriansoundy@gmail.com

@josesimoes
Copy link
Member

Going through this!

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

Looks good. But would like to test it. Can you please start the PR to the SPI C# library to be able to do it?

A few questions: why do we need to have the option for the SPI operation to be async or not? Unless there is a good reason for not having it, I believe it should be async. Plus is seems that this mixes with CLR events for long running operations...

CMakeSettings.json Outdated Show resolved Hide resolved
src/Windows.Devices.Spi/win_dev_spi_native.cpp.org Outdated Show resolved Hide resolved
src/Windows.Devices.Spi/win_dev_spi_native.h.org Outdated Show resolved Hide resolved
@josesimoes josesimoes force-pushed the develop branch 11 times, most recently from b714673 to b7023a7 Compare June 12, 2020 12:07
@josesimoes josesimoes force-pushed the develop branch 4 times, most recently from d5f7bc6 to e56d843 Compare June 24, 2020 09:01
@nfbot
Copy link
Member

nfbot commented Aug 23, 2020

@AdrianSoundy there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please review and merge the changes. See https://github.com/AdrianSoundy/nf-interpreter/pull/7.

Make sure you are using the project code style. Check the details here.

@nfbot
Copy link
Member

nfbot commented Aug 28, 2020

@AdrianSoundy there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please review and merge the changes. See https://github.com/AdrianSoundy/nf-interpreter/pull/8.

Make sure you are using the project code style. Check the details here.

Automated fixes for code style.
@nfbot
Copy link
Member

nfbot commented Aug 28, 2020

@AdrianSoundy there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please review and merge the changes. See https://github.com/AdrianSoundy/nf-interpreter/pull/9.

Make sure you are using the project code style. Check the details here.

@AdrianSoundy
Copy link
Member Author

AdrianSoundy commented Aug 28, 2020

Think i am there now. Managed code is also committed.
Tested with ESP32 OK
Need to test with other platforms. Will do testing once i get the Discovery board.

I have one issue that i would like your thoughts on.
Sharing the Spi bus with an internal device or 2 internal devices. How do we manage 2 devices accessing the same SPI bus at same time. Its ok for managed code threads as there are lock objects for that.
With Enc28j60 driver it accesses bus in a continuation call. What if we want LCD display and Ethernet on same bus.

@AdrianSoundy AdrianSoundy marked this pull request as ready for review August 28, 2020 11:02
@josesimoes josesimoes changed the title Spi update Rework SPI to detach it allowing reuse by several devices Sep 7, 2020
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM

@AdrianSoundy
Copy link
Member Author

Tested with an SPI LCD display on both STM32F769 + ESP32

@nfbot
Copy link
Member

nfbot commented Sep 8, 2020

@AdrianSoundy there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please review and merge the changes. See https://github.com/AdrianSoundy/nf-interpreter/pull/10.

Make sure you are using the project code style. Check the details here.

Automated fixes for code style.
@nfbot
Copy link
Member

nfbot commented Sep 8, 2020

@AdrianSoundy there are issues with the code style on the source files.
A PR was submitted with the code style fixes. Please review and merge the changes. See https://github.com/AdrianSoundy/nf-interpreter/pull/11.

Make sure you are using the project code style. Check the details here.

@AdrianSoundy AdrianSoundy merged commit 9ce0284 into nanoframework:develop Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants