search

eclipse / mraa

Linux Library for low speed IO Communication in C with bindings for C++, Python, Node.js & Java. Supports generic io platforms, as well as Intel Edison, Intel Joule, Raspberry Pi and many more.
http://mraa.io
MIT License
1.37k stars 613 forks source link

[RPi] spi.frequency(x) is not working above 500khz #255

Closed leonyuhanov closed 7 years ago

leonyuhanov commented 9 years ago

Ok I'm back, i know I'm annoying... Running MRAA on an RPi A+ in NodeJs MRAA Module Version: 'v0.7.3-20-gef28607' Node Version: v0.12.6 NPM Version: 2.11.2 I manualy compiled mraa on the board as per previous instructions and got it working. However I can not change the SPI frequency above 500Khz. I can put in any number i like into

var mraaModule = require('mraa'); var spiDevice = new mraaModule.Spi(0); spiDevice.frequency(4000000);

But the frequency sticks at 500khz. I can set it lower and can very that it changes on my scope. I can also verify that my RPi has a working SPI device as i have tested the pi-spi node module and i can run that at 8mhz with very little issue.

arfoll commented 9 years ago

Reporting bugs != annoying.

@michael-ring I'm away from my rpi, and since you're the expert can you have a look at this?

michael-ring commented 9 years ago

I will try my very best, will look at this in the evening

michael-ring commented 9 years ago

The problem is described very easily:

mraa_spi_frequency queries the driver about the maximum possible speed and allows only setting values up to this speed.

You should have seen a message: spi: Selected speed reduced to max allowed speed in syslog. The value that the driver returns is obviously rubbish as the raspberry can go a lot higher with the SPI clock. Commenting out the following code solves the issue:

// if (ioctl(dev->devfd, SPI_IOC_RD_MAX_SPEED_HZ, &speed) != -1) { // if (speed < hz) { // dev->clock = speed; // syslog(LOG_WARNING, "spi: Selected speed reduced to max allowed speed"); // }

I think I 'invented' this sanity check, arfoll, if you have no objections then I can create a patch to eliminate those lines.

ohne titel

arfoll commented 9 years ago

Actually the sanity check seems quite useful to me, let's keep it but just add an advance function hook to bypass it in the case of rpi. Also a bug should be filed to that SPI driver because it's obviously saying rubbish. Are you ok to do this?

michael-ring commented 9 years ago

Fine with me, I will provide a patch for what you described in the next days.

leonyuhanov commented 9 years ago

Awesome, no rush, im using a different spi node module for the moment, but would prefer to have mraa in the production version Will i need to recompile this on my RPi again from scratch?

arfoll commented 9 years ago

Sadly yes. However I'm planning on a making a new release soon and that will have NPM on arch fixed (it's fixed but not pushed on NPM due to no tag) so you'll be able to just do npm mraa on your rpi :)

On 19 August 2015 at 15:11, Leon notifications@github.com wrote:

Awesome, no rush, im using a different spi node module for the moment, but would prefer to have mraa in the production version Will i need to recompile this on my RPi again from scratch?

— Reply to this email directly or view it on GitHub https://github.com/intel-iot-devkit/mraa/issues/255#issuecomment-132612462 .

leonyuhanov commented 9 years ago

No stress

michael-ring commented 9 years ago

@arfoll: Hmm, new release... I realized tha my patch for 16bit length spi on raspberry is not merged yet, could you include the patch in the release before it gets bit rot?

arfoll commented 9 years ago

@michael-ring where is that patch? I tried to quickly find it but couldn't...

michael-ring commented 9 years ago

I meant this one: https://github.com/intel-iot-devkit/mraa/pull/153 It's about Soft-SPI

leonyuhanov commented 9 years ago

Guys I'm a bit new to github, how do i know when you have committed the new changes? WIll there be an post here?

tingleby commented 9 years ago

If in the commit message refers to "#255" in some way yes!

leonyuhanov commented 9 years ago

Hey team, i just realized that i could have just done this myself in my local copy. And that seems to have fixed this issue... I commented out the lines of code as sugested above, recompiled and now I'm running a higher frequency. It looks as though its capping at around 4-5mhz.. But that is good enough for me. Question: should i CLOSE this bug??

michael-ring commented 9 years ago

Nope, i will fix it correctly, did not find the time last weekend

leonyuhanov commented 8 years ago

@michael-ring Wondering when this will be resolved? I am still commenting out these lines pre compilation: // if (ioctl(dev->devfd, SPI_IOC_RD_MAX_SPEED_HZ, &speed) != -1) { // if (speed < hz) { // dev->clock = speed; // syslog(LOG_WARNING, "spi: Selected speed reduced to max allowed speed"); // }

alext-mkrs commented 8 years ago

@michael-ring, is there any chance you'll tackle this one?

leonyuhanov commented 8 years ago

Guys I am just commenting this out when I compile and it works just fine. Not sure why this is in there...

alext-mkrs commented 8 years ago

This is to make sure we use the actual max frequency - and the assumption is that driver has the ultimate knowledge. And apparently on rpi the driver is broken in this part, so there's a mechanism in mraa, which allows to implement a workaround.

leonyuhanov commented 8 years ago

I have been able to get 10mhz but anything higher seems the same(but only if i comment out these lines of code) Iether way,im happy to close this if its not worth looking into

leonyuhanov commented 8 years ago

I'm closing this issue. As stated above, this code depends on the RPI driver reporting correct information back to the MRAA module. It seems like this is not happening properly. To fix this, all you need to do is comment out:

// if (ioctl(dev->devfd, SPI_IOC_RD_MAX_SPEED_HZ, &speed) != -1) { // if (speed < hz) { // dev->clock = speed; // syslog(LOG_WARNING, "spi: Selected speed reduced to max allowed speed"); // }

alext-mkrs commented 8 years ago

Actually, it's worth it to be fixed properly, and that means exactly what @arfoll discussed with @michael-ring earlier. Namely, to implement a RPi-specific _replace function, which would make it work without commenting suff out and rebuilding + a bug should be filed for the driver to make it work properly.

That's why I pinged @michael-ring earlier, to see if he's still willing to do that. That's quite a simple fix I just don't have a RPi to test and prepare it myself.

alext-mkrs commented 8 years ago

Let me open this one back as I believe this should be fixed (worked around) properly in mraa to avoid user confusion.

@arfoll, feel free to override.

DavidYon commented 7 years ago

Am running into this bug as well, agree with @alext-mkrs and @arfoll, rather than having to comment this out, a simple replace function would fix it for RPi. That opens the door for the replace function being able accommodate changes with the out-of-spec RPi SPI driver over time.

I.e., if the driver's ioctl() was brought in line with the drivers on other embedded systems, the replace function could detect the driver/OS version and implement the appropriate algorithm.

I'd be happy to implement this if you'll accept a pull request. Seems pretty safe from a regression standpoint as:

  1. Whereever you want to place the blame, the fact remains that it's broken on RPi as it stands now.
  2. The fix is clean, using an existing platform-hook mechanism.
  3. It won't affect other plaforms.
arfoll commented 7 years ago

Sure, I totally agree - how about this fix 644064f883a47ab73010569d521031006076b8fe? If someone tests it and tells me it works I'll merge it.

skoehler commented 7 years ago

I just hit the same issue. Do I understand correctly, that the kernel driver is reporting a value which is much too low? Has somebody reported this issue to the guys at https://github.com/raspberrypi/linux?

leonyuhanov commented 7 years ago

@skoehler just coment out // if (ioctl(dev->devfd, SPI_IOC_RD_MAX_SPEED_HZ, &speed) != -1) { // if (speed < hz) { // dev->clock = speed; // syslog(LOG_WARNING, "spi: Selected speed reduced to max allowed speed"); // } And it will work!

skoehler commented 7 years ago

I will patch mraa tomorrow. I'll probably use the patch by arfoll, but my question remains: did anybody report this too the guys maintaining the Raspberry Kernel sources (see my link above)? Because that should be done. If nobody has, I will do it asap.

leonyuhanov commented 7 years ago

@skoehler i have not so feel free to do so. I stopped using mraa a long time ago in favor of rpis native spi C library

skoehler commented 7 years ago

FYI: they patched the device tree such that the maximum frequency reported by the driver is now 125kHz and not 500MHz anymore. On Raspbian, rpi-update kernel should give you the new device tree.