Are pi4j-v2 provides thread-safe?

[tatery]

Hello,

I wonder if current pi4j v2 providers implementations are thread-safe. I'm working on a GPIO provider based on MCP23017 (please see: #138) and because I'd like to use it in a multi-threading application I searched for an example of how currently available providers are synchronized. Unfortunately, I could not find any synchronized method. Does this mean that it is not recommended to use the new pi4j in multi-threading applications?

In case of using MCP23017 to build GPIO provider, accessing the same i2c device at the same time from different threads is very likely. However, other providers seem to be not synchronized - maybe currently available providers don't really need to be synchronized, but my provider does?

[taartspi]

Executing the MCP23017 example, there is a lock synchronization for the i2c path in the LinuxFSI2CBus, so I believe this multiThread concern is addressed at present. public R execute(I2C i2c, CheckedFunction<LinuxFile, R> action) {
if (i2c == null) {
throw new NullPointerException("Parameter 'i2c' is mandatory!");
} else if (action == null) {
throw new NullPointerException("Parameter 'action' is mandatory!");
} else {
try {
if (!this.lock.tryLock() && !this.lock.tryLock(this.lockAquireTimeout, this.lockAquireTimeoutUnit)) {

[tatery]

@taartspi thanks a lot. I have not noticed that.

[eitch]

Hi @tatery, @taartspi is correct, I2C is synchronized. But beware that each call is synchronized, so if you need to do multiple calls, then be sure to use the execute method for your entire synchronized block.

So if you look at the example on https://pi4j.com/documentation/io-examples/i2c/ you will see that there are multiple calls on the I2C object. Each of these calls are synchronized, but a different thread could write in between two writeRegister() calls.

[savageautomate]

@tatery

This impl file would probably benefit from some sync blocks around the cases where there are multiple calls to the I2C bus for expected "atomic" operation.

• https://github.com/savageautomate/pi4j-plugin-microchip/blob/main/src/main/java/com/pi4j/plugin/microchip/mcp23017/internal/MCP23017Device.java

The actual risk is probably low assuming consumer code does not attempt to reconfigure the same I/O pin on the same chip in different threads. Not sure that is even possible with the current Pi4J APIs.

[savageautomate]

I have added a few sync blocks on the MCP23017 impl.

    public MCP23017Device(I2C i2c){
        // set local reference to I2C instance
        this.i2c = i2c;

        // atomic operation to configure chip registers
        synchronized (this.i2c) {                           <<--------- LOCKING ON I2C INSTANCE
            // set all default pin interrupt default values
            this.i2c.writeRegister(REGISTER_DEFVAL_A, (byte) 0x00);
            this.i2c.writeRegister(REGISTER_DEFVAL_B, (byte) 0x00);

            // set all default pin interrupt comparison behaviors
            this.i2c.writeRegister(REGISTER_INTCON_A, (byte) 0x00);
            this.i2c.writeRegister(REGISTER_INTCON_B, (byte) 0x00);
        }
    }

@eitch

Do you think using the I2C instance is the appropriate object to sync lock on?
This I2C instance is created by the end user and represents a unique I2C bus and device address (i.e. single chip) (within the Pi4J context/runtime). So locking on this object should prevent other operations on this specific chip while in the sync block.

Thanks, Robert

[taartspi]

Robert's change protects app while it runs, but still between invocations another app could reconfig the pin. So, the device code should validate the pin is still configured for the proper direction. I did this in my old example but I also logged a failure thru my own ffdc (log4j) implementation, overkill for this example. But I do think we should log somehow when the pin is no properly configured. At present is slf4j or the console the preferred manner ?

[savageautomate]

@taartspi

.... But I do think we should log somehow when the pin is no properly configured. At present is slf4j or the console the preferred manner ?

For any Pi4J core code or plugins/providers, SLF4J would be the preferred logging mechanism.
I don't think we directly write to the console anywhere except maybe sample code or in lower level native C code.

[eitch]

I have added a few sync blocks on the MCP23017 impl.

    public MCP23017Device(I2C i2c){
        // set local reference to I2C instance
        this.i2c = i2c;

        // atomic operation to configure chip registers
        synchronized (this.i2c) {                           <<--------- LOCKING ON I2C INSTANCE
            // set all default pin interrupt default values
            this.i2c.writeRegister(REGISTER_DEFVAL_A, (byte) 0x00);
            this.i2c.writeRegister(REGISTER_DEFVAL_B, (byte) 0x00);

            // set all default pin interrupt comparison behaviors
            this.i2c.writeRegister(REGISTER_INTCON_A, (byte) 0x00);
            this.i2c.writeRegister(REGISTER_INTCON_B, (byte) 0x00);
        }
    }

@eitch

Do you think using the I2C instance is the appropriate object to sync lock on? This I2C instance is created by the end user and represents a unique I2C bus and device address (i.e. single chip) (within the Pi4J context/runtime). So locking on this object should prevent other operations on this specific chip while in the sync block.

Thanks, Robert

No these synchronizations won't help. This synchronization only stops two thread from constructing this instance... which is moot as it would be a new instance anyhow...

No, what we need is something like v1 had, where we can execute something on the I2C bus:

    return this.i2CBus.execute(this, file -> {
        file.write(b);
        return 1;
    });

We have this inside the implementations of the I2C providers, e.g. the above code is from the LinuxFsI2C class, but there is, at least from what i can see, no way for a user to perform such a blocking action on the bus.

Basically this above synchronized call might not even need to be synchronized, as it only writes a byte to the stream. But of course any read-then-write methods would need it.

And from a user perspective it might be important to be able to synchronize the communication with the I2C bus completely, not just a single I2C instance, which is an address, not the entire bus, which probably would still need synchronization.

Or am i missing something here?

[savageautomate]

@taartspi

Not sure I follow .... Anytime Pi4J creates the Digital input/output IO object (on the MCPxxxx pin) it should be properly configuring the pin direction and interrupt config. (If it's not properly configuring the pin each time, then that is a bug.)

SEE:

• https://github.com/savageautomate/pi4j-plugin-microchip/blob/main/src/main/java/com/pi4j/plugin/microchip/mcp23017/internal/MCP23017Device.java#L244-L278

Thanks, Robert

[tatery]

@savageautomate @eitch @taartspi many thanks for great inputs.