Using Wire library from DxCore (or megaTinyCore) with other cores, master + slave simultaneous functionality

[jl-lewis]

I wasn't sure the best place to discuss, as I know this was discussed at MegaCoreX MCUdude/MegaCoreX#134 and that person seemed to get it working successfully. I know it was suggested in more than one discussion to incorporate this library into MegaCoreX, for various features and in my case for the master+slave operation. The maintainer doesn't seem to be interested in doing that since it was quite some time ago it was suggested. I was really surprised to find that neither ArduinoCore-megaavr or MegaCoreX support the M+S capability, since the standard AVR core has supported it for as long as I've used it (many years).
This issue arduino/ArduinoCore-megaavr#66 had discussion explaining a method for adding that functionality back as well, and makes that sound like a fairly simple way.

So where I stand with it is that I replaced the MegaCoreX Wire with the one from DxCore and added:

  void badArg(const char*) __attribute__((error("")));
  void badCall(const char*) __attribute__((error("")));
  //#define TWI_MORS
  #define TWI_MANDS   /* Comment out above and uncomment this to use wire as both master and slave using the same TWI instance on the same pins */

at the top of Wire.h, specifically the TWI define has to be before twi.h include it seems (took a little time figuring that one out, however obvious it should be).
Also corrected the very recent _toggleStreamFn issue in twi.c, since not released with that yet, per: #396

That got me to the point of linking, but there was an issue with an asm directive for pointer/register manipulation used in HandleSlaveIRQ, in some versions (mTC I think) it's done as a macro _fastPtr_y to do something same/similar and that macro is defined in dirty_tricks.h. Using the files from DxCore, this asm is written directly in the function. At first this did not appear to work because I think just swapping files did not make them appear as "touched" so I still got the "undefined reference" to the macro when linking, but doing a touch and rebuilding seems to have worked. That sorta happened in the midst of writing out some of this, so the simple answer may be to just use the direct line, but thought it's still worth bringing some attention to this for the benefit of others and see if there's any reason to do something like the macro in this case or information to add for doing this. I have not yet tried to program and run a device.

[SpenceKonde]

Yah I have no idea why he won't steal the DxC/mTC wire library. That you can't do both slave and master even on the same set of pins when the stock arduino core can is particularly egregious (you'll note that I implemented support in twipins for the megaAVR 0-series too, not just the parts I support >.>). Though i'll be the first to admit that making the simultaneous master+slave work was hard - in fact I had tried, failed, and eventually gave up. It was still crap (though the baud rate calculation wasn't as wrong as the stock core - though it was still quite wrong) when @MX682X came along and pretty much rewrote the whole thing. You may have noticed the binaries come out smaller too :-P

fastPtr_y is just a way to bully the compiler into generating better code, because the two asm nerds wizards here (myself and MX682X - I don't think we have any others at the moment) kept running into places where C would be just fine.... except that the compiler made moronic register allocation decisions. And asm takes too long to write to use it everywhere - so you want to write the code in C but trick the compiler into not doing something irredeemably awful. People complain when I put assembly in places that don't really need it too, because it's hard to read (I can't say I agree, unless you let the compiler generate it without intermixing the C code use that. Which is what the official Arduino core does for pulseIn() - now THAT was hard to read. I could probably do it now without needing to take such extreme measures, but at the time I printed the whole file out, and marked it up with a rainbow of colored pencils (to trace which things were effecting stuff where. Because it had a bug in it, which had been present I think since pulseIn was first implemented. (The reason they included compiler generated asm is that the new compiler version (5.x instead of 4.3) did a much worse time optimizing the loop and it no longer gave accurate times. But it never honored the timeout before the pulse started, until I found the problem with the asm and fixed it for the cost of 2 instruction words, which I reclaimed by spotting a missed optimization. That bug may very well still be in the official core, actually. Before that, it was written in C and the time per iteration of the loop had apparently been determined empirically).

As we all know the compiler is nearly immune to hints and even forceful nudges towards making better code. To change it's dumb decisions often requires nothing short of a knee to the groin will do it, in the form of near-do-nothing asm statements that simply force it to allocate registers less badly; the main reason for this is when it's getting into a place where it's generating adiw-ld-sbiw-adiw-ld-sbiw typer sequences, because the higher level code assumes the three pointer registers are equal, and has generated virtual instructions to do displacement with X. X can't do displacement, so it converts these into an adiw (add immediate to word), then a load, then sbiw (subtract immediate from word). There is no later pass that removes two consecutive instructions where one directly undoes the effect of the first, so you get that 3:1 size and time multiplication instead of 2:1.

Building pointers in a real hurry is also a common thing it's bad at, because you can often use assumptions that it can't legally make to simplify it (for example, you know that the PORTx structs start at 0x0400 and are 0x20 bytes long, so you can go from a port number to a pointer register pointing to that port in just 3 clocks. I forget how many the compiler takes, but it's too stupid many.

And multibyte datatypes and bitshifts are often a disaster too. (I used semi-automated means to make all 60 xorshift random number generators with 16 bits of state which had a full 2^16-1 period with assembly, so I could test which ones generated the best random numbers (got 15 really good ones, though I ruled out some of them based on concerns about patterns my tests might not spot). It was doing some of the stupidest things I've ever seen avr-gcc do for certain shift amounts, like copying two registers with movw, xoring one of them with itself, copying the other byte of the word to that zero, and then zeroing that byte - when the thing itwas going to do at the end was an xor, and anything xor'ed with zero is left unchanged... AND IT STILL DID THE XOR OF THE SEED BIT WITH THE REGISTER IT HAD JUST ZERO'ED!
Like, really? The whole bloody reason I made an xorshift prng is that if I used Arduino's random function, it would require over 300% of the execution time budget I had for all of the pattern generation just generating random numbers, Arduino's random() takes 700someodd clocks, because it's based on avr-libc's random number generator, which for some stupid reason has A THIRTY TWO BIT DIVISION in it. and I didn't need 32-bit values anyway, as I was slicing each random number up a few times because I only needed to select from a small number of options with it. The winning xorshift algorithm takes 29 clocks.

Now granted, that project probably already is a rolling disaster - the board is on Rev. G, and there will be a Rev. H, and like a dozen of the earlier rev's were burned out before I found out what was happening to them, and I have killed at least 5 strings of 2811's, probably more...(I mean, I have a bin with a bunch that don't work and I don't know why. In one case I found the bad LED, cut away the potting material and was able to spot the crack in the IC where it had been split open by an overvoltage event that would have let smoke out but for the potting material) But if it's a rolling disaster now, imagine how bad it would be if I needed to have 5 times as many controllers because they were all spending the majority of their time generating random numbers.

I know very well how bad the performance with 32-bit division is. One time on ATTinyCore, I'd let something end up not getting optimized away... in micros(). People were understandably not thrilled when micros was taking about 100 us to return. (They also weren't happy before, because it was giving the wrong answer (not a little bit wrong either - I think the worst one was like 33% slow or something godawful like that) for some clock speeds... I tell you, there's just no satisfying people.... You would not believe the amount of time that went into testing and tweaking millis() and micros() on mTC (and by extension DxC, since it got the same code) to eliminate the cases where micros would fall behind millis, and then catch up at the end of the millisecond, and generally eliminate every single instance of timetravel in either direction. I've quantified the micros error for all 1000 values that the first 3 decimal digits can take (it repeats after that) to make sure that there was no time travel, sometimes having to change it because when it rolled over from 999 to 1000 it would jump backward 3 microseconds. And it was then reimplemented in asm because even the bitshift division was slower than it had to be and micros is one of those functions where you really do care how long it takes to execute, yaknow? and even a single microsecond of backwards timetravel beyond the execution time of micros (which may be quite fast, especially if you're overclocking the hell out of it) is enough to have catastrophic effects (every time delay hits the backwards jump, an extra millisecond will go by as far as delay is concerned)

[MX682X]

the main reason for this is when it's getting into a place where it's generating adiw-ld-sbiw-adiw-ld-sbiw typer sequences, because the higher level code assumes the three pointer registers are equal, and has generated virtual instructions to do displacement with X.

Actually, not quite right. The code does not assume anything. In fact, it has no idea how many registers there are and how they can be used. The register allocation is all done by GCC. And you can tell it to not use adiw-ld-sbiw by using "-mstrict-X". it might generate bigger code though as it still avoids Y @SpenceKonde

and fastPtr_y stopps GCC from "optimizing" constant memory accesses (because at some point it is still addr+const, and it thinks, let's avoid this +, make a const out of it), and sts/lds is 4 bytes wide. Technically, accessing a simple variable is faster, but if there are more then 4 accesses, it is faster to load an address in the displacement register (4 bytes overall) - and this macro is telling gcc to do exactly that

[jl-lewis]

I think I may be stuck on the pinswap issue actually, as far as using this with MegaCoreX. Using swap(0) or swap(1) compiles, but swap(2) doesn't.
I get (at linking I think):

In function 'swap.constprop',
    inlined from 'setup' at [removed path]\bib_main.ino:210:14,
    inlined from 'main' at C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\cores\coreX-corefiles\main.cpp:38:8:
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\Wire.cpp:170:19: error: call to 'badArg' declared with attribute error: 
             badArg("The requested swap level is not available on this part.");
                   ^

which seems to occur if PIN_WIRE_SDA or variant is not defined?
There are really only 2 pin swap options for the 32-pin 4808 as I understand it, since there's no TWI1, so I'm not sure how to correctly set those options.

[SpenceKonde]

Oh god a bunch of stuff is busted in there....

[SpenceKonde]

(latest version should fix issues when moved to the mega0's

[jl-lewis]

I believe there are some typos that affect build and then other issues that could be typo or they could be due to compiler directive issues in twi_pins.c. I pulled the latest .zip from the GitHub main page.
Here's the build output:

C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c: In function 'TWI0_Pins':
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:182:50: error: 'PORTMUX_TWI0_gc' undeclared (first use in this function); did you mean 'PORTMUX_TWI0_gm'?
         uint8_t twimux = PORTMUX.TWISPIROUTEA & ~PORTMUX_TWI0_gc;
                                                  ^~~~~~~~~~~~~~~
                                                  PORTMUX_TWI0_gm
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:182:50: note: each undeclared identifier is reported only once for each function it appears in
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:197:32: error: 'x' undeclared (first use in this function)
           /* end can'thappen */x
                                ^
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:199:9: error: expected ';' before '}' token
         } else if (sda_pin == PIN_WIRE_SDA && scl_pin == PIN_WIRE_SCL) {
         ^
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c: At top level:
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:199:11: error: expected identifier or '(' before 'else'
         } else if (sda_pin == PIN_WIRE_SDA && scl_pin == PIN_WIRE_SCL) {
           ^~~~
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:203:11: error: expected identifier or '(' before 'else'
         } else {
           ^~~~
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:259:3: error: expected identifier or '(' before 'return'
   return false;
   ^~~~~~
C:\Users\jlewis\AppData\Local\Arduino15\packages\MegaCoreX\hardware\megaavr\1.1.1\libraries\Wire\src\twi_pins.c:260:1: error: expected identifier or '(' before '}' token
 }
 ^

So I take it that 'x' is just errant and can be deleted. Sorting out what's going on with the other structure is a little more challenging and I figure you already know what's going on there.

[SpenceKonde]

Thanks. x is errant character, and the compiler's suggestion of _gm instead of _gc is correct.

I think the third one is a missing semicolon on previous line, and it looks like there probably aren't the same number of opening and closing parens somewhere