Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Questions about I2C slave #379

Open
toptensoftware opened this issue May 29, 2023 · 33 comments
Open

Questions about I2C slave #379

toptensoftware opened this issue May 29, 2023 · 33 comments

Comments

@toptensoftware
Copy link

Where can I find some more information about how i2c slave works? For example:

  • I'm looking at the i2cping example, and it looks like the slave is polling for updates. Is it possible to get an interrupt notification instead of polling?
  • If a read asks for more bytes than available does it block until that many bytes have been read, or does it return immediately with whatever is currently available?
  • Is there any buffering on the received data and if so what size is the buffer?
@rsta2
Copy link
Owner

rsta2 commented May 30, 2023

The Circle I2C slave driver is simple polling driver. The I2C slave peripheral in the Raspberry Pi supports interrupts, but this is not used here. When more bytes are requested than are actually sent by the master, the received bytes are returned immediately in the buffer with the byte count returned by Read() smaller than the given buffer size. The peripheral has a FIFO. I guess it can buffer 16 bytes, but this is not specified in the hardware specification, which is here (page 160-171).

@toptensoftware
Copy link
Author

Thanks Rene, that's useful info. Hopefully polling will be enough for now.

@rsta2
Copy link
Owner

rsta2 commented Jun 15, 2023

Did this work for you or do we need an interrupt driven I2C slave?

@toptensoftware
Copy link
Author

Hey Rene,

I haven't had a chance to look at it properly yet (I've been stuck trying to get a DPI LCD display panel working). If it's not too hard to implement I'd almost certainly use it. My main concern about polling it is I'll be running an OpenGL render loop too which I think throttles the loop to the frame rate - not sure if that'd be fast enough for polling i2c.

Brad

@rsta2
Copy link
Owner

rsta2 commented Jun 15, 2023

Hi Brad, OK, let me know, when polling I2C is not fast enough.

Rene

@toptensoftware
Copy link
Author

Hi Rene,

I'm finally getting around to integrating this with my OpenGL render loop and have hit a blocking issue - it seems CI2CSlave::Read() blocks if nothing has been received. I've tried just reading a single byte and it doesn't return until the first byte is received.

How do I poll and just read what's available and return immediately if there's nothing?

Brad

@toptensoftware
Copy link
Author

toptensoftware commented Jun 21, 2023

fwiw, not sure if this correct but I got this working by replacing the first while loop in CI2CSlave::Read with this:

	while (nCount > 0 && (read32 (ARM_BSC_SPI_SLAVE_FR) & FR_RXFE) == 0)
	{
		if (read32 (ARM_BSC_SPI_SLAVE_RSR) & RSR_OE)
		{
			nResult = -1;

			break;
		}

		*pData++ = read32 (ARM_BSC_SPI_SLAVE_DR) & DR_DATA__MASK;

		nResult++;
		nCount--;
	}

@rsta2
Copy link
Owner

rsta2 commented Jun 21, 2023

Hi Brad, yes, you are right, it blocks and there is no way out with the current implementation. Please give me two days for an update, which is also in sync with sample/16-i2cping.

Rene

@toptensoftware
Copy link
Author

Thanks Rene. No rush as I can work with my temp hack until you get it sorted.

rsta2 added a commit that referenced this issue Jun 22, 2023
* Comment for Doxygen
* Add note on General Call Address 0 (#383)
@rsta2
Copy link
Owner

rsta2 commented Jun 22, 2023

I added a timeout parameter to CI2CSlave::Read() and CI2CSlave::Write() as a more generic solution. When you specify TimeoutNone as 3rd parameter, you should get the same behavior, as suggested by yourself. By default the methods will block, when no transfer occurs, which is similar to the previous behavior. It's on the develop branch.

@toptensoftware
Copy link
Author

Awesome, thanks Rene. I'll check it out but might be a few days... I've got some other high priority work that's popped up but hope to get back to this soon.

@rsta2
Copy link
Owner

rsta2 commented Jun 23, 2023

Thanks Brad for info. No problem.

@toptensoftware
Copy link
Author

Hi Rene,

I'm back working on my bare metal Pi project again and I'm having issues with I2C - receiving corrupted packets and occasional lock ups. Some questions:

  • How hard would it be to update CI2CSlave to support an interrupt mode that moves incoming data to a temporary buffer to make it more resilient in case the program is busy and doesn't poll quickly enough?
  • Is there any way I can check if there's been I2C data dropped/lost and/or monitor the state of the I2C port?
  • Are there any other timeouts or control over the slave mode? I2C lockups are usually because the slave is holding the data line and the master can't get control again. I think that might be happening in my case and wondering if there's anything I can do to release it. I'm still investigating what's causing the lock up, but it'd be good to have a way to force the slave to let go on a timeout (if possible).

Anything that can be done to make this more reliable would be much appreciated (at the end of the day I need something rock solid).

Brad

@rsta2
Copy link
Owner

rsta2 commented Sep 20, 2023

Hi Brad,

it should be possible to extend the I2C slave receiver with an IRQ mode. The hardware spec. (pg. 160) mentions a RXINTR, but because I never used it so far, it requires some experimenting. There is also an overrun error interrupt (OEINTR), which could be monitored. There is a BRK (break current operation) bit in the Control register, which according to the spec. stops the current operation and clears the FIFOs.

I'm not fully aware of your use case. Can you please give me more info about how you want to use the I2C slave, so that I can prepare a test program and define the class interface.

Thanks,

Rene

@toptensoftware
Copy link
Author

Emailed you.

@toptensoftware
Copy link
Author

toptensoftware commented Sep 21, 2023

For reference, here's a good description of i2c lockup, prevention and recovery:

https://pebblebay.com/i2c-lock-up-prevention-and-recovery/

@rsta2
Copy link
Owner

rsta2 commented Feb 5, 2024

So that it is not lost, attached there is a slightly modified sample/16-i2cping/slave, which contains a I2C slave driver in the files i2cslaveirq.*. The I2C receiver in this driver is IRQ driven and should not "loose" bytes any more. The receiver is always on after Initialize() and can receive packets in the background. It can buffer up to 4096 bytes. When you call Read(), the recently received packet(s) will be returned. If no packet has been received, it immediately returns 0. On error a negative value is returned. The Write() method is mostly unmodified.

i2c-slave-irq.zip

@rsta2
Copy link
Owner

rsta2 commented Feb 5, 2024

While porting the I2C master driver for the Raspberry Pi 5, I discovered this. How it seems, the I2C bus recover must be done by the I2C master, which is implemented here for Linux. The slave cannot do this.

@toptensoftware
Copy link
Author

Thanks Rene.. Unfortunately I've had to put my project on hold for the moment due to other commitments but this will be useful when I get back to it.

@rsta2
Copy link
Owner

rsta2 commented Feb 6, 2024

No problem Brad. Understood.

@sebastienNEC
Copy link

Hello,
I had a similar need recently whereby I was spending a good share of the CPU continuously polling an I2C slave device (I was too lazy to solder its interrupt GPIO). So I wrote an I2CMasterIRQ class - it's probably not 100% complete but it works in my case.
Here it is, if interested ?
i2c-master-irq.zip

@rsta2
Copy link
Owner

rsta2 commented Feb 29, 2024

Thanks @sebastienNEC! I think, @toptensoftware needs an I2C slave on the Raspberry Pi and runs the master on an other type of board. But perhaps your work can be used to add an IRQ-driven I2C master to Circle. What do you think has to be added/changed on your driver to prepare it for that purpose?

@sebastienNEC
Copy link

sebastienNEC commented Feb 29, 2024

Not much needs changed I would think, just a bit of sanity-check on your side :-)
Right now, my driver has these limitations:

  • It only supports the Write-Then-Read operation, i.e. not individual Read or Write operations. It's not hard to add but it might end up not being used because most I2C slave boards expose a Write/Read register-based "API".

  • It assumes that the number of bytes read/written fits into the I2C's FIFO. I just noticed btw that I am not checking this for the read buffer, so it needs added. Then again, this is not a limitation for usual I2C slave devices.

Also, I might have been a bit lax with the checks on the Status register in CI2CMasterIRQ::InterruptHandler(). I am only checking the 3 more important indicator bits but one should probably have a look at other status bits.

@sebastienNEC
Copy link

Oh, one important extra limitation: due to the asynchronous nature, it's possible/realistic that users could make a second call to CI2CMasterIRQ::StartWriteRead() while a first transaction is not done yet. For example when using this class for two different I2C slaves.
This would of course completely fail, but I am not safe-checking this right now in the code. It's just a matter of checking the m_nStatus flag at the start of the method, but that's not in the code I shared above :)

@sebastienNEC
Copy link

I can of course do these modifications if you'd like, or feel free to do them on your side as part of checking the code if you prefer :)

@rsta2
Copy link
Owner

rsta2 commented Feb 29, 2024

It would be a great help, if you would do the necessary modifications. It's also easier for you to do it, because it's your driver and you are familiar with it. So yes, please do!

@sebastienNEC
Copy link

Will do ! One question: the driver shares quite a bit of code with the regular CI2CMaster class. Is it ok to inherit from that class, in order not to duplicate code ? What's the general policy in circle ?

@rsta2
Copy link
Owner

rsta2 commented Feb 29, 2024

Thanks! I would think OOP here: If the IRQ-driven I2C master "is a" I2C-polling-master (which is the class CI2CMaster) then I would inherit it from the polling-master. But it isn't. So I wouldn't do that. There could be a common I2C-master base class for both classes, but I think their interfaces are different and I don't want to make it too complicated. The general policy is: Keep it simple! ;) Where inheritance really gives an advantage, just use it. But I wouldn't use it to safe some code here. I think, it's not that much.

@sebastienNEC
Copy link

Ok I've implemented the changes above, I think it's cleaner now.
i2c-master-irq.zip

@rsta2
Copy link
Owner

rsta2 commented Mar 4, 2024

Thank you! This looks good. I will test it this evening.

@rsta2
Copy link
Owner

rsta2 commented Mar 4, 2024

I wrote a simple test program using your driver, which sends some bytes to a I2C device and receives some bytes then. I tested it with a BMP180 air pressure sensor and it works well. I will add your driver soon to the develop branch. Thank you for this contribution!

@sebastienNEC
Copy link

Nice, hope it will be useful to somebody else !

@rsta2
Copy link
Owner

rsta2 commented Mar 5, 2024

I think so. It's merged to develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants