Add r/w functions to take a separate uint8_t register address

[MikeFair]

In #13 it came up about keeping the library clean by avoiding a whole host of wrapper functions that more complex libraries provide.

While I agree with keeping it clean and minimal; I disagree those extra commands clutter a usable and useful I2C library. Useful work with I2C devices requires flipping bits without altering the rest of the register, changing endianness on Word read/writes, and most of those other provided functions.

In fact, when you look at the implementation of those extra commands, they don't actually do any bus work. They keep using the same couple of read/write functions to do the actual Wire work. So you could take any of those libraries, replace a few functions with the brzo functions and get the full functionality of the library without adding any functions to brzo at all.

And that's what I recommend as a target for brzo. Being a useful bit banging on the bus part for Wire and libraries like Wire. The Wire library (and others) already support using different underlying bit banging methods, so making brzo compatible with them in that sense (to be used as the bit banging method underlying the library) is where I think brzo (and other platform/MCU specific libraries) belong.

So how does this relate to optionally providing register addresses separate from the data? (And functions like writeByte that don't take a buffer address but instead just take a direct value..)

Unlike bit flipping and byte ordering, sending the register address is something that happens on the wire and is required in many, if not most, command circumstances.

The way most code deals with the register and device addresses keeps those items in separate variables, or even in #define statements which makes is extremely cumbersome to always put at address 0 of the data buffer.

Having #define statements with values from the product sheets, or hand crafted for the various modes of the device can save a lot of code space and pointer dereference requirements by hardcoding those immediate values straight into the function calls.

The compiler optimizer might even find ways to rearrange that code to make it even more efficient. When everything is always wrapped behind a pointer dereference, there is fewer opportunities for those optimizations. The Tensilica assembly can even use smaller commands in some cases when it knows it's dealing with immediate values.

And those arguments are secondary to the primary argument which is how programmers actually work with I2C devices. They have data buffers where the data to send to the device and read from the device get stored, then they have #define and const uint8_t and other mechanisms to store the device and register addresses that those data buffers belong too.

So it's just plain inconvenient to rearrange my data buffers to force a #define value into data_buffer[0] when I could have more easily placed it as its own value in the function call. To the brzo function it just means sending a byte from a separate location before sending the data at the pointer location. It's exactly the same idea as the device Id. It's not the "data" it's about the "where/what" to send/read the data to/from.

I've already done the work for this with the write function. I'll post my changes in the next comment and you can tell me if you think this is keeping with the spirit of a clean/lean i2c bit banging machine. :)

[MikeFair]

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;".

[MikeFair]

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

[MikeFair]

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