raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.26k stars 838 forks source link

Strange behavior with 'i2c_write_blocking' #1626

Closed seamusdemora closed 4 months ago

seamusdemora commented 5 months ago

This could be caused because I've missed something, but if that's the case, someone will have to point that out to me.

According to the latest version of the Pico C/C++ SDK:

4.1.10.3.14. i2c_write_blocking
int i2c_write_blocking (i2c_inst_t * i2c, uint8_t addr, const uint8_t * src, size_t len, bool nostop) 

Attempt to write specified number of bytes to address, blocking.

Parameters
   i2c Either i2c0 or i2c1
   addr 7-bit address of device to write to
   src Pointer to data to send
   len Length of data in bytes to send
   nostop If true, master retains control of the bus at the end of the transfer (no Stop is issued), and the next transfer will begin with a Restart rather than a Start.

Returns
Number of bytes written, or PICO_ERROR_GENERIC if address not acknowledged, no device present.

If I do this - everything works as it should; i.e. I can read the values back from CONFIG_REG, and they match with the values I write below.

#define ADDR _u(0x40)
#define CONFIG_REG _u(0x00)
i2c_init(i2c_default, 100000);
uint8_t condata[3];
condata[0] = CONFIG_REG; 
condata[1] = 97
condata[2] = 37
i2c_write_blocking (i2c_default, ADDR, condata, 3, false);

But if I do this - things do not work as they should; the values I read back from the register (CONFIG_REG) do not match with the values I write below.

#define ADDR _u(0x40)
#define CONFIG_REG _u(0x00)
i2c_init(i2c_default, 100000);
uint8_t reg = CONFIG_REG;  
uint8_t condt[2];
condt[0] = 97;
condt[1] = 37;
i2c_write_blocking (i2c_default, ADDR, &reg, 1, true);
i2c_write_blocking (i2c_default, ADDR, condt, 2, false);

And here's how I read the register:

#define ADDR _u(0x40)
#define CONFIG_REG _u(0x00)
i2c_init(i2c_default, 100000);
uint8_t reg = CONFIG_REG;  
uint8_t rcvdata[2];
i2c_write_blocking (i2c_default, ADDR, &reg, 1, true);
i2c_read_blocking (i2c_default, ADDR, rcvdata, 2, false);

The difference between the two methods for write is that I am using a single 3-byte write in the first case, and two (2) writes in the second case - one with a single byte for the register ADDR, the next with a 2-byte array. But the second case does not appear to work!

I've checked the return values from the two i2c_write_blocking calls, and I get a 1 from the first, and a 2 from the second (i.e. what I expected to see for the commands above). I'm using a recently-installed SDK for the Pico, and running that on an RPi5. I've read the documentation until I am blue in the face, and I find no mention of anything that might explain it.

Can someone explain what is happening? ... Why is the second method failing???

peterharperuk commented 5 months ago

Shouldn't that be...?

i2c_write_blocking (i2c_default, ADDR, &reg, 1, true); i2c_write_blocking (i2c_default, ADDR +1, condt, 2, false);

lurch commented 5 months ago

The docs you've quoted say "the next transfer will begin with a Restart rather than a Start", so perhaps the behaviour you're seeing depends on how the device you're talking to responds to the Restart condition?

It sounds like you might be trying to do the same thing as #812 ?

seamusdemora commented 5 months ago

@peterharperuk

Shouldn't that be...?

i2c_write_blocking (i2c_default, ADDR, &reg, 1, true); i2c_write_blocking (i2c_default, ADDR +1, condt, 2, false);

Why would you increment the bus address of the device??

peterharperuk commented 5 months ago

My mistake, ignore me

seamusdemora commented 5 months ago

@lurch

The docs you've quoted say "the next transfer will begin with a Restart rather than a Start", so perhaps the behaviour you're seeing depends on how the device you're talking to responds to the Restart condition?

It sounds like you might be trying to do the same thing as #812 ?

"The docs I've quoted" are directly from the Pico SDK. And I've tried it both ways... in fact, I think I've tried it every way possible! But it still will not work.

WRT device dependency: I can't rule that out without more data from the mfr (I am working to get that) --- but I tend towards discounting that as an explanation because the part is manufactured by Texas Instruments. It's an INA260 ICYI.

When I "do the math" to guess where responsibility might lie, I think like this: TI : in the business since 1951, make huge numbers of parts incl. processors/uC/IC/etc/etc RPi: makes "good stuff", but no comparison to TI

That's definitely not to say that it's an RPi issue; there are actually 3 possibilities: RPi, TI... or Me. :) ... I guessed Me first, RPi second. Thus, my post here. But the fact that no one can explain this doesn't inspire confidence... what's most surprising is that no one else has ever reported this????

WRT #812 : I don't know how the two functions relate, but again - not confidence-inspiring.

lurch commented 5 months ago

I dunno much about I2C, but I did a quick search and found https://learn.adafruit.com/working-with-i2c-devices/repeated-start which looks relevant. Note that even Adafruit say "The answer is target device specific and would be found in the target device datasheet.".

maqifrnswa commented 5 months ago

From the INA260 datasheet 8.5.3 Communications Bus Overview: "After all [8 bits of] data have been transferred, the master generates a stop condition, indicated by pulling SDA from low to high while SCL is high. The device includes a 28-ms timeout on its interface to prevent locking up the bus."

It looks like TI designed the chip to not allow repeated starts so that it can do locking up protection. Which is a valid design choice, not a bug in the TI chip.

Edit: auto correct changed "not a bug" to "but a bug". It's not a bug in either the TI or RP2040, it's the designed behavior for both.

seamusdemora commented 5 months ago

@maqifrnswa Scott - Many thanks for bringing that to my attention. I'd actually read it 2 or 3 times, but may have mis-interpreted the meaning. And I tried both (true and false) stop/reset settings to no good effect - which convinced me there was "something else" going on.

Let me try this again... I'll add a 30 ms sleep command between the writes, and I'll follow up with my results here.

lurch commented 5 months ago

@seamusdemora As I've already mentioned, I don't have a great deal of familiarity with I2C, so my descriptions here might not be 100% accurate...

You seem to be assuming that:

i2c_write_blocking (i2c_default, ADDR, condata, 3, false);

should produce exactly the same result as:

i2c_write_blocking (i2c_default, ADDR, &reg, 1, true);
i2c_write_blocking (i2c_default, ADDR, condt, 2, false);

but AFAICT, it won't. (this is why I said that the behaviour is going to be device-specific)

I believe the first example will produce something like "START, BYTE1, BYTE2, BYTE3, STOP" whereas the second example will produce "START, BYTE1, RESTART, BYTE2, BYTE3, STOP". So I don't think that adding a 30ms sleep between the writes will change things?

seamusdemora commented 5 months ago

@lurch @maqifrnswa

Andrew - If I understand your comment, the issue is that I should have done it this way:

i2c_write_blocking (i2c_default, ADDR, &reg, 1, false);
i2c_write_blocking (i2c_default, ADDR, condt, 2, false);

Maybe I'm not clear on the bool nostop? I understood a true to be stating: more data follows. Is that wrong? AFAIK, the SDK doesn't elaborate on this? At any rate, the double false writes do not make any difference - or at least the result remains incorrect.

Scott & Andrew -

Adding a sleep_ms(30); statement in between the two write statements does not make any difference to the final result. IOW, whatever it is that's causing this does not seem to be affected by the INA260 datasheet 8.5.3 you referenced.

And so my question/confusion remains: "How do I write a one-byte message, and then write a two-byte message?

seamusdemora commented 5 months ago

@lurch

I dunno much about I2C, but I did a quick search and found https://learn.adafruit.com/working-with-i2c-devices/repeated-start which looks relevant. Note that even Adafruit say "The answer is target device specific and would be found in the target device datasheet.".

Andrew - That's a v. good reference - thanks. AIUI, the START & STOP signals have to do with the relative states of SDA & SCL.

But since the SDK doesn't (AFAIK) supply a means to signal START & STOP, I'm not quite sure what to do with this information. It could certainly be relevant, but it seems we may be limited by how the writes are implemented in the Pico SDK? Or am I missing something??

lurch commented 5 months ago

Andrew - If I understand your comment, the issue is that I should have done it this way:

i2c_write_blocking (i2c_default, ADDR, &reg, 1, false);
i2c_write_blocking (i2c_default, ADDR, condt, 2, false);

No, AIUI that would give you "START, BYTE1, RESTART, BYTE2, BYTE3".

Maybe I'm not clear on the bool nostop?

AFAICT it omits the "STOP".

And so my question/confusion remains: "How do I write a one-byte message, and then write a two-byte message?

So it sounds like (for your particular device) you need to send "START, BYTE1, BYTE2, BYTE3, STOP" with no "RESTART" between BYTE1 and BYTE2. That's exactly why I linked to #812 :roll_eyes:

Alternatively, you could just stick with the "everything works as it should" approach you described in your original message?

EDIT: I've just realised that I've put "START" where I should have put "RESTART". I've now edited both this and my previous comment.

maqifrnswa commented 5 months ago

Let me try this again... I'll add a 30 ms sleep command between the writes, and I'll follow up with my results here.

That wouldn't work, like you saw, because the pico never did a STOP. After 28 ms, the chip thought "I haven't heard a STOP, I guess that means whatever I was talking to hung the line (which you did do, intentionally). Let me do everyone a favor and reset the I2C bus."

The chip you're talking to doesn't seem to support what you are trying to do. According to the data sheet, AFAICT all messages are one byte messages followed by a STOP. So you can send a two byte message over I2C without stopping using the SDK just like you are doing (without a stop in between), but if you do, the device you're talking to will think something crashed in the system and essentially go into recovery mode.

The way TI wants you (and requires you) to talk to its chip is one byte at a time so that it can use its recovery feature in case something bad happens while two devices are talking to each other.

seamusdemora commented 5 months ago

Let me try this again... I'll add a 30 ms sleep command between the writes, and I'll follow up with my results here.

That wouldn't work, like you saw, because the pico never did a STOP. After 28 ms, the chip thought "I haven't heard a STOP, I guess that means whatever I was talking to hung the line (which you did do, intentionally). Let me do everyone a favor and reset the I2C bus."

"... wouldn't work"... Ya - I figured that much out. :) That was the only idea I had in an effort to avoid the device's 28ms timeout.

The chip you're talking to doesn't seem to support what you are trying to do. According to the data sheet, AFAICT all messages are one byte messages followed by a STOP. So you can send a two byte message over I2C without stopping using the SDK just like you are doing (without a stop in between), but if you do, the device you're talking to will think something crashed in the system and essentially go into recovery mode.

The way TI wants you (and requires you) to talk to its chip is one byte at a time so that it can use its recovery feature in case something bad happens while two devices are talking to each other.

I'm not sure that's the case. Assuming you're referring to the passage from the data sheet you quoted earlier, I just don't read the same meaning into it that you do.

The return value from the 2-byte write is 2 - indicating that two bytes were sent. I guess that's what happened, but I'll have to put a scope or LA on it to see. But what appears to be happening is that the chip (INA260) is being RESET - which requires a 1 in bit position 15 of the Config register. The values that I read back from the register show the values as default (i.e. Reset) values. ... still more baffling.

seamusdemora commented 5 months ago

@maqifrnswa

The chip you're talking to doesn't seem to support what you are trying to do. According to the data sheet, AFAICT all messages are one byte messages followed by a STOP. So you can send a two byte message over I2C without stopping using the SDK just like you are doing (without a stop in between), but if you do, the device you're talking to will think something crashed in the system and essentially go into recovery mode.

The way TI wants you (and requires you) to talk to its chip is one byte at a time so that it can use its recovery feature in case something bad happens while two devices are talking to each other.

But if that's the case, why does it work with a 3-byte message? i.e. register_id, LSB, MSB as I showed in my original post

seamusdemora commented 5 months ago

@lurch Andrew,

So it sounds like (for your particular device) you need to send "START, BYTE1, BYTE2, BYTE3, STOP" with no "RESTART" between BYTE1 and BYTE2. That's exactly why I linked to #812 🙄

Alternatively, you could just stick with the "everything works as it should" approach you described in your original message?

Yeah - I know... but I just do not like being forced to create a 3-byte array to do something that I feel should work. Once again - why can I send 3-bytes in one function call, but cannot send 1 byte, then 2 bytes in two separate calls? That just doesn't make sense to me. Does it to you??

maqifrnswa commented 5 months ago

But if that's the case, why does it work with a 3-byte message? i.e. register_id, LSB, MSB as I showed in my original post

Got it, what you're saying makes sense. I was thinking there was some interrupt out something else happening between the two functions calls causing it to delay longer than 28 ms.

seamusdemora commented 4 months ago

Where does this issue "go" from here? I feel I've raised an issue/question that bears further investigation, but I'm not familiar with the remediation process. Should I submit this as a "bug report"??? If so, to which group does it go... documentation, or SDK, or somewhere else ?

maqifrnswa commented 4 months ago

This is the bug report. It's up to developers to decide if anything should be done.

On one hand, it is working as designed and documented. @lurch pointed out that the INA260 does not support RESTART in the middle of the transmission over I2C, and "nonstop" adds a RESTART. The INA260 expects the data to come in the format that the SDK sends without setting nonstop to True. From the SDK documentation:

nostop If true, master retains control of the bus at the end of the transfer (no Stop is issued), and the next transfer will begin with a Restart rather than a Start.

On the other hand, there could be value in exposing a "no stop with no restart" option, which is part of the other GitHub issue @lurch linked to (that asks "what should raw writing end with?") It might not be worth adding that extra complexity, but that's a design trade-off decision the developers make. It's also likely that adding that option could lead to confusion by users expecting a certain behavior that different devices respond to differently.

The good thing is that you did raise the issue so that others that are trying to send data without a RESTART can see that they need to use a single write command in the SDK.

peterharperuk commented 4 months ago

It sounds like this has concluded that it's a device issue, apart from the issue raised in https://github.com/raspberrypi/pico-sdk/issues/812 Correct me if I'm wrong but I don't see that there's anything for us to do, so closing.

seamusdemora commented 4 months ago

I don't feel that it's necessarily a device issue. AFAICS, no one has pointed that out. But go ahead and close it; I guess you guys have enough open issues to keep you busy for a while, and I can re-open this/begin a new issue once I've got some hard data.