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

fix SPIDEV interface #315

Merged
merged 2 commits into from
Jan 4, 2017
Merged

fix SPIDEV interface #315

merged 2 commits into from
Jan 4, 2017

Conversation

plasticassius
Copy link
Contributor

fix several errors:
struct spi_ioc_transfer tr; was not initialized to zero.
initializer did not set the proper members.
cs_change = 0 causes incorrect csn behaviour.
using namespace std; can cause name conflicts in client code.

struct spi_ioc_transfer tr;
memset(&tr, 0, sizeof(tr));
tr.tx_buf = (unsigned long)&tx;
tr.rx_buf = (unsigned long)&tx;
Copy link

Choose a reason for hiding this comment

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

The use of tx for output and input buffers is innecessarily grubby - why not leave rx as a separate entity?


struct spi_ioc_transfer tr;
Copy link

Choose a reason for hiding this comment

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

It's polite to stick to the indentation style of the existing code, so you should be using tabs not spaces.

@plasticassius
Copy link
Contributor Author

This commit doesn't fix anything that was strictly wrong, but gets rid of some code that looks complicated and unnecessary. In particular, I removed many includes from the header since they weren't needed by the header. The includes needed in the C file are included there. This way, client code doesn't get all of those things. Admittedly, this could break client code that relied on those includes being there. I also moved member variables to stack variables, and removed the virtual declaration for the destructor. In the C code, I removed the use of the mutex since I couldn't see what purpose it served.

I don't think these changes will affect anyone since the code didn't work before the cs fix.

@pelwell I made the stylistic fixes you mentioned, but in the process I noticed that the previous code wasn't consistent in it's use of tabs vs spaces or whether lines were indented by 4 or 2 spaces. I also removed trailing whitespace from all lines that contained that since that's something that bugs me.

@plasticassius plasticassius merged commit e234ba7 into nRF24:master Jan 4, 2017
@Avamander Avamander requested review from Avamander and removed request for Avamander January 4, 2017 13:47
@razerraz
Copy link

razerraz commented Jan 11, 2017

Seems that this commit breaks ability to use spidev, at least on a beaglebone black
Before last upgrade, build 23/10/2016, all was working fine
Now I have :
from RF24 import *
radio = RF24(59,0)
radio.begin()
can't open device: No such file or directory
[1] 3709 abort (core dumped) python

with /dev/spidev0.0 - /dev/spidev0.1 enabled

EDIT : I confirm issue is related to this commit. I try to use spi.h and spi.cpp from lemariva fork, rebuild, and it's working as before

@Avamander
Copy link
Member

@razerraz The error is rather verbose. It can't find the interface. Make it print out the device it tries to open and see if the user you're executing the program has permissions to access it.

@razerraz
Copy link

I've tried it as root
The device is the one hardcoded in spi.cpp : /dev/spidev0.0
Tell me the way I can inspect things with more verbose output, I'm a python guy, don't expect me to find out by myself a way in this ugly (humble opinion) C++ language

@plasticassius
Copy link
Contributor Author

plasticassius commented Jan 12, 2017

@razerraz more information would help. I take it you are calling this from python, how? Can get debug output from the C code? The code in question is in spi.cpp:

`

char device[] = "/dev/spidev0.0";
device[11] += (busNo / 10) % 10;
device[13] += busNo % 10;
printf("busNo %d -> %s\n", busNo, device);

`
The printf I've put in there will show you the argument passed in to SPI::begin as well as the resulting device it is trying to open. Can you determine if the open is failing? or is it failing later (perhaps in the init function)? The key to finding that out is getting output from the C code.

Also, it could help to know what tools you are using for the build:
$(CXX) --version
In the make file would show this. For example, I am using:
arm-linux-gnueabihf-g++ (crosstool-NG linaro-1.13.1+bzr2650 - Linaro GCC 2014.03) 4.8.3 20140303

As a possible fix, would you try

`
static char _device[] = "/dev/spidev0.0";
char device[sizeof(_device)];
memcpy(device, _device, sizeof(_device));

`

in place of the line char device[] = "/dev/spidev0.0";

This spells out explicitly the initialization of the device variable, and may work better on your compiler.

@razerraz
Copy link

I'll try your fix & debug stuff this weekend. Feedback soon
Cheers

@razerraz
Copy link

So, seems it's time spend for nothing
I tried adding printf line, it was working fine.
Since it can't normally be a fix to add debug messages, I tried to have the same issue I had previously, looking for updated stuff in the toolchain that can be the fix, without success.
I don't understand from what comes the issue, maybe from an updated kernel on disk without reboot...
Sorry, forget it.
Regards

@plasticassius
Copy link
Contributor Author

I agree it's disconcerting when the diagnostic doesn't make any sense. However, if adding a function call (the printf) changes the operation of the code, I'd say that points to an optimization bug in the compiler. Since you've already spent the time to set up this test, would you try the fix I recommended before? In my experience, compiler bugs can be worked around by changing the form, but not the function of the code. I think that explicitly initializing the device variable as I suggested above is worth a try.

@razerraz
Copy link

I was not clear in my post : it's working now without any changes, building directly from the git repo.
So, even without your fix on the size of your device array...
By the way, I build from AUR pkgsrc (homemade) on an archlinux; so I don't have looked at the compiler options, using default makepkg behaviour.
When the bug occurs, It was in the middle on an big upgrade (python 3.6, new kernel...), I'm pretty sure it was the cause.
Regards & thanks for your interest

@plasticassius
Copy link
Contributor Author

Ok, thanks for clearing that up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants