cpu/cc2538: adapt periph to new I2C API

[smlng]

Contribution description

This adapt the CC2538 I2C to the new API.

Problem right now: it works with debug enabled but not without.

Issues/PRs references

#6577

[MrKevinWeiss]

Hmmm, it seems like the register is volatile which means it shouldn't be optimized out. It could just be the delay from the print of the debug. I'll do a proper test (and might need some hardware from you) after I finish current task.

[smlng]

Hey I fixed the issue by adding a small delay incrementing a variable, now it works with and without debug output.

[smlng]

please note this also introduces a new gpio init function because the present cannot handle init of I2C pins but is used for UART. I'll will sort out things later on ...

[dylad]

@smlng Can you tell us on which sensors did you test it ? It can help merging some devices faster at the same time.
I'll take a look at it but if @MrKevinWeiss or someone can test it. It would be awesome !

[smlng]

I used the reworked periph_i2c by @MrKevinWeiss for testing, this is even better than just using some i2c sensor driver.

[MrKevinWeiss]

Ya, so far it looks like there is a way to go... I will play around with it for the remainder of the day but currently it seems like there is unexpected behavior in the remote-reva.

• read/write to wrong addr will result in data clk after failed addr
• a wrong addr write seems to block all reads until the correct write happens

I will spend the rest of the day trying to test/provide fix suggestions

[MrKevinWeiss]

Man, I thought the sam_common0 had HW issues. So I believe that the hardware will automatically send/clk a data byte regardless of an addr ack. We probably have to accept it. It seems like the write addr ack error does not get cleared properly. If a read is issued after it will fail and will not be able to clear the byte (even though the scope looks fine). It seems the only way to clear the error is with a successful write...

[MrKevinWeiss]

If you can find a way to clear the error flags before exiting that would be nice... I ran out of time.

[smlng]

yeah, I came to the same conclusion - will look into it.

[MrKevinWeiss]

The only way I could clear the flag is with a successful write. Maybe try the standard solution? (turn it off and on again)

[dylad]

@smlng Overall changes look ok but you need to add unsupported features check at the beginning of read_bytes and write_bytes.

if (flags & (I2C_REG16 | I2C_ADDR10)) {
    return -EOPNOTSUPP;
}

[jnohlgard]

@dylad I edited your comment to fix some typos in the code example

[smlng]

@dylad good point, will do - but also need to sort out the write issue reported by @MrKevinWeiss

[MrKevinWeiss]

I could be convinced to remove that from the test (or make it not critical). This would mean that there must be a successful write before a read can happen. Ideally there should be a reset of error state though. I imagine the hardware isn't the most versatile.

[smlng]

@MrKevinWeiss maybe the STOP command on error is not (always) needed, i.e., only put STOP when flags wasn't set in the original command. I need to tests this, because the data sheet doesn't say anything about reset error state (only IRQs), but it does say the STOP is either for successful write/read or error.

I also need to add the flags handling as noted by @dylad, try to get this done in the next hours.

[smlng]

hey @dylad I added the flags check and I (believe) have fixed read/write issue with wrong addresses -> need to access the data register before checking for errors 😩 @MrKevinWeiss can you verify by (re)testing? THX!!

[MrKevinWeiss]

So the problem of not clearing error flags before a read is still there. The test was altered to specify a successful write must occur before a read.
Also an error was expected when sending a nostart read outside of a frame and did not occur. The test was altered to allow for this.

It is up to you to decide if that should be acceptable behavior. Everything else seems to work fine (ie 99% of use cases).

Results

[MrKevinWeiss]

If you would like to reproduce the two questionable error cases.

1. Send read_reg to a incorrect addr, then read_bytes to the correct one
2. Send a read_reg, then send a read_bytes with I2C_NOSTART flag active (should be an error since it no address should be sent after stop)

[MrKevinWeiss]

Just to confirm if a NOSTART read is done it says it succeeds, nothing happens on the scope, it seems to just read back whatever data was last in the register... This is improper use of the I2C frame, if it is not easy to catch in hardware though maybe we just leave if for the time being.

[MrKevinWeiss]

I retested and it seems like the only issue left is the error flag not resetting when triggered with a NACK addr write. To keep things simple I suggest we just specify that any read should have had a ACKed write before. Eventually we will make this a warning for the test but not a test failure. If that is the case just make Travis happy then we can merge.

[MrKevinWeiss]

Also I don't think new_i2c_if branch goes through murdock... but I could be wrong.

[dylad]

Murdock will only be used at the very end, right now, this branch is completely broken :)
For now, Travis is enough.

[MrKevinWeiss]

@dylad Is it ok to merge? Aside from the one corner case I mentioned it seems to be alright!

[dylad]

Here we go !