add some cautions and an example to Wire documentation around use of sleep

[ObviousInRetrospect]

The prior description of Wire.slaveTransactionOpen() left out a bunch of pitfalls on correct use.

[SpenceKonde]

@MX682X @ObviousInRetrospect is this still relevant? If so, please explain what the conclusion was to me and I will write the note in the docs, that note is inconsistent with the documentation style of the core and some points remain unclear.

We shall state that wire cannot be used to wake from sleep in versions prior to 1.5.0/2.6.0 (or rather that any arbitrary and undesired behavior is not considered a bug and that if they want support please upgrade to the version that offers it. Nobody uses idle sleep because it sucks just as much as it did on classsic avr, there is no need to mention that that worked, nor is this the appropriate place for instructions on upgrading specific pieces of the core. No instructions of that sort belong in the core, period regarding making partial upgrades, as that is fully unsupported and we provide no guarantee that a library version of any library supplied with the core from any given version of the core is compatible with any other core version. We should be sure not to imply that that is a supported configuration to use versions of any included libraries from any version other than the one it came from. The instructions should be provided only explicitly in issues and discussions for specific cases for which they are known to be safe. Changes requiring both modifications to core and library simultaneously are routine and it is the exception not the rule that library versions can be mixed and matched like this, it should not be in the docs, because what is true this version may or may not be true next verision. ]

Use of sleep mode power down is ubiquitous in code all over the place.

use of sleep mode standby is in existing code is essentially absent because people have been slow to recognize that it got turned from worthless on classic AVR to as as good as power down except with massive feature additions on tinyAVR and nearly as good on Dx-series and because the sleeplib planned for 1.1.0 of mTC is still not written (and needs very tight integration with the core. It can only be written be by me realistically speaking, if the advertised features are to be present. and to work in the next version.

[MX682X]

Based on the fact that a test with my suggested changes did not generate a single error in 16 hours, I don't think slaveTrasactionOpen() will require the use of sei()/cli() and I think we should leave it out to keep things simple. I don't see any reason in using an old core with a new version of the Wire library either. It seems to be more trouble in making sure that there is no accidential override then upgrading the whole core. But assuming there is some company that let's say tested the 1.4.10 and wants to use the Wire sleep fix, they should be able to figure things out themselves, since they are making money from it anyway. Keep the documentation clear and easy for the normal people, for normal usecases out there.

[ObviousInRetrospect]

I was still able to reproduce the problem in sleep mode power down without cli/sei a few times but also had stretches where a million in a row passed. Given the result is a slave that goes to sleep permanently until reset I think it is worthwhile. Especially since it will be basically impossible to track down.

with the cli/sei I am up to 5.6 million without error:

CRC valid
fail: 0 pass: 5675787

I think the way to make this simple if we want to is to add Wire.sleepIfSlaveTransactionNotOpen() and bury the ugly code there.

[ObviousInRetrospect]

FWIW I did some testing and found a minor but non-zero power difference between IDLE and Active but agree that IDLE sucks. I think it was somewhere around 400ua. Are you sure standby sees little use? It is pretty obviously good and what I normally use. In mTC you already support millis() on the rtc.

Everything about this also applies to mTC.

@MX682X in your 16-hour test was the compiler inlining or calling the slaveTransactionOpen() I thought yours inlined because you compiled 1xWire and mine did a call because I compiled 2xWire.

I updated the pull-request to remove the objectionable directions (you are still welcome to rewrite it). I still think it would be better if we added a function in Wire that combined the check and the sleep (Wire.sleepIfSlaveTransactionNotOpen()) instead of explaining how to safely avoid the race condition inherent in them being separate.

[MX682X]

was the compiler inlining or calling the slaveTransactionOpen() I thought yours inlined because you compiled 1xWire and mine did a call because I compiled 2xWire.

Just double checked, both Wire objects are used, but the compiler optimized both calls to slaveTrasactionOpen() with an rjmp. This is with my changes to Wire (see the .zip) and this code:

while (!wake && Wire.slaveTransactionOpen()) {
    delay(1);
  }
  Serial.flush();
  while (!wake && !Wire.slaveTransactionOpen()) {
    sleep_cpu();
  }

[ObviousInRetrospect]

I can't explain why our compilers are generating different code. I'm using the stock one DXCore installs for OS X. But it is a reasonably horrible to debug failure mode (the slave dies silently extremely rarely) so you might have noticed my proposed guard code uses inline assembly to be 100% sure the compiler doesn't fuck it up. That might be overkill vs sei(); sleep(); but I am absolutely convinced the cli/call/sei/sleep sequence is both correct and necessary. When slaveTransactionOpen() is a call I can repro the problem (rarely).

If I hadn't added the boneheaded serial step which you noticed I don't know that I ever would have tracked down the remaining failure every few hundred thousand requests.

I'm not sure I buy the text that sleep wasn't a feature in 1.4.10 given the pre-existing text around using slaveTransactionOpen() before sleeping.

[ObviousInRetrospect]

And stopping the test at 12.2 million cycles without a single error. This is fixed.

CRC valid
fail: 0 pass: 12246236

[SpenceKonde]

Fireworks and champagne for the both of you.

Okay is this all we gotta merge?

And what about megaTinyCore?

[SpenceKonde]

No this is just docs ....

[ObviousInRetrospect]

Same fix applies to mTC per MX682X

I just killed wire/src and replaced with his final zip in the bug

[MX682X]

You can also find the updated Wire library in my fork of DxCore

[ObviousInRetrospect]

Coming back to this for a minute. It is totally obvious to me that sleeping while a slave transaction is open will trash that transaction. It is less obvious to me why sleeping while a slave transaction is open trashes all future use of the slave until RST. Is there a way this could be made to fail more gracefully?

[MX682X]

open trashes all future use of the slave until RST

This is because the slave keeps sleeping. It should be enough to wake the slave up. The reason why it's trashed is because the slave module waits for a command (send NACK or ACK)

[SpenceKonde]

Okay so what needs to get wire working properly here Do I just take what was PR'ed for mTC and copy it over here or is there more to it?

[SpenceKonde]

I think this is sorted out in current wire docs on megaTinyCore which will be ported here.