Pi4J / pi4j-v2

Pi4J Version 2.0
Apache License 2.0
280 stars 62 forks source link

Issue while reading SPI data #405

Open beulenbilly opened 3 weeks ago

beulenbilly commented 3 weeks ago

At the moment I am using the version 2.7.0 of your great library with raspberry pi and pigio.

My implementation reads the data from MCP3208:

ByteBuffer writeBuffer = ByteBuffer.wrap(data);
ByteBuffer readBuffer = ByteBuffer.allocate(writeBuffer.capacity());
final int bytesRead = spi.transfer(writeBuffer, readBuffer, writeBuffer.capacity());

If it was successful the logging looks like (three bytes sent, three bytes received):

2024-10-31 08:58:48,464 [Thread-6] TRACE d.b.rpi.mqtt.service.SpiAdcService - sending data for channel: 7 are 7, -64, 0 
2024-10-31 08:58:48,465 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketImpl - [SPI::XFER] -> [0]; Serial Transfer [3 bytes] 
2024-10-31 08:58:48,466 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketBase - [TX] -> CMD=SPIX(75); P1=0; P2=0; P3=3; PAYLOAD=[0x07 C0 00] 
2024-10-31 08:58:48,469 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketBase - [RX] <- CMD=SPIX(75); P1=0; P2=0; P3=3; PAYLOAD=[0x00 0C C5] 
2024-10-31 08:58:48,470 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketImpl - [SPI::XFER] <- HANDLE=0; SUCCESS=true; BYTES-READ=3 
2024-10-31 08:58:48,471 [Thread-6] TRACE d.b.rpi.mqtt.service.SpiAdcService - received data for channel: 7 are 0, 12, -59 

But something something different happens (first transfer return 0 bytes, second transfer return 6 bytes):

2024-10-31 08:58:49,473 [Thread-6] TRACE d.b.rpi.mqtt.service.SpiAdcService - sending data for channel: 7 are 7, -64, 0 
2024-10-31 08:58:49,474 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketImpl - [SPI::XFER] -> [0]; Serial Transfer [3 bytes] 
2024-10-31 08:58:49,475 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketBase - [TX] -> CMD=SPIX(75); P1=0; P2=0; P3=3; PAYLOAD=[0x07 C0 00] 
2024-10-31 08:58:49,477 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketBase - [RX] <- CMD=SPIX(75); P1=0; P2=0; P3=3; PAYLOAD=[0x] 
2024-10-31 08:58:49,478 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketImpl - [SPI::XFER] <- HANDLE=0; SUCCESS=true; BYTES-READ=0 
2024-10-31 08:58:49,479 [Thread-6] TRACE d.b.rpi.mqtt.service.SpiAdcService - received data for channel: 7 are 0, 0, 0 
2024-10-31 08:58:49,479 [Thread-6] DEBUG d.b.rpi.mqtt.service.SpiAdcService - received value is: 0 

2024-10-31 08:58:50,480 [Thread-6] TRACE d.b.rpi.mqtt.service.SpiAdcService - sending data for channel: 7 are 7, -64, 0 
2024-10-31 08:58:50,481 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketImpl - [SPI::XFER] -> [0]; Serial Transfer [3 bytes] 
2024-10-31 08:58:50,483 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketBase - [TX] -> CMD=SPIX(75); P1=0; P2=0; P3=3; PAYLOAD=[0x07 C0 00] 
2024-10-31 08:58:50,486 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketBase - [RX] <- CMD=UNKNOWN(-1); P1=0; P2=0; P3=6; PAYLOAD=[0x00 00 00 00 0C C5] 
2024-10-31 08:58:50,487 [Thread-6] TRACE c.p.l.pigpio.impl.PiGpioSocketImpl - [SPI::XFER] <- HANDLE=0; SUCCESS=true; BYTES-READ=6 
2024-10-31 08:58:50,488 [Thread-6] WARN  d.b.rpi.mqtt.service.SpiAdcService - could not read data java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 6 out of bounds for byte[3]
    at java.base/java.lang.System.arraycopy(Native Method)
    at com.pi4j.library.pigpio@2.7.0/com.pi4j.library.pigpio.impl.PiGpioSocketImpl.spiXfer(PiGpioSocketImpl.java:1477)
    at com.pi4j.plugin.pigpio@2.7.0/com.pi4j.plugin.pigpio.provider.spi.PiGpioSpi.transfer(PiGpioSpi.java:167)
    at com.pi4j@2.7.0/com.pi4j.io.spi.Spi.transfer(Spi.java:253)
    at com.pi4j@2.7.0/com.pi4j.io.spi.Spi.transfer(Spi.java:314)

At the moment I don't see any possibility to handle it in my implementation because I expect three bytes and the readBuffer.postion is three as well as spi.transfer returns three.

If I look into com.pi4j.library.pigpio.impl.PiGpioSocketImpl.spiXfer I wonder if int actual = rx.result(); is really correct or if it should be int actual = rx.dataLength();.

Is the there a relation to https://github.com/Pi4J/pi4j-v2/issues/16?

What do you think is the best solution to get this issue solved?

taartspi commented 2 weeks ago

I ordered this chip to see if I can duplicate your error. A couple things I will/would try with the chip.

  1. On the first error, some transfers do not return data. In the data sheet it mentions: If necessary, it is possible to bring CS low and clock in leading zeros on the DIN line before the start bit. This is often done when dealing with microcontroller-based SPI ports that must send 8 bits at a time. Refer to Section 6.1 “Using the MCP3204/3208 with Microcontroller (MCU) SPI Ports” for more details on using the MCP3204/3208 devices with hardware SPI ports.

I believe the pigpio spi always send 8 bits, have you tried as suggested ?

  1. Does the SPI frequency alter the behavior ? Section 6.2 must not exceed 1.2 ms (effective clock frequency of 10 kHz)

  2. Reading six bytes of data. Not sure on your question whether the code is incorrect. I need to dig and see if this condition could actually happen, you requested 3 bytes and get 6, and should be an error.

If you solve this please update the post, otherwise when I have a chip I will do some testing myself.

beulenbilly commented 2 weeks ago

Yes I always send 3 bytes and as far as I can see there no other option.

I altered baud rate but I couldn't see any difference.

I used pigpioj-java which is based on netty and it works without any problems. When I looked into the library there is wait for response implemented. This approach I applied into your code (just for testing):

com.pi4j.library.pigpio.PiGpioPacket:

switch (packet.cmd) {
            case I2CRI:
            case I2CRD:
                return packet.p3;
            case SPIX:
                while (stream.available() == 0) {
                    logger.info("have spi xfer without data ... waiting");
                    try {
                        Thread.sleep(50);
                    } catch (InterruptedException ex) {
                        ex.printStackTrace();
                    }
                }
            default:
                return stream.available();
        }

In my tests the change resulted in the data being able to be read.

Assuming this error is not caused by the MCP3208 but in the availability of the data from the socket's InputStream. In the PIGPIO documentation I couldn't find that an end character is used in communication, because I would use that to detect the end of the data together with a read timeout.

Do you know if there is end character?

taartspi commented 3 days ago

I do not know if there is an end character, and prefer we not inspect for a character to know the data was received. I was looking at your change that fixed your problem. Would you please paste your code where you create the SPI device, and when you call the transfer. Thanks much

beulenbilly commented 3 days ago

I used for my tests this implementation:

import com.pi4j.Pi4J;
import com.pi4j.context.Context;
import com.pi4j.io.spi.Spi;
import com.pi4j.io.spi.SpiProvider;
import com.pi4j.plugin.pigpio.provider.spi.PiGpioSpiProvider;
import java.nio.ByteBuffer;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Pi4jTest {

    private static final Logger logger = LoggerFactory.getLogger(Pi4jTest.class);

    public static void main(String[] args) throws InterruptedException {
        Context pi4JContext = Pi4J.newAutoContext();
        SpiProvider spiProvider = pi4JContext.provider(PiGpioSpiProvider.class);
        Spi spi = spiProvider.create(Spi.newConfigBuilder(pi4JContext)
                .id("my-spi-device")
                .name("My SPI Device")
                .address(0)
                .baud(2 * Spi.DEFAULT_BAUD)
                .build());

        final SpiReader spiReader = new SpiReader(spi);

        ExecutorService executor = Executors.newSingleThreadExecutor();//newFixedThreadPool(10);

        for (int i = 0; i < 60; i++) {
            executor.submit(new Callable<Object>() {
                @Override
                public Object call() throws Exception {
                    return spiReader.read(0);
                }
            });

            Thread.sleep(1000);
        }
        executor.shutdown();
        pi4JContext.shutdown();
    }

    private static class SpiReader {

        private final Spi spi;

        public SpiReader(Spi spi) {
            this.spi = spi;
        }

        public synchronized Object read(int channel) {
            byte[] rxBuffer = new byte[]{-1, -1, -1};
            ByteBuffer readBuffer = ByteBuffer.allocate(3);
            spi.transfer(createWriteBuffer(channel), readBuffer, 3);
            logData(readBuffer.array());
            return readBuffer;
        }

        private ByteBuffer createWriteBuffer(int channel) {
            return ByteBuffer.wrap(createWriteBytes(channel));
        }

        private byte[] createWriteBytes(int channel) {
            return new byte[]{
                (byte) (0b00000110 | ((channel & 0x0007) >> 2)), // first byte, start bit
                (byte) (((channel & 0x0007) << 6)), // second byte transmitted -> (SGL/DIF = 1, D2=D1=D0=0)
                (byte) 0b00000000 // third byte transmitted....don't care
            };
        }

        private void logData(byte[] result) {
            int value = ((result[1] & 0x0f) << 8);
            value |= (result[2] & 0xff);

            logger.info("received value: {}", (value & 0x0fff));
        }
    }

}

Some explanations: In my application I use threads to read the SPI values. To avoid any issues while reading the method is synchronized. If I remove these parts the errors are still there, sometimes just fewer.

import com.pi4j.Pi4J;
import com.pi4j.context.Context;
import com.pi4j.io.spi.Spi;
import com.pi4j.io.spi.SpiProvider;
import com.pi4j.plugin.pigpio.provider.spi.PiGpioSpiProvider;
import java.nio.ByteBuffer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class Pi4jTest {

    private static final Logger logger = LoggerFactory.getLogger(Pi4jTest.class);

    public static void main(String[] args) throws InterruptedException {
        Context pi4JContext = Pi4J.newAutoContext();
        SpiProvider spiProvider = pi4JContext.provider(PiGpioSpiProvider.class);
        Spi spi = spiProvider.create(Spi.newConfigBuilder(pi4JContext)
                .id("my-spi-device")
                .name("My SPI Device")
                .address(0)
                .baud(2 * Spi.DEFAULT_BAUD)
                .build());

        final SpiReader spiReader = new SpiReader(spi);

//      ExecutorService executor = Executors.newSingleThreadExecutor();

        for (int i = 0; i < 60; i++) {
//          executor.submit(new Callable<Object>() {
//              @Override
//              public Object call() throws Exception {
//                  return spiReader.read(0);
//              }
//          });
            spiReader.read(0);
            Thread.sleep(1000);
        }
//      executor.shutdown();
        pi4JContext.shutdown();
    }

    private static class SpiReader {

        private final Spi spi;

        public SpiReader(Spi spi) {
            this.spi = spi;
        }

        public /*synchronized*/ Object read(int channel) {
            byte[] rxBuffer = new byte[]{-1, -1, -1};
            ByteBuffer readBuffer = ByteBuffer.allocate(3);
            spi.transfer(createWriteBuffer(channel), readBuffer, 3);
            logData(readBuffer.array());
            return readBuffer;
        }

        private ByteBuffer createWriteBuffer(int channel) {
            return ByteBuffer.wrap(createWriteBytes(channel));
        }

        private byte[] createWriteBytes(int channel) {
            return new byte[]{
                (byte) (0b00000110 | ((channel & 0x0007) >> 2)), // first byte, start bit
                (byte) (((channel & 0x0007) << 6)), // second byte transmitted -> (SGL/DIF = 1, D2=D1=D0=0)
                (byte) 0b00000000 // third byte transmitted....don't care
            };
        }

        private void logData(byte[] result) {
            int value = ((result[1] & 0x0f) << 8);
            value |= (result[2] & 0xff);

            logger.info("received value: {}", (value & 0x0fff));
        }
    }

}
taartspi commented 2 days ago

You create the provider as spiProvider the upper interface class rather than pigpio-spi. I will need to run your code to validate but i suspect you follow a different path than my transfer. https://github.com/Pi4J/pi4j-example-devices/blob/master/src/main/java/com/pi4j/devices/mcp3008/MCP3008.java. I need to try your code before i have a suggestion. Maybe someone following this thread has time/ideas

beulenbilly commented 2 days ago

I think I didn't mention it: I run it with -Dpi4j.remote=true -Dpi4j.host=localhost -Dpi4j.port=8888

taartspi commented 6 hours ago

I mentioned your provider create using the parent class. My code does not enter that function you were modifying for a fix. I ran your code and the transfer runs spi class functions prior to the PigpioSpi class. My implementation directly enters the Pigpio classes. I would suggest you try using the provider create as pigpio-spi as I do in that example. Please let me know what happens. Tom