Wire Slave: arbitrary user code is called from ISR

[greyltc]

When a user is using the Wire library in slave mode, they generally need to register a function to handle requests for data from the master. They write some function, which must contain Wire.write and register it like this: Wire.onRequest(someFunction);.

The issue here is that someFunction gets called (via twi_onSlaveTransmit) right in the slave's interrupt service routine:

ArduinoCore-avr/libraries/Wire/src/utility/twi.c

Line 528 in 58081c0

twi_onSlaveTransmit();

First of all, the documentation for Wire.onRequest is truly tragic. It doesn't even tell the user they must make a call to Wire.write or else the wire library will decide for them to send 0x00 to the master, and it doesn't warn the user that they're writing code for inside an ISR.

Secondly, this architecture raises all kinds of potential usability issues. I think quite often a slave will need to do something much more complicated to answer a master's request than what should/can be done in an ISR. As I understand the code (please someone correct me if I'm wrong), it's impossible to answer the master outside of the call from the ISR. What if the slave needs the delay() function in forming its answer for the master? Impossible. What if the slave needs to fetch an answer for the master over SPI? Maybe that's possible, but it sure seems like a terrible idea to do that from inside an ISR.

I noticed a stub:

ArduinoCore-avr/libraries/Wire/src/Wire.cpp

Lines 271 to 274 in 58081c0

	void TwoWire::flush(void)
	{
	// XXX: to be implemented.
	}

Maybe the point of that was to help solve this problem?

Would you consider accepting a PR I might make so that a slave can form an answer to a master outside of the ISR?

[greyltc]

I think my issue#342 branch solves this. Need to do a bit more testing to check if this is backwards compatible with the Wire library API before this change.

[Ragebone]

@greyltc

As I understand the code (please someone correct me if I'm wrong), it's impossible to answer the master outside of the call from the ISR.

I understand the current implementation like that as well.

I think my issue#342 branch solves this. Need to do a bit more testing to check if this is backwards compatible with the Wire library API before this change.

Have you had any progress on this?
After a brief look, i think this should be backwards compatible since your additional handling is using the previously unused return-value of twi_onSlaveTransmit

Do i understand correctly that you disable TWI Interrupts to cause clock-stretching?
And then enable them again on flushing?

One of your commits for reference.

I think quite often a slave will need to do something much more complicated to answer a master's request than what should/can be done in an ISR.
[...]
What if the slave needs the delay() function in forming its answer for the master? Impossible.

I kind of disagree because i had issues with "excessive" clock-stretching on an Arduino Uno.
I was implementing the i2c protocol of an IR35201 which only requires to answer a single Byte write with a single Byte from a Register.
All the needed information is already present in the Wire.onReceive() registered method.
So Wire.write(outByte) was the only operation i was needing in the onRequest() registered method.
But the call to an empty twi_onSlaveTransmit() was enough to cause the last two Data-Bytes to be stretched consistently.
To me it was an issue because the used Master would error the moment clock-stretching occurred.
After a lot of digging, i was only able to reduce it to sporadic clock-stretching.
Because of that, i am under the impression that as long as the ISR has not finished, the TWI Hardware will clock-stretch.
S that makes me think that you can do what ever you want in the twi_onSlaveTransmit() method.

[...] , but it sure seems like a terrible idea to do that from inside an ISR.

I have no idea if that is a bad idea or not.