brzo_i2c_write: allow subsequent calls #13

valkuc · 2016-10-05T16:33:59Z

One thing that makes library hard to use for EEPROM write operations.
Write operation to EEPROM is made by two steps: first, send address; second, send data to write. Address and Data must sent as single one array. The tricky part here is that if I will use two separate calls to brzo_i2c_write I will have additional "Setup write" before address and data - and this is error. Address and Data must be sent one after another, so imagine that I have next function prototype to write data to EEPROM:
bool at24c_write(uint16_t addr, uint8_t* data, uint8_t len)
I can't write body like that because in this case I will have extra "Setup write" between address and data transmission:

uint8_t data_addr[2];
data_addr[0] = (uint8_t)(((unsigned)addr) >> 8);
data_addr[1] = (uint8_t)(((unsigned)addr) & 0xff);
brzo_i2c_write(data_addr, 2, true);
brzo_i2c_write(data, len, false);

To overrun this I need to malloc temporary array with size of data + 2, copy first two bytes of address into it, then copy data and after that send it in one brzo_i2c_write call. I guess this looks like redundant memory allocation:

uint8 *d = os_malloc(len + 2);
d[0] = (uint8)(((unsigned)addr) >> 8);
d[1] = (uint8)(((unsigned)addr) & 0xff);
os_memcpy(d+2, data, len);
brzo_i2c_write(d, len+2, false);
os_free(d);

Here is some ideas:

Move device address transmission to separate function (maybe in start_transaction?) or...
Introduce bool argument to brzo_i2c_write function to control send or not to send device address.

The text was updated successfully, but these errors were encountered:

MikeFair · 2016-11-03T23:53:18Z

I had this same problem, and to skirt the issue as a quick hack I did the same thing. I added a function to the library that took a register address as the first parameter, then allocated a new array that was 1 byte larger, copied the register address to the first element, then copied the rest of the data array into this new array and then called brzo_i2c_write with the new array.

For read I hacked it by taking reg_addr and stuffing it into data[0] before calling the brzo_read; which is what the caller seemed to be expected to do in the first place.

Though, I always knew fixing the body of the read/write code is what needed to happen.

So here's a first cut with code that almost works (tries to use too many registers atm).

The code for brzo_i2c_write first preloads the device address into a register called "%[r_byte_to_send]"; it then sends this byte; then if the "%[r_no_of_bytes]" counter hasn't reached 0 yet, it loads the next element from the data array into "%[r_byte_to_send]", increments the array pointer , decrements the counter, and directly jumps to "l_send_byte".

You can find this code just before "l_send_stop:" (after "l_slave_ack:") ~line 225 - 235.

Modifying the code here and adding a flag can optionally send the register address too.

Here's my first take on it and it's almost working; like I said I ran out of registers to directly map the register address and I'm not sure how to look it up from the stack; and a little bit of modification to change the r_repeated register into a bitfield that tracks multiple boolean flags.
(That could free up a couple more registers (like clock high/low) removing the stack access need.)

Here's some psuedo code of the basic idea:

    // Before loading from the array, add code to jump to a new label "l_send_next_array_byte" 
    if "%[r_send_addr]" equals "0" then jump to label "l_send_next_array_byte".
    // (I would prefer a bit flag for this, suggest changing [r_repeated] to a bitfield of flags (pasko-zh?)).  

    Set "%[r_byte_to_send]" to "%[r_reg_addr]";   // Or load reg_addr from the stack
    Set "%[r_send_addr]" to "0"
    Jump to "l_send_byte"

    Label "l_send_next_array_byte"

After the first time through the code, "%[r_send_addr]" will be 0 and it will jump over the register address load/send and behave just like it has before from then on.

Here is the code snippet:

add this 6 line section above the code that loads the data array.
(Between the "l_slave_ack:" and "l_send_stop:" labels and above the "BEQZ %[r_no_of_bytes, ...]" line.
~line 225:

        "BEQZ   %[r_send_addr], l_send_next_array_byte;"   // Branch to array address if the register address does not need to be sent
        "MOV.N %[r_byte_to_send], %[r_reg_addr];"
        "MOVI.N %[r_send_addr], 0;"
        "j l_send_byte;"

        "l_send_next_array_byte:"

Here's what the whole section looks like:

        "BEQZ   %[r_send_addr], l_send_next_array_byte;"   // Branch to array address if the register address does not need to be sent
        "MOV.N %[r_byte_to_send], %[r_reg_addr];"
        "MOVI.N %[r_send_addr], 0;"
        "j l_send_byte;"

        "l_send_next_array_byte:"
        "BEQZ   %[r_no_of_bytes], l_send_stop;"    // Branch if there are no more Data Bytes to send
        // Load the corresponding element of array data[.] into byte_to_send
        "L8UI   %[r_byte_to_send], %[r_adr_array_element], 0;"
        // Move the pointer to the next array element (since we have an array of bytes, the increment is 1)
        "ADDI.N %[r_adr_array_element], %[r_adr_array_element], 1;"
        // Decrement the number of bytes to send
        "ADDI.N %[r_no_of_bytes], %[r_no_of_bytes], -1;"
        "j l_send_byte;"

Now for the rest of the housekeeping.

In the .C file add "_internal" to the original function and add two new parameters for "boolean send_addr" and "uint8_t reg_addr".

Then add two new functions, one to provide the original write, and the other to take a register address.

The final form will look something like this:

    void ICACHE_RAM_ATTR brzo_i2c_write_internal(uint8_t *data, uint8_t no_of_bytes, boolean repeated_start, boolean send_reg_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);
    }

    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) {
        ...  Original function is this ...

            : [r_set] "+r" (a_set), [r_repeated] "+r" (a_repeated), [r_temp1] "+r" (a_temp1), [r_in_value] "+r" (a_in_value), [r_error] "+r" (i2c_error), [r_bit_index] "+r" (a_bit_index), [r_adr_array_element] "+r" (&data[0]), [r_byte_to_send] "+r" (byte_to_send), [r_no_of_bytes] "+r" (no_of_bytes), [r_send_addr] "+r" (send_addr), [r_reg_addr] "+r" (reg_addr)
            : [r_sda_bitmask] "r" (sda_bitmask), [r_scl_bitmask] "r" (scl_bitmask), [r_iteration_scl_halfcycle] "r" (iteration_scl_halfcycle), [r_iteration_minimize_spike] "r" (iteration_remove_spike), [r_iteration_scl_clock_stretch] "r" (iteration_scl_clock_stretch)
            : "memory"
        );
        return;
    }

Like I said, as written, it requires too many registers; but it's really close.
I'm open to comments; but I'll try changing r_repeated to a bitfield and see if I can go from there.

Mike

pasko-zh · 2016-11-04T07:18:11Z

Before rushing to (my "other") work... and being heavily occupied by it the last weeks, I just want to thank both of you for your (detailied) comments/thoughts. I should be able to spend some time on the weeknd on this issue.

pasko-zh · 2016-11-06T20:57:26Z

Well, I can see your point.
However, when I wrote the library and their functions, I wanted to keep them as simple as possible. For brzo_i2c_write it just means, send n (>= 0) bytes from a buffer to an i2c slave. If the buffer contains "data" or "register addresses" doesn't matter, because it is the notion of a buffer.

In contrast, have a look for instance at the i2c ibrary of DSScircuits. You will find many i2c write methods, for instance I2c.write(address, registerAddress) or I2c.write(address, registerAddress, *data, numberBytes). The latter comes close to what you would like to have.

I didn't want to go the DSS or wire library way with half a dozen of overloaded methods. Like send a register and one byte, or send only one byte, or two, or a register alone, or...

So, coming back to the EEPROM. Now, if would like to do a page write to an EEPROM, you need to transmit: START, Control byte (aka i2c slave address of the EEPROM), Address high byte, Address low byte, Data byte 0, ..., 31, STOP.
So, what are the arguments against having all the data to be transmitted in one array? You don't need to do a memcopy, just keep additional two bytes at the beginning of your array. OK, it is a bit "ugly" having addresses and data in the same array, but...

(btw: The wire library uses a fixed buffer size of 32 bytes.. So, TWI will just copy everything you want to send the TwoWire::txBuffer[BUFFER_LENGTH] and will then transmit it.)

valkuc · 2016-11-06T22:48:04Z

Well, actually idea of this change is to allow to send to i2c slave any data from any "buffers" using brzo_i2c_write subsequently. Currently this is not possible because brzo_i2c_write always send "Setup Write" i2c command. If you just add ability to suppress that - this will solve this problem and give developers ability to run next code:

brzo_i2c_write(buf1, sizeof(buf1), true); // true - Setup write and send buf1 data to i2c slave
brzo_i2c_write(buf2, sizeof(buf2), false); // false - means just to send data from buf2 without issuing "Setup Write"
brzo_i2c_write(buf3, sizeof(buf3), false); // same as before - just send another buffer

This appoach will allow to send multiple buffers to i2c slave and slave will see all that buffers as one large buffer.

MikeFair · 2016-11-07T05:36:46Z

So, what are the arguments against having all the data to be transmitted in one array?

Primarily that variables in the code are typically stored with the data to be read/written in data arrays and the register addresses and device addresses stored separately in #defines or other const expressions.

So typically I have multiple data array variables, and a mixture of immediate values and variables for device and register addresses:
#define SOMEDEV_DEVADDR 0x01
#define SOMEDEV_REGADDR 0x00
uint8_t devAddr= SOMEDEV_DEVADDR;
uint8_t regAddr = SOMEDEV_REGADDR;
uint8_t wData[12] = {'d','a','t','a',' ','t','o',' ','s','e','n','d'};
uint8_t rData[13] = {'d','a','t','a',' ','w','a','s',' ','r','e','a','d'};

Given this, and this pattern is very typical, there are few options to prepend regAddr in the data[0] location of either array without doing a lot of memory copying or clobbering actual data.

As for @valkuc's comment; I haven't written to EEPROM's but I think his case is already taken care of with the restart flag isn't it?

brzo_i2c_begin_transaction(devAddr, devSpeed);
brzo_i2c_write(&reg_addr, 1, true);
brzo_i2c_write(buf1, sizeof(buf1), true);
brzo_i2c_write(buf2, sizeof(buf2), true);
brzo_i2c_write(buf3, sizeof(buf3), false);
uint8_t result = brzo_i2c_end_transaction();

Why doesn't that do what he wants?
It issues a restart with the device, doesn't let go of the bus, and keeps streaming to the same device.
Now if buf1, buf2, and buf3 all have reg_addr in their [0] spot, then that's the buffer sending the register address byte, not brzo.

It looks like I misread the initial post and am going to open a separate case for what I'm talking about which is more about mixing reads and writes and providing an optional function to send the register address separately from the address of the data buffer.

Thanks

valkuc · 2016-11-07T06:43:37Z

@MikeFair, good explanation. Yes, actually main idea of all this is to allow streaming of data. But... I had some time to think about that and I looks like @pasko-zh is actually right to not implement that. The problem that can occur here is that at high speeds (say > 400KHz) subsequent calls to brzo_i2c_write can introduce extra delays in data transmission. Maybe this is the reason why TWI library use internal array of data where all user-provided data is temporary stored and then sent at once from it.

UPD.
Anyway, one month of 24/7 testing shows that library works like a charm (I using it to write statistical information to EEPROM every 10 minutes at 100 KHz and interaction with DS3231 RTC at 400KHz).

pasko-zh · 2016-11-07T06:50:02Z

@valkuc : By "setup write" do you mean

Send START
Transmit i2c slave address

If so, then you would like to have an additional parameter which allows to control that behaviour, i.e. something like brzo_i2c_write(uint8_t *data, uint8_t no_of_bytes, boolean repeated_start, boolean send_start_with_slave_address).

OK, really misleading names for the parameters now, but I think you got the point 😄

valkuc · 2016-11-07T06:53:24Z

By "Setup Write" I mean what the Saleae Logic analizer shows me :) Yes, I guess that is exactly START command and i2c address. The same is for read command, but then Logic analizer shows "Setup Read".
Here is example: http://3.bp.blogspot.com/-ksMiaHCq_7A/VSS9gkROTQI/AAAAAAAAAH0/9F-RuNW1ijU/s1600/Screen%2BShot%2B2015-04-07%2Bat%2B10.22.27%2BPM.png

pasko-zh · 2016-11-07T06:55:54Z

@MikeFair : Probably you do mean something different than @valkuc.

I certainly see your points, of course. However in the particular code you presented, why not use uint8_t wData[14] = {'', '', d','a','t','a',' ','t','o',' ','s','e','n','d'}; ? And then copy those register addresses, when needed, to wData[0] and wData[1] and transmit it all together?

valkuc · 2016-11-07T07:01:48Z

brzo_i2c_write(uint8_t *data, uint8_t no_of_bytes, boolean repeated_start, boolean send_start_with_slave_address)
Yes, that is exactly what I'm talking about, but now I'm unsure about necessity of that (see my thinking #13 (comment) about extra delays)

pasko-zh · 2016-11-07T07:14:10Z

OK, thanks for the pic; it is START and slave address.

Glad to hear that the library works for you :)

Let me know about the necessity of this additional parameter.

valkuc · 2016-11-07T07:17:35Z

Well, if it's not hard to implement it, let's try. Then I will compare signals with logic analizer and will see.

MikeFair · 2016-11-07T13:51:34Z

@valkuc I'll let @pasko-zh be the final word, but I agree with those extra delays between function calls being precisely why those starts are likely required.

FWIW, I added a similar flag in the code I posted in #15 you could look at. In that case it was to send an extra byte (the register address); but you could use the basic idea of testing a ctrl_flags bit and jumping based on whether or not the bitflag is set/clear.

Instead of hoping returning back to the loop was fast enough, I could see myself trying to use hardware line interrupts, timer interrupts, or some other technique to let the library keep control of sending the clock pulses (aka the i2c function loop is still running) and feeding it a stream of buffer address pointers and lengths. So instead of calling the loop with the data pointer and length, I either give it a function callback that sets the next buffer address and length and returns a 1 or 0 to inform the loop whether or not it succeeded (or setting the length to 0 means "no work to do") or give it the address of a couple global variables where the new addresses and lengths get placed.

It's an interesting idea, to provide the i2c routines with a mechanism to keep the clock pulses going while getting fed new sources/targets of data to send/fill...

valkuc · 2016-11-07T15:34:07Z

I'm not very well understand assembler. To be clear, last time I was trying to write something on it - that was on ZX-Spectrum :) But, I definetely sure that introducing any mechanisms to "keep-alive" clock pulses will be total overkill for this lib. That is redundant in my opinion. I think that it's better to:

either use my variant (with extra argument to write method)
either your variant (with send single byte)
leave as is :) to not broke anything that is currently working perfect :)

pasko-zh added the enhancement label Oct 7, 2016

MikeFair mentioned this issue Nov 7, 2016

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

brzo_i2c_write: allow subsequent calls #13

brzo_i2c_write: allow subsequent calls #13

valkuc commented Oct 5, 2016 •

edited

Loading

MikeFair commented Nov 3, 2016 •

edited

Loading

pasko-zh commented Nov 4, 2016

pasko-zh commented Nov 6, 2016

valkuc commented Nov 6, 2016

MikeFair commented Nov 7, 2016 •

edited

Loading

valkuc commented Nov 7, 2016 •

edited

Loading

pasko-zh commented Nov 7, 2016

valkuc commented Nov 7, 2016 •

edited

Loading

pasko-zh commented Nov 7, 2016

valkuc commented Nov 7, 2016 •

edited

Loading

pasko-zh commented Nov 7, 2016

valkuc commented Nov 7, 2016

MikeFair commented Nov 7, 2016

valkuc commented Nov 7, 2016 •

edited

Loading

brzo_i2c_write: allow subsequent calls #13

brzo_i2c_write: allow subsequent calls #13

Comments

valkuc commented Oct 5, 2016 • edited Loading

MikeFair commented Nov 3, 2016 • edited Loading

pasko-zh commented Nov 4, 2016

pasko-zh commented Nov 6, 2016

valkuc commented Nov 6, 2016

MikeFair commented Nov 7, 2016 • edited Loading

valkuc commented Nov 7, 2016 • edited Loading

pasko-zh commented Nov 7, 2016

valkuc commented Nov 7, 2016 • edited Loading

pasko-zh commented Nov 7, 2016

valkuc commented Nov 7, 2016 • edited Loading

pasko-zh commented Nov 7, 2016

valkuc commented Nov 7, 2016

MikeFair commented Nov 7, 2016

valkuc commented Nov 7, 2016 • edited Loading

valkuc commented Oct 5, 2016 •

edited

Loading

MikeFair commented Nov 3, 2016 •

edited

Loading

MikeFair commented Nov 7, 2016 •

edited

Loading

valkuc commented Nov 7, 2016 •

edited

Loading

valkuc commented Nov 7, 2016 •

edited

Loading

valkuc commented Nov 7, 2016 •

edited

Loading

valkuc commented Nov 7, 2016 •

edited

Loading