master+slave Wire/TWI code seems to break on 1.5+ core version, worked up to 1.4.10

[jl-lewis]

I have a program that uses default Wire as master and slave on a multi-master bus and also uses Wire1 as standard master only on a separate bus. The device is a DA 32-pin.
So I'm setting the "2x master AND slave" options. I then have many functions that use Wire or Wire1 to perform tasks and then a main controller also periodically operates as master to communicate with the DA.

This has functioned generally well for a while with multiple devices, and then when testing recently broke significantly. After a lot of checking and trying things, I realized it was a result of updating to 1.5+ core version, including latest 1.5.x. Switching back to 1.4.10 has everything working again, and it's repeatable.
It appears that the default Wire peripheral still responds to transmissions as a slave device, but is not functioning as master.
Wire1 peripheral only operates as master as expected and appears to work normally.

Initializing as below (simplifying):

#define SELF_ADDR 0x10

void setup(){
    Wire.swap(2);
    Wire.usePullups();

    Wire.begin();
    Wire.begin(SELF_ADDR, 0, 0);

    Wire1.begin();
}

I do have receive/request callers:

    Wire.onReceive(receiveEvent);
    Wire.onRequest(requestEvent);

Then many functions are writing to or requesting from devices using beginTransmission(), write(), endTransmission(), or requestFrom() in the usual way but are not working due to no master operation. I'm not using any special device libraries for any I2C devices, all the functions are within my multi-file project. I'm thinking there's an issue with how the peripherals or library are being initialized to function as master, unless there is an issue with the above master operation functions not being called correctly? I started to look through the library a bit and try to do comparisons between the versions but I know there was a major change to the Wire library so there's a lot to dig through. So far it appeared that a lot of function overloads were removed but I could be misunderstanding the diff results I was looking through.

If anyone could point me in a direction to work out what could be going on and how to address it, much appreciated.

[SpenceKonde]

@MX682X Any thought? I'm not exactly sure what the system that is causing the problem really looks like from above.

[SpenceKonde]

If this is a regression in 1.5.x, it is CODE-RED-CRITICAL considering who I know is used this for what, and I would have to back out these changes if we cant either determine that the users code is defective or correct whatever regression is in the code. Like, this code is being used by microchip to promote their products, so regressions not only make me look bad, they make a lot business users look bad.

[MX682X]

@SpenceKonde looking at the TWI registers in a debugger, it seems Wire works fine. So a Simple CODE-RED tops

[MX682X]

@jl-lewis Could you try to exchange to put the Slave begin() in front of the Master begin()? I'd like to know if the slave begin is overwriting something

[jl-lewis]

OK, tried switching the order and did not make a difference.

[MX682X]

I used the code you provided to look at the results in a debugger. The Wire registers are all set correctly. What error code does Wire.endTransmission() return?

[SpenceKonde]

Can we get a schematic of simplest setup that manifests this bug?

[jl-lewis]

OK, I will work on getting setup to get the error codes out by serial probably. I will have to piece together schematics into more of a system overview schematic showing the buses and devices. The MCU is pretty much a minimalist setup on a castellated module, with just GPIOs and TWI connections, that attaches to a larger board that's part of a larger system. 3.3V, internal oscillator.

In the meantime, I'll attach the output of a compile that may or may not give any clues, in case you see anything unusual.
This is the output with 1.5.4:
https://gist.github.com/jl-lewis/2ce76e05dec2ce1dd3379da3ca97a4cc
I noticed that there is a warning for the portmux variable being unused, which I don't see with 1.4.10. This only seems to relate to TWI1, however.

[SpenceKonde]

There is?

There absolutely should not be warnings generated by examples, included libraries, or the core. Please let me know the error you get.

[jl-lewis]

The gist is the compile output so if you mean from that:
C:\Users\jlewis\AppData\Local\Arduino15\packages\DxCore\hardware\megaavr\1.5.4\libraries\Wire\src\twi_pins.c: In function 'TWI1_ClearPins': C:\Users\jlewis\AppData\Local\Arduino15\packages\DxCore\hardware\megaavr\1.5.4\libraries\Wire\src\twi_pins.c:518:13: warning: unused variable 'portmux' [-Wunused-variable] uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm; ^~~~~~~ C:\Users\jlewis\AppData\Local\Arduino15\packages\DxCore\hardware\megaavr\1.5.4\libraries\Wire\src\twi_pins.c: In function 'TWI1_usePullups': C:\Users\jlewis\AppData\Local\Arduino15\packages\DxCore\hardware\megaavr\1.5.4\libraries\Wire\src\twi_pins.c:632:13: warning: unused variable 'portmux' [-Wunused-variable] uint8_t portmux = PORTMUX.TWIROUTEA & PORTMUX_TWI1_gm; ^~~~~~~

[SpenceKonde]

Looks to me like that is the cause of the problem, I will look at it tomorrow

[MX682X]

The only reason the variable is unused is because it's a 32 pin chip with no real choice on the pins for TWI1. Best way to fix is to do || (defined(TWI1_DUALCTRL) && defined(PORTB)) I think

[jl-lewis]

This is the basic schematic of the MCU module:

PF0 interrupt, TWI0 alternate on PC2/PC3, and TWI1 on PF2/PF3, and then PA2-PA7 are general digital I/O.

The main system controller is a PC with USB-I2C interface, and connects through an I2C bus switch array to multiple MCU boards.
That looks something like this in somewhat simplified form:

The PC can switch boards on/off via upstream I2C GPIO and can set the interrupt to an MCU to get exclusive control of that module's bus. This is necessary because the I2C bus switches are unintelligent and do not monitor or handle behavior based on downstream bus status. If the bus is busy when it connects, it will cause a crash. The interrupt is set for the MCU so that it inhibits master functions, the current loop operations will complete but the next loop will block master operations, then after some delay the PC controller connects and initiates transfers. I'm not doing any end() or messing with interface setup, just not initiating master operations at certain times. So it's not really a normal multi-master situation. Either master can share the same bus but arbitration shouldn't occur since bus control is exchanged by an interrupt status. In hindsight, there are better ways to build out a system like this but it's legacy that I have to stick with at this point, and there are mechanical/layout reasons for it to be done this way as well.

Generally, there is a fast task loop that's 20 ms and slow task loop that's 500 ms, which is just done by gating with timed if() statements using millis() tracking. The PC acquisition happens asynchronously at usually 5-30 second intervals and takes about 100 ms or a bit more.

All that said, this has worked well for some time with essentially the same codebase on ArduinoCore-avr with a 328P and then of course with DxCore 1.4.10 and prior. With only 1 TWI with 328P, I have the ability to operate with just a single bus for all devices if necessary. I was trying out the capability of going with 2x TWI to free up bandwidth on the main bus and also physically separate the bus into smaller ones potentially. Another "in hindsight", there are software I2C libraries that would probably handle my use case fine and be more portable code.

I made another gist with a more complete listing of the code, overall there are something around 1500 lines between all the files so I'm trying to give a condensed version of it without stripping out something that might be relevant to what's going wrong. This is not expected to compile and I haven't tried yet. It's missing most function definitions/libraries and variable declarations to keep it simple for now.
https://gist.github.com/jl-lewis/259d8181c9f106b088d129a685e1b804
I could at some point try re-working it into an example that could be compiled to test the essential functions and demonstrate the issue with a simple codebase, but of course it depends on hardware available as to what level of testing can be done (for someone else).

It does appear that communicating with specific TWI devices in the setup() section is successful, as I'm able to physically check the GPIO outputs and also get valid data from the ADC. So at that point, master operation is working, then I don't know what is altering the behavior after that point.
I checked the return value for various instances of endTransmission() and I'm always seeing 0, which should be success. I don't see how that's possible if there is no response, as released/high SDA should be read as NACK I believe, and the reads of devices are all solid 0xFF.

Is there potentially any state the library or the peripheral itself could be put in at a later time, that it hangs or stops operating normally as master but slave operation still continues? Coming back to the fact that it seems to still operate correctly as slave, but no longer seems to be successfully performing master operations, neither writing bytes to slave devices or reading data back from them. Perhaps something with not driving the clock output correctly when switching to master which would result in devices just not responding with any data bit changes? I'm assuming the clock stretching support (talking to slaves that can stretch included) is there as well, as I've always counted on having that or some devices won't work correctly, and it seems that should still be supported from reading the documentation.

I'm sure the unusual use of an interrupt to gate master operations would be suspect, but I can confirm that has worked normally with the other platforms and versions. There's some more testing I can probably do with that, but this is also something I'd like to eliminate from working example code as well. I have used an LED to see that the interrupt is triggering and reading the pin state correctly, but have not dug deeper into the masterState variable state and function execution yet.

Obviously I have a fairly complex project and I'm trying to reframe my approach to figure out this issue, but I'm starting to see I may need to just start a new approach with a basic example code and see if I can replicate the issue. I want to do what I can to make sure there's not something weird I'm doing that happens to be interacting, but either way there is some kind of interaction. I will try to take some time to build a much simpler approach and see if the discrepancy still exists.

[SpenceKonde]

@MX682X - does this give you enough to go on? This is way out of my league.

It is also a very high priority bug as it likely effects both cores.

[MX682X]

@jl-lewis I've got to admit that I have no idea yet what might cause the issue - the only thing that was changed between Wire versions had something to do with sleep and slave operation, as far as I remember.
At points like this I have to ask you to take the Wire Library from 1.5.x and replace the one in 1.4.10. There were some other changes of the core, I'd like to make sure that I'm searching in the right spot.
Furthermore, the Wire library has some guards that make sure the bus is ready, including a timeout check, so if there is something wrong, it should not return 0.

Also, do you have an oscilloscope? If so, could you try to monitor the TWI0 bus?

Also, feel free to post the entire code, It's always easier for me to have the entire picture. At least as far the company policy alows, if there is any.

[SpenceKonde]

the only thing that was changed between Wire versions had something to do with sleep and slave operation, as far as I remember.

That's not exactly true. Between your handing it off to me, and release, two things were additionally changed.

1. Classes and multiple files are toxic to the optimizer, including the part that does constant folding and the __builtin_constant_p stuff that comes with it. The use of those builtins had to be passed off to a wrapper for swap in order to correctly generate a compile error when they try to swap to an illegal value. As you had refactored it, they were not seeing the constant arguments and never generated the compile error (which is of course bad when it's a simple easy problem to flag, but there's no exception handling to communicate that to the user).
2. A method (or "member function" if you prefer) was added for the purpose of handling all those stupid little options like the voltage levels on Dx, SDA hold time, crap like that. The implementation is insanely simple - it's mostly meant to ensure that a user who knows what those options do and that he needs one can enable them in a way that the core doesn't counteract. And because I was writing about the TTL voltage level option, and started to say this is like the SMBus voltage level option and went to give people a pointer to it in the wire docs, then saw that there wasn't any way implemented. .

I doubt either of my changes broke anything, but it is not impossible.

[MX682X]

As the first Master interactions work, the only way those changes can affect anything is when swap is used after .begin, which does not seem to be the case

[jl-lewis]

Well, I have access to a Saleae analyzer, so I did some captures with both core versions and definitely see the different behavior. The first place I see a difference is when I call the function below, via loop() --> updateCurr() which calls setDACMulti() and this sends a new array of values to an I2C DAC.

//***************************************************************************************
//set current setpoint DACs
int Load::setDACMulti(uint16_t dacValue[]){
  // return 0 : success
  // return 1 : wrong dac index
  // return 2 : value over range
  // return 3 : DAC 1 transmit error
  // return 4 : DAC 2 transmit error

  uint8_t ch_MSB = 0;
  uint8_t ch_LSB = 0;
  
  // Serial.print("SET DAC _");
  // Serial.print(dac, DEC);
  // Serial.print("_ to: ");
  // Serial.println(dacValue, DEC);

  Wire.beginTransmission(DAC1_ADDR);

  for(int i=0; i < 3; i++){
	  dacValue[i] = (dacValue[i] / 5) << 2;
	  
	  if(dacValue[i] > 4095){
		  //Serial.println("LD OVRNG,SET 0");
		  return 2;
	  }
	  
	  ch_MSB = dacValue[i] >> 8;
	  ch_LSB = (dacValue[i] & 0xFF);
	  
	  Wire.write(ch_MSB); //upper 8 bits
	  Wire.write(ch_LSB); //lower 8 bits
  }
  if(Wire.endTransmission(1) != 0){
	  return 3;
  }
  
  Wire.beginTransmission(DAC2_ADDR);

  for(int i=3; i < 6; i++){
	  dacValue[i] = (dacValue[i] / 5) << 2;
	  
	  if(dacValue[i] > 4095){
		  //Serial.println("LD OVRNG,SET 0");
		  return 2;
	  }
	  
	  ch_MSB = dacValue[i] >> 8;
	  ch_LSB = (dacValue[i] & 0xFF);
	  
	  Wire.write(ch_MSB); //upper 8 bits
	  Wire.write(ch_LSB); //lower 8 bits
  }
  if(Wire.endTransmission(1) != 0){
	  return 4;
  }

  return 0;

  
}

With 1.4.10, this looks like I'd expect, you can clearly see the frame with 3x 2-byte values (repeated values in this case):

And one with analog capture (sorry, did this after but it's also zoomed a bit more):

And with 1.5.4, I see a start, address + write, ACK, then stop. Then repeats for other devices as well, just start, address byte, stop:

The write to 0x41 is another section of code, but is another ADC I've shown code for before. It's in this case a write-read using a restart but same behavior, and since the initial write of register value doesn't happen, the read doesn't return expected value.

Again with analog capture as well, on 1.5.4:

I certainly wouldn't try to pretend I have a clean and beautiful waveform, capacitance is certainly high and there is plenty of crosstalk, but it's generally had enough margins to work well and based on what I know of the timing and when SDA is sampled, etc., it should be pretty well tolerated. It certainly can cause arbitrary transaction failures, particularly when other system noise is added, but I'm operating with most power circuits off for all of this debugging, just logic supply running.

As a bigger picture, 1.4.10 from program start:

1.5.4 from program start:

Setup() happens and that's the first cluster of transactions, nothing unusual here. D2 is the interrupt from PC controller so the MCU is responding to those requests only during the time that is high, also looks OK. Then the last cluster is where I highlighted the problems above. D3 is the UART output for debug.

[MX682X]

Thanks. The graphs helped me a lot, as I found my Typo:
twi.c, line 637. _toggleStreamFn should be set to 0. (Copy-Paste does it again).
this variable switches the access between master and slave array for writes, reads, available. When it's 1, all writes go to the Slave part, and this results in the "empty" Master write after the first slave transaction.

[lauritsriple]

I have the same issue. Tested v1.5.4 with the suggested patch. Setting the _data->_bools._toggleStreamFn = 0x00; and it works fine here.

[jl-lewis]

I made the change at line 637 and now the behavior is correct. I could have probably saved us a lot of time by skipping right to the bus analyzer! Oh well, just glad a simple fix was found.

[MX682X]

Thank you for checking, PR is open with the fix. And Sorry for the all the trouble....