mikaelpatel / Cosa

An Object-Oriented Platform for Arduino/AVR
https://mikaelpatel.github.io/Cosa/
GNU Lesser General Public License v2.1
338 stars 76 forks source link

[BUGFIX] - Broken SYNC TWI SLAVE #509

Closed shahzadlone closed 6 years ago

shahzadlone commented 6 years ago

Modified this condition that was calling the completion callback and setting some variables for async and sync mode (in the same way).

if (m_dev->is_async() || m_status == SR_STOP)

The issue was that when we weren't using async mode m_status would still be true and go in the conditional block. In the conditional block setting m_dev to NULL and TWCR to 0 breaks the sync mode (but those are there for asynchronous mode).

So i took the common part:

m_dev->on_completion(type, m_count);

And moved it out of the conditional block (because it is needed for both sync and async modes, so it should always happen).

Then we just set m_busy to false (setting this to false has no effect on sync mode).

But as setting m_dev(NULL) and TWCR(0) breaks sync, so we only do them if it is async (i.e. is_async()).

Some asynchronous mode testing wouldn't hurt (please let us know if you have any rough async mode TWI examples).

mikaelpatel commented 6 years ago

This is a good start but there is still some details missing before async-request works as designed.

The API pattern for async should have a different approach to twi.release() as this is pushed into the future with async. Also async_request() as a too long life-span. It should be on a per call basis in the driver to avoid confusion, i.e. after an async_request() has been completed the state should fall back to sync_request().

For now, as I previously stated, async should not be used right now. There is too much missing.

shahzadlone commented 6 years ago

Thanks for the quick response! You are right that the API pattern for async shouid have a different approach, but our fix here is not for async mode (we are not using async mode at all), we are just trying to get the normal sync mode working (which this commit fixes).