doceme / py-spidev

MIT License
461 stars 203 forks source link

silent failure in python when using xfer() and xfer2() #40

Closed prygaard closed 8 years ago

prygaard commented 8 years ago

I recently tried running this code: import sys import spidev spi_bus = spidev.SpiDev() exit_code = spi_bus.open(0, 0) print('getting the adc output ') adc_output = spi_bus.xfer2(1, 240, 0 ) print('adc read complete')

The code would get to the xfer2() and the just stop and return to the prompt with no error or warning.

The bug in the above code is that there should have been brackets around the list passed to xfer2() as shown below.

import sys import spidev spi_bus = spidev.SpiDev() exit_code = spi_bus.open(0, 0) print('getting the adc output ') adc_output = spi_bus.xfer2([1, 240, 0] ) print('adc read complete')

Ideally SpiDev.xfer2() would have generated some type of error or warning rather than have the whole program just stop without an error message.

Note: I am a newbee so I do not even know if it is possible to do what I am suggesting. However, if you can, it would sure save the next person to hit the problem a lot of effort.

prygaard commented 8 years ago

Hmmm. My code did not format well in my post. Here is another try:

I recently tried running this incorrect python 3 code with spidev:

 import sys
 import spidev
 spi_bus = spidev.SpiDev()
 exit_code = spi_bus.open(0, 0)
 print('getting the adc output ')
 adc_output = spi_bus.xfer2(1, 240, 0 )
 print('adc read complete')  

The code would get to the xfer2() and then just stop and return to the prompt with no error or warning. (The last print statement would never execute)

The bug in the above code is that there should have been brackets around the list passed to xfer2() as shown below.

 import sys
 import spidev
 spi_bus = spidev.SpiDev()
 exit_code = spi_bus.open(0, 0)
 print('getting the adc output ')
 adc_output = spi_bus.xfer2([1, 240, 0] )
 print('adc read complete') 

Ideally SpiDev.xfer2() would have generated some type of error or warning rather than have the whole program just stop and return to the prompt without an error message.

Note: I am a beginner so I do not even know if it is possible to do what I am suggesting. However, if you can, it would sure save the next person to hit the problem a lot of effort.

semininja commented 8 years ago

I agree, I would have expected a TypeError either for too many arguments or arguments not being lists. Just out of curiosity, does it work with tuples?

prygaard commented 8 years ago

I won't be able to get to it for a couple days, but my guess is it won't. I had tried this: xfer2(0) and it did not work. The readme documentation says the parms are:

xfer2(list of values[, speed_hz, delay_usec, bits_per_word])

I think the first parameter must be a list with the '[ ]' brackets. When I get a chance I will play with it some more.

I tried looking at the wrapper code

prygaard commented 8 years ago

It seems to me that no code should abort and not provide some kind of error or warning.

I tried looking at the C wrapper code but quickly got lost in the magic of python calling c code. Someone more experienced than I will be needed to figure this out.

doceme commented 8 years ago

I agree that it should not fail silently. I'll try to take a look at it soon.

doceme commented 8 years ago

Please check the latest code and confirm that TypeError is raised when specifying a non-list type.

prygaard commented 8 years ago

That worked like a charm!

this statement works fine: adc_output = self.spi_bus.xfer2([1, 240, 0])

This statement fails gracefully with an error message adc_output = self.spi_bus.xfer2(1, 240, 0)

The error message is: TypeError: must be list, not int

Thank you for fixing this!! I am sure it will help the next guy to hit the problem a lot.

I am new to all of this. Is there something that needs to be done to get the fix into other branches of spidev such as (https://pypi.python.org/pypi/spidev/3.0)

doceme commented 8 years ago

No problem, thanks for pointing it out! I just need to test a few more things on my RPi and Beaglebone to confirm that the latest commits haven't broken anything. Then, I'll make a new PyPi release that includes this fix.

prygaard commented 8 years ago

Thanks!! 

doceme commented 8 years ago

Done: https://pypi.python.org/pypi/spidev/3.2

prygaard commented 8 years ago

Wow! That was fast. Thanks!!

doceme commented 8 years ago

Looks like I should have used PySequence_* functions to handle this. I created a pull request on the fix-list branch to address this. Would anyone care to test it out before I merge?