swharden / FtdiSharp

A .NET library for interfacing FTDI USB controller ICs
https://www.nuget.org/packages/FtdiSharp/
MIT License
27 stars 5 forks source link

Array.Copy() destination index of tx.length causes exception #9

Closed sdunc closed 3 days ago

sdunc commented 3 days ago

https://github.com/swharden/FtdiSharp/blob/146cfb80db8a3280a8bbae866464d098c1075aa9/src/FtdiSharp/Protocols/SPI.cs#L172

swharden commented 3 days ago

Hi @sdunc, I don't understand this question.

Are you suggesting that line could be changed to improve behavior of this app?

https://github.com/swharden/FtdiSharp/blob/146cfb80db8a3280a8bbae866464d098c1075aa9/src/FtdiSharp/Protocols/SPI.cs#L163-L181

sdunc commented 3 days ago

Hey Scott, thanks for the quick reply. Sorry for lack of clarity.

Calling ReadWrite with tx.Length + tx.Length > shiftOut.Length leads to exception. Minimal example:

byte[] ReadWrite(byte[] tx) 
 { 
     byte[] shiftOut = new byte[tx.Length + 3]; 
     shiftOut[0] =(byte)0x31;
     shiftOut[1] = (byte)tx.Length; 
     shiftOut[2] = (byte)(tx.Length >> 8); 
     Array.Copy(tx, 0, shiftOut, tx.Length, tx.Length); 
     byte[] rx = new byte[tx.Length]; 
     return rx; 
 } 
byte[] tx = [0x00, 0x00, 0x00, 0x00];
ReadWrite(tx);

Am I missing something here?

Is the fix:

Array.Copy(tx, 0, shiftOut, 3, tx.Length);
swharden commented 3 days ago

For my records, here are the Array.Copy() parameters for this overload...

https://learn.microsoft.com/en-us/dotnet/api/system.array.copy?view=net-8.0

Array.Copy(sourceArray, sourceIndex, destinationArray, destinationIndex, length);

So, yeah, it looks like destinationIndex should be 3 indeed!

I suspect this was overlooked because the demo SPI apps I made only read from sensors and don't appear to write bytes to them, so this method may have been inadequately tested.

I can make that change and release a new version here in the next few minutes! I'm interested to hear if changing that value to 3 makes it work as expected for you and the gear you're working with.

swharden commented 3 days ago

this method may have been inadequately tested

It actually was tested here, but by coincidence the transmission packet was 3 bytes long so it worked by accident in my benchtop tests 😅

https://github.com/swharden/FtdiSharp/blob/146cfb80db8a3280a8bbae866464d098c1075aa9/src/FtdiSharpDemo/SPI_MCP3008.cs#L36-L43

swharden commented 3 days ago

I can make that change and release a new version here in the next few minutes! I'm interested to hear if changing that value to 3 makes it work as expected for you and the gear you're working with.

I just pushed FtdiSharp 0.1.0 to NuGet. If you give it a go, I'd love to hear how it did in your hands! I don't have a test bench set up, so your feedback is really useful 😄

https://www.nuget.org/packages/FtdiSharp/0.1.0