hybridgroup/gobot

Tests, which using sysfs mocks are/can conflicting each other

gen2thomas opened this issue · 3 comments

While working on the driver for PCA9533 I have recognized, that there is a bug in i2c.WriteBlockData() for sysfs implementation. To show this in a unit test I tried to write one and recognized, that the tests in i2c_device_test.go are not independent (means can not be run in parallel or when running sequential are order dependant), because the usage of the same resource "sysfs.fs" and "sysfs.syscall", which is set to a mocked system. There are 2 problems with the current approach:

  • after setting the variable it is not reset after the test is finished (also affects tests when not running in parallel)
  • one test can set this variable(s) and another can use it (when running in parallel)

The first step will be to cleanup after each test by using a defer function. This ensures independently running for sequential call and do not affect the implementation itself. I'm currently working on it.

To ensure the tests can be running in parallel (without becomes flaky), some changes in implementation are needed.

Further findings:

  • do prevent "uintptr" as long as possible in general and the behavior is more restricted since 1.15, see https://go101.org/article/unsafe.html
  • ReadBlockData() and WriteBlockData() must not provide the pointer to the slice, but the pointer to the first data element
  • tinkerboard has bad timings for Read*Data (when using sysfs-ioctl), so it will not work with PCA9501 EEPROM directly (but it works fine with digispark)
  • we should consider using "i2c_smbus_*" methods from the Kernel, see https://www.kernel.org/doc/Documentation/i2c/dev-interface

After re-reading the section "Implementation details" of https://www.kernel.org/doc/Documentation/i2c/dev-interface this seems to be the important information: "Other ioctl() calls are converted to in-kernel function calls by i2c-dev. Examples include I2C_FUNCS, which queries the I2C adapter functionality using i2c.h:i2c_get_functionality(), and I2C_SMBUS, which performs an SMBus transaction using i2c-core-smbus.c:i2c_smbus_xfer()." and further "while i2c-core-smbus.c:i2c_smbus_xfer() calls either
adapter.algo->smbus_xfer() if it is implemented, or if not, i2c-core-smbus.c:i2c_smbus_xfer_emulated()"

When looking to the implementation of "i2c_smbus_xfer_emulated()", I can determine, that the case I2C_SMBUS_BLOCK_DATA not use the size parameter as size. Instead the first data value is used for the size in case of write.

This leads to the following message after filling the msg.buf[0]: command, size, data[0..n]

The command is the register address, the size seems to be the size of the following data block (because it is used to create the buffer by adding "2" for the command and the size element).

Additionally:
The usage of the suggested "i2c_smbus_*" functions would lead to call the same smbus_xfer() function finally. And we can see, that the data.block[0] is filled with the length, followed by the given values.

I will fix and retest the block functions with this information. Possibly the PCA9501 will work with that change. In addition I will add the information to the README.md.

part of release v2.0.0