Improve tca955x driver - (and add block GPIO access) #2420
Conversation
…ll return - constructor can not return null) and improved address validation. Fixed bug where exception was created but not thrown.
…eone signs up to get changes on the ioexpander lines. Only unregister when we are disposed.
…hat were signed up for.
… read/write of blocks of gpio pins.
…mise i2c traffic. Made Read/Write public and implemented IGpioDriverBlockAccess.
…strategy is to shift the i2c reading and event handler firing to another thread. We save one INT pin event if it comes while we are busy and then ignore any subsequent.
# Conflicts: # src/devices/Tca955x/Tca9555Register.cs # src/devices/Tca955x/Tca955x.cs # src/devices/Tca955x/tests/Tca9554Tests.cs
…ved unused code. Added to some comments. Protect add/remove event handlers. Corrected IsPinModeSupported, ioexpander does not support pull up/down.
… new non-blocking scheme.
src/System.Device.Gpio/System/Device/Gpio/IGpioDriverBlockAccess.cs
Outdated
Show resolved
Hide resolved
| { | ||
| _controller.SetPinMode(interrupt, PinMode.Input); | ||
| } | ||
|
|
||
| // This is only be done once as there is only one INT pin interrupt for the entire ioexpander | ||
| _controller.RegisterCallbackForPinValueChangedEvent(_interrupt, PinEventTypes.Falling, InterruptHandler); |
There was a problem hiding this comment.
_controller can still be null at this point (if _interrupt == -1), so this would crash.
There was a problem hiding this comment.
_controller = gpioController ?? new GpioController(); This was in there before I started modifying the driver. Now that I look, I see that the parameterless constructor of GpioController does not return a working GpioController. I'm not sure what the point of it is. Should we just insist on a non-null GpioController if interrupt is not -1?
There was a problem hiding this comment.
I made the construction requirements more explicit.
There was a problem hiding this comment.
new GpioController() returns a working copy on the Raspberry Pi or some flavors of linux only. On other systems, this in fact returns a dummy instance. At the moment, all bindings have this fallback behavior if gpioController is null. I'm not a fan of this either, but it's been there from the beginning and it would be a major breaking change to make that argument mandatory.
There was a problem hiding this comment.
Okay, so It should just make a default GpioController and let it fail elsewhere if it is not actually able to get events from that pin. I'll change it back. I agree we should endeavour to not break the existing API. I'm wondering if I should actually drop this PR and fork the TCA955x driver. There are a couple of other i2c gpio drivers that are very similar already. Perhaps we should make a more generic one and people can migrate to it if they want the better behaviour and bug fixes? The new one could demand a working GpioController in the ctor right from the start. I would probably get rid of the locks too.
There was a problem hiding this comment.
No, I think it's fine like it is now and we should merge it. You can still propose a new set of changes later, but this is already an improvement.
And yes, there are a few very similar devices (namely the MCP23xxx series). I haven't checked whether they could be unified, though.
| if (_interruptProcessingTask == null) | ||
| { | ||
| _interruptProcessingTask = ProcessInterruptInTask(); |
There was a problem hiding this comment.
The code is very complex and it's hard to imagine all possible execution paths. (Note for instance that the started task will just run to the lock initially, having to wait for this method to end, which requires at least 2 task switches)
I think this will be simpler if just the callback happens in a task, but not the readout of the new values. I don't think that would perform worse than this solution (if we aren't fast enough to read out the values after each interrupt, we're lost anyway).
There was a problem hiding this comment.
Thank you for looking through this. It is complex, but the problem is that the i2c reading is very slow relative to the rate that the ioexpander chip can generate interrupts. Any pin-toggle can cause a new interrupt even if the chip has not been read to service the last interrupt. I think this is bad behaviour of the chip but I can't change that. This is not an academic exercise, I'm watching the scope and trying to make the driver behave better when there is a noisy pin on the ioexpender. It has made a huge difference - the i2c now only gets 4 transactions (16 bit i/o expander) for a short burst of input toggles. Previously the interrupts would queue up, resulting in a whole heap of unnecessary i2c reads. I'll move the mirroring of the pins and values out of the task (they will be added to the closure for the task lambda). That will remove the initial lock in the task.
dotnet-policy-service
bot
added
the
Needs: Author Feedback
|
/azp run dotnet.iot |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dotnet-policy-service
bot
removed
the
Needs: Author Feedback
|
@pgrawehr oh dear... I've broken the building of several other GpioDriver subclasses. I've changed their methods to be overrides and removed duplicate code from seesaw. It is suspicious to me that the same signature was sprinkled around in some of the drivers. Either they were cut and paste implementations or there used to be a block-gpio api and it got ripped out at some point. I haven't been through the history to check. |
|
@felixcollins Thanks again for this contribution. Feel free to create new PRs for further changes/refactorings/improvements. |
This PR has numerous bug fixes and improvements to the TCA955x driver.
In particular, the handling of the INT pin on the chip is vastly improved. The event handler for the falling edge on that pin now delegates to a Task. This does the work of reading the chip via i2c and calling any signed up events for the pins of the chip. While all this is going on, if there are any more than one further falling edge of the INT pin, they are ignored. If there is at least one then the check of the chip input registers and event firing is redone at the end. This behaviour prevents blocking the GpioDriver that is servicing the INT pin. Blocking could (especially in the case of libgiod v1) cause a queue up the events and subsequent delivery of them long after the actual inputs on the chip have settled.
The TCA955x driver now just does the one or two reads of the i2c that are needed, when the INT pin asserts, rather than repeating that read for every pin that has an event handler. Similar to this change, the GpioController now does a block read of its GpioDriver if the GpioDriver implementation supports block reads of I/O by implementing IGpioDriverBlockAccess. (This should probably be in a separate PR - sorry about that)
Other fixes:
Report event type of pin in event rather than reporting the types signed up for.
A few more tests and fixed some redundant/incorrect tests.
Fixed reporting of supported pin types.
Fixed pin type of INT pin to support Input or InputPullup and default to Input.
Improved the block io read/write to reduce i2c traffic
Fixed incorrect register address constant
Spelling improved
Microsoft Reviewers: Open in CodeFlow