I considered adding a single "brzo_i2c_write_byte" function which took its immediate values directly from the stack instead of a buffer pointer to handle these cases of writing the register byte and/or a single byte or word to that register.

Something like:

// take the bytes to write directly from the stack.
function brzo_i2c_write(uint8_t no_of_bytes, ...) {
}

It's the same basic code, but in this case the buffer address is the stack pointer.

With all the assembly code sharing the same namespace, I was tempted to make "l_send_byte" a function that read and write both shared (via call0) instead of a separate labels (l_send_byte and l_send_byte_r) but I wasn't sure if the they did exactly the same thing or not. If they do exactly the same things, this would cut out a lot of the seemingly repeated code between the two functions, pick a hard coded register to use for the byte to send, ensure register a0 is available for the system to put the return address, and use "CALL0 l_send_byte;".

from brzo_i2c.

As for the code I wrote for write.

The instructions cost with the way I wrote this is one BBCI call per buffer byte.

And that's mostly because I did it in the least intrusive way I could see with the absolute minimum lines of code changed that I could manage.

First thing I did was extend the function spec of the write function with the assembly code to take the register address, and a boolean flag on whether or not that address should get sent. The read function was easier. Since it's changing read/write direction doing a write function call with a repeated_start, then a read call, was easier than modifying the assembly to mimic the same.

// Here's the brzo_i2c_read_addr call; not sure if it works in all cases... some things do not support repeated start like this....
void ICACHE_RAM_ATTR brzo_i2c_read_addr(uint8_t reg_addr, uint8_t *data, uint8_t nr_of_bytes, boolean repeated_start) {
    brzo_i2c_write(&reg_addr, 1, true);   // Hmm, perhaps repeated_start should be false here...
    brzo_i2c_read(data, nr_of_bytes, repeated_start);
}

// And the updated _write functions
void ICACHE_RAM_ATTR brzo_i2c_write_internal(uint8_t *data, uint8_t no_of_bytes, boolean repeated_start, boolean send_addr, uint8_t reg_addr);

void ICACHE_RAM_ATTR brzo_i2c_write(uint8_t *data, uint8_t no_of_bytes, boolean repeated_start) {
    brzo_i2c_write_internal(data, no_of_bytes, repeated_start, false, 0x00);
}

void ICACHE_RAM_ATTR brzo_i2c_write_addr(uint8_t reg_addr, uint8_t *data, uint8_t no_of_bytes, boolean repeated_start) {
    brzo_i2c_write_internal(data, no_of_bytes, repeated_start, true, reg_addr);
}

I then renamed "a_repeated" to "a_ctrl_flags" and used bit 0 for "send_reg_addr" and bit 1 for "repeated_start":

    uint32_t a_set, a_ctrl_flags, a_in_value, a_temp1, a_bit_index;
    a_ctrl_flags = 0;
    a_ctrl_flags |= (send_addr) ? 0x01 : 0x00;
    a_ctrl_flags |= (repeated_start) ? 0x02 : 0x00;

Then just before loading the next byte from the data array, I added this block of code to jump to loading the next byte in the data array if bit 0 in r_ctrl_flags is clear. Otherwise it loads the register address from the top of the stack, clears the send_reg_addr bit from the flags, and jumps to l_send_byte.

        "BBCI   %[r_ctrl_flags], 0, l_send_next_array_byte_w;"   // Branch to loading from array address if the register address does not need to be sent
        "L8UI   %[r_byte_to_send], sp, 0;"             // Load the register address byte from the top of the stack
        "ADDI.N %[r_ctrl_flags], %[r_ctrl_flags], -1;" // clear send_reg_addr bit; would prefer AND with an immediate; but don't seem to have that instrxn.
        "j l_send_byte;"

        "l_send_next_array_byte_w:"
        "BEQZ   %[r_no_of_bytes], l_send_stop;"    // Branch if there are no more Data Bytes to send

Then I updated the r_repeated test to look at bit 1 of r_ctrl_flags instead:

        // Check for a repeated start
        // Branch if bit 1 of r_ctrl_flags is 0, i.e. no repeated start, just send stop
        "BBCI %[r_ctrl_flags], 1, l_no_repeated_start;"
        //"BEQZ.N %[r_ctrl_flags], l_no_repeated_start;"

Lastly, I slightly modified the INPUT and OUTPUT lines for the asm call. To get around not having any additional registers, I replaced a_repeated with a_ctrl_flags, and made reg_addr accessible to memory. By putting reg_addr at the end of the list it seemed to make it available at the top of the stack:

                : [r_set] "+r" (a_set), [r_ctrl_flags] "+r" (a_ctrl_flags), ...
                : [r_sda_bitmask] "r" (sda_bitmask), ... , [m_reg_addr] "m" (reg_addr)
                : "memory"

Thanks

from brzo_i2c.

One of the questions has been why not/what's wrong with just always making a data buffer that is intended to go to a device with a couple bytes up front to store the register address?

Here's the reverse question; given the frequency with which register addresses are required to be sent in an i2c conversation, what's wrong with having an i2c function that can directly set them?

Why addressing information for the device and register should be kept separated from the data buffers is because addressing information isn't logically part of the data. Fetching data from location A, transforming it, then sending the data to location B is the typical flow. Locations A and B are not logically part of the data stream, they are addresses the data goes from and to.

If I was sending audio data to a device for hardware encoding, I wouldn't want to insert register addresses into the middle of the audio data stream buffers despite the fact the i2c protocol requires it (I wouldn't want to insert device addresses either; and the protocol requires those too).

I think you can also see that my options for doing so are limited; I can replace part of an audio sample; I can send part of the data stream, then send a separate buffer with just the register address, then resume sending the audio buffer; or I can make a dedicated "send_buffer", put in the register address, copy the audio data into the bufer, then send that special "send_buffer"...

What I'd want is to read audio data from the microphone source into an audio_buffer; then request that audio_buffer data be sent to the appropriate register on the device. I can send X bytes per write call.
Then issue consecutive read requests from the appropriate register pointing the incoming data to another encoded_buffer where I expect the encoded data to end up (and it'd be convenient if the write request (for the address) and the consecutive read request were handled transparently by the function).

Here's a different kind of example from code I'm actually working with right now.

    // Power up the MPU6XXX/MPU9XXX
    devId = 0xFE;
    readBytes(MPU9250_ADDRESS_AD0_HIGH, MPU9250_RA_WHO_AM_I, true, 1, &devId);
    Serial.print("Read MPU0x69 Device ID: 0x"); Serial.println(devId, HEX);
    if (devId < 0x68 || 0x7C < devId) return 0; // If we didn't read a valid device id, stop

    writeByte(MPU9250_ADDRESS_AD0_HIGH, MPU9250_RA_PWR_MGMT_1, true, 1<<MPU9250_PWR1_SLEEP_BIT | MPU9250_CLOCK_PLL_XGYRO);       // Power down 
    writeByte(MPU9250_ADDRESS_AD0_HIGH, MPU9250_RA_GYRO_CONFIG, true, (MPU9250_GYRO_FS_250 << MPU9250_GCONFIG_FS_SEL_BIT) | 3);  // 250dps and DLPF Rate = 4kHz
    writeByte(MPU9250_ADDRESS_AD0_HIGH, MPU9250_RA_ACCEL_CONFIG, true, (MPU9250_ACCEL_FS_2 << MPU9250_ACONFIG_AFS_SEL_BIT) );    // 2G 
    writeByte(MPU9250_ADDRESS_AD0_HIGH, MPU9250_RA_PWR_MGMT_1, true, MPU9250_CLOCK_PLL_XGYRO);  // Power up again

Those calls all use #define values for the device address, register address, and most of the value parameters. The "true" is for sending the register address as part of the read/write instruction.
The functions take immediate uint8_t values directly. There is/are no buffers.
Even the read call is reading a single byte back to the address of a uint8_t variable.

It'd really change the structure of the code to rewrite them to work directly through buffers and it's not clear to me that it would be a good thing to changeover to. What I'd do/have done is write functions that take these calls and wrap the parameters in a 2 byte buffer because that's what brzo expects. (Which is also what prompted the idea for the assembly based function that directly took no_of_bytes and a ... va_list as parameters.)

While the i2c protocol itself is about reading/writing a byte stream; programmers using the i2c APIs are mostly interacting with devices and their settings. It's like networking; "send this byte(s) to this address" or "read this byte(s) from this address". Putting the address into the data stream is the equivalent of saying "Why not just reserve some bytes in front of all the data arrays to put in the destination IP address and port?".

Because even though the IP address and port information do go over the wire as part of every data packet sent (as does a MAC address in most cases), these addresses are not part of the data; they are part of the infrastructure the data is flowing through.

The Device ID and Register Address in I2C are analogous to the IP Address and Port concept; they are part of the infrastructure where data is flowing to/from, not part of the actual data that is flowing.

Now I'm all for paradigm shifts, so if these examples don't highlight the distinction regarding why register addresses aren't part of the programmer's data stream, then I'm good with you standing by your guns. :)

The ideas of the device controlling the conversation speed instead of the "bus" and the concept of i2c_txns have both been inspiring to me on what/how I think I2C can look. So I like very much that you're thinking differently and that the library is being kept focused.

Thanks
Mike

from brzo_i2c.