arduino / ArduinoCore-megaavr

Arduino Core for the ATMEGA4809 CPU
103 stars 62 forks source link

This implementation conflicts with SMBus specification #139

Open tamebits opened 2 months ago

tamebits commented 2 months ago

Today I ran into an issue while implementing a PMBus interface on an Arduino every. PMBus derrives from SMBus which uses I2C and ultimately TWI.

From System Management Bus Specification Version 3.3:

image

This requires a 0 byte read or write transaction where the read is currently not possible with this implementation as the write bit is implicitly set in both cases.

Both cases result in a call to

uint8_t TWI_MasterWriteRead(uint8_t slave_address,
                         uint8_t *write_data,
                         uint8_t bytes_to_write,
                         uint8_t bytes_to_read,
                         uint8_t send_stop)

with bytes_to_write and bytes_to_read both being 0.

In this case the function always sets the write bit:

else if (master_bytesToWrite == 0 && master_bytesToRead == 0) {
            twi_mode = TWI_MODE_MASTER_TRANSMIT;
            uint8_t writeAddress = ADD_WRITE_BIT(master_slaveAddress);
            TWI0.MADDR = writeAddress;
        }

Am I missing something here? Or did somebody want to save a few lines, not having this special case in mind?

https://github.com/arduino/ArduinoCore-megaavr/blob/5e639ee40afa693354d3d056ba7fb795a8948c11/libraries/Wire/src/utility/twi.c#L322

facchinm commented 2 months ago

Hi @tamebits , that piece of code is tackling one shortcomings of Wire APIs, that the only way to scan the bus is the pattern

Wire.beginTransmission(address);
if (Wire.endTransmission() == 0) //device found

A solution would be to keep track of the user intention and just set the write bit in this particular use case. If you want to provide a PR I'll be glad to review it!