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

Add sanity check for SPI dataBitLength #1391

Merged
merged 1 commit into from
Jul 2, 2019
Merged

Add sanity check for SPI dataBitLength #1391

merged 1 commit into from
Jul 2, 2019

Conversation

AdrianSoundy
Copy link
Member

@AdrianSoundy AdrianSoundy commented Jul 1, 2019

Description

Add sanity check to dataBitLength.
If zero seems to dump out memory.

Fixes nanoframework/Home#497

How Has This Been Tested?

Run provided test code.

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

@nfbot
Copy link
Member

nfbot commented Jul 1, 2019

Hi @AdrianSoundy,

I'm nanoFramework bot.
Thank you for your contribution!

A human will be reviewing it shortly. 😉

@AdrianSoundy AdrianSoundy merged commit 7faed7c into nanoframework:develop Jul 2, 2019
@AdrianSoundy AdrianSoundy deleted the spi_check branch July 2, 2019 00:51
@josesimoes josesimoes added Series: ESP32 Everything related specifically with ESP32 series targets Type: bug labels Jul 2, 2019
@@ -254,6 +254,7 @@ HRESULT Library_win_dev_spi_native_Windows_Devices_Spi_SpiDevice::NativeTransfer

// get data bit length
int databitLength = pConfig[ SpiConnectionSettings::FIELD___databitLength ].NumericByRef().s4;
if (databitLength <= 0) databitLength = 8;
Copy link
Member

Choose a reason for hiding this comment

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

@AdrianSoundy wouldn't be better to throw a bad argument exception instead of "fixing" the value?
I mean, as it is it works, of course, but the developer is not aware that he has made a mistake or set it to a wrong value.

That's the usual procedure throughout the code base and it's done in a number of places.

From what I could found (I've only worked with 8 and 16 bits) but the data can be from 8 to 16. I don't know if ESP32 can handle anything in the between...

So I suggest that the check is made for a valid number >= 8 and <= 16 (or just 8 and 16 to keep it simple) and throw an exception on invalid argument. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Series: ESP32 Everything related specifically with ESP32 series targets Type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spi.write on ESP32 writes erronous data to the bus
3 participants