Help with fixing clock gen on McBazel ODV GBS-C

[Infection321]

As some may know the I2C wires for the clock generator on certain McBazel GBS-C boards have been wired backwards. Luckily the ESP82666 allows any pin to be assigned as an I2C, so I modified the GBS code to swap the pins whenever communicating with the clock gen.

The good news is that it works, the clock generator is recognised and is used by the board for syncing. The bad news is that it 90% works. If you enable frametime lock the image can become fuzzy, as if it has lost horizontal sync. This fuzziness appears at random and lasts a random amount of time. If frametime lock is disabled, eventually a tear line appears somewhere on the screen that can only be removed with a reset. I have done all my tests with a PS1 and PS2 connected via a RGB cable at 50hz. Seeing as turning off frametime lock makes the issue dissapear I don't think it has anything to do with the cables I'm using. When this issue appears I sometimes get a debug error of 0x1A. I saw people with NeoGeo having the same error but the solutions to their problems didn't work for me.

I have attached an image and video of the fuzziness. While this issue isn't technically about the code here on this repository, many people own this device (me included) so I do think its relevant especially with my solution mostly working. Any help or pointers in what is causing this issue are greatly apreciated.

https://youtu.be/dtE4qe3fRG0

I modified the following code:

gbs-control.ino:

void i2c_swap_pins() {
    Wire.begin(default_scl_pin, default_sda_pin);
}

void i2c_restore_pins() {
    Wire.begin(default_sda_pin, default_scl_pin);
}

void externalClockGenResetClock()
{
    if (!rto->extClockGenDetected) {
        return;
    }
    fsDebugPrintf("externalClockGenResetClock()\n");

    uint8_t activeDisplayClock = GBS::PLL648_CONTROL_01::read();

    if (activeDisplayClock == 0x25)
        rto->freqExtClockGen = 40500000;
    else if (activeDisplayClock == 0x45)
        rto->freqExtClockGen = 54000000;
    else if (activeDisplayClock == 0x55)
        rto->freqExtClockGen = 64800000;
    else if (activeDisplayClock == 0x65)
        rto->freqExtClockGen = 81000000;
    else if (activeDisplayClock == 0x85)
        rto->freqExtClockGen = 108000000;
    else if (activeDisplayClock == 0x95)
        rto->freqExtClockGen = 129600000;
    else if (activeDisplayClock == 0xa5)
        rto->freqExtClockGen = 162000000;
    else if (activeDisplayClock == 0x35)
        rto->freqExtClockGen = 81000000; // clock unused
    else if (activeDisplayClock == 0)
        rto->freqExtClockGen = 81000000; // no preset loaded
    else if (!rto->outModeHdBypass) {
        SerialM.print(F("preset display clock: 0x"));
        SerialM.println(activeDisplayClock, HEX);
    }

    i2c_swap_pins();
    // problem: around 108MHz the library seems to double the clock
    // maybe there are regs to check for this and resetPLL
    if (rto->freqExtClockGen == 108000000) {
        Si.setFreq(0, 87000000);
        delay(1); // quick fix
    }
    // same thing it seems at 40500000
    if (rto->freqExtClockGen == 40500000) {
        Si.setFreq(0, 48500000);
        delay(1); // quick fix
    }

    Si.setFreq(0, rto->freqExtClockGen);
    i2c_restore_pins();
    GBS::PAD_CKIN_ENZ::write(0); // 0 = clock input enable (pin40)
    FrameSync::clearFrequency();

    SerialM.print(F("clock gen reset: "));
    SerialM.println(rto->freqExtClockGen);
}

void externalClockGenSyncInOutRate()
{
    fsDebugPrintf("externalClockGenSyncInOutRate()\n");

    if (!rto->extClockGenDetected) {
        return;
    }
    if (GBS::PAD_CKIN_ENZ::read() != 0) {
        return;
    }
    if (rto->outModeHdBypass) {
        return;
    }
    if (GBS::PLL648_CONTROL_01::read() != 0x75) {
        return;
    }

    float sfr = getSourceFieldRate(0);
    if (sfr < 47.0f || sfr > 86.0f) {
        SerialM.print(F("sync skipped sfr wrong: "));
        SerialM.println(sfr);
        return;
    }

    float ofr = getOutputFrameRate();
    if (ofr < 47.0f || ofr > 86.0f) {
        SerialM.print(F("sync skipped ofr wrong: "));
        SerialM.println(ofr);
        return;
    }

    uint32_t old = rto->freqExtClockGen;
    FrameSync::initFrequency(ofr, old);

    setExternalClockGenFrequencySmooth((sfr / ofr) * rto->freqExtClockGen);

    int32_t diff = rto->freqExtClockGen - old;

    SerialM.print(F("source Hz: "));
    SerialM.print(sfr, 5);
    SerialM.print(F(" new out: "));
    SerialM.print(getOutputFrameRate(), 5);
    SerialM.print(F(" clock: "));
    SerialM.print(rto->freqExtClockGen);
    SerialM.print(F(" ("));
    SerialM.print(diff >= 0 ? "+" : "");
    SerialM.print(diff);
    SerialM.println(F(")"));
    delay(1);
}


void externalClockGenDetectAndInitialize()
{
    const uint8_t xtal_cl = 0xD2; // 10pF, other choices are 8pF (0x92) and 6pF (0x52) NOTE: Per AN619, the low bytes should be written 0b010010

    // MHz: 27, 32.4, 40.5, 54, 64.8, 81, 108, 129.6, 162
    rto->freqExtClockGen = 81000000;
    rto->extClockGenDetected = 0;

    if (uopt->disableExternalClockGenerator) {
        SerialM.println(F("ExternalClockGenerator disabled, skipping detection"));
        return;
    }

    i2c_swap_pins();
    uint8_t retVal = 0;
    Wire.beginTransmission(SIADDR);
    retVal = Wire.endTransmission();

    if (retVal != 0) {
        i2c_restore_pins();
        return;
    }

    Wire.beginTransmission(SIADDR);
    Wire.write(0); // Device Status
    Wire.endTransmission();
    size_t bytes_read = Wire.requestFrom((uint8_t)SIADDR, (size_t)1, false);

    if (bytes_read == 1) {
        retVal = Wire.read();
        if ((retVal & 0x80) == 0) {
            // SYS_INIT indicates device is ready.
            rto->extClockGenDetected = 1;
        } else {
            i2c_restore_pins();
            return;
        }
    } else {
        i2c_restore_pins();
        return;
    }

    Si.init(25000000L); // many Si5351 boards come with 25MHz crystal; 27000000L for one with 27MHz
    Wire.beginTransmission(SIADDR);
    Wire.write(183);    // XTAL_CL
    Wire.write(xtal_cl);
    Wire.endTransmission();
    Si.setPower(0, SIOUT_6mA);
    Si.setFreq(0, rto->freqExtClockGen);
    Si.disable(0);
    i2c_restore_pins();
}


                if (what.equals("f")) {
                    if (rto->extClockGenDetected) {
                        Serial.print(F("old freqExtClockGen: "));
                        Serial.println((uint32_t)rto->freqExtClockGen);
                        rto->freqExtClockGen = Serial.parseInt();
                        // safety range: 1 - 250 MHz
                        if (rto->freqExtClockGen >= 1000000 && rto->freqExtClockGen <= 250000000) {
                            i2c_swap_pins();
                            Si.setFreq(0, rto->freqExtClockGen);
                            i2c_restore_pins();
                            rto->clampPositionIsSet = 0;
                            rto->coastPositionIsSet = 0;
                        }
                        Serial.print(F("set freqExtClockGen: "));
                        Serial.println((uint32_t)rto->freqExtClockGen);
                    }

There where also two identical instances of Si.enable(0) that I wrapped with the swap pin code:

                        if (rto->extClockGenDetected) {
                            // switch to ext clock
                            if (!rto->outModeHdBypass) {
                                if (GBS::PLL648_CONTROL_01::read() != 0x35 && GBS::PLL648_CONTROL_01::read() != 0x75) {
                                    // first store original in an option byte in 1_2D
                                    GBS::GBS_PRESET_DISPLAY_CLOCK::write(GBS::PLL648_CONTROL_01::read());
                                    // enable and switch input
                                    i2c_swap_pins();
                                    Si.enable(0);
                                    i2c_restore_pins();
                                    delayMicroseconds(800);
                                    GBS::PLL648_CONTROL_01::write(0x75);
                                }
                            }
                            // sync clocks now
                            externalClockGenSyncInOutRate();
                        }

Here is my modified code for framesync.h:

void setExternalClockGenFrequencySmooth(uint32_t freq) {
    uint32_t current = rto->freqExtClockGen;

    rto->freqExtClockGen = freq;

    constexpr uint32_t STEP_SIZE_HZ = 1000;

    i2c_swap_pins();
    if (current > rto->freqExtClockGen) {
        if ((current - rto->freqExtClockGen) < 750000) {
            while (current > (rto->freqExtClockGen + STEP_SIZE_HZ)) {
                current -= STEP_SIZE_HZ;
                Si.setFreq(0, current);
                handleWiFi(0);
            }
        }
    } else if (current < rto->freqExtClockGen) {
        if ((rto->freqExtClockGen - current) < 750000) {
            while ((current + STEP_SIZE_HZ) < rto->freqExtClockGen) {
                current += STEP_SIZE_HZ;
                Si.setFreq(0, current);
                handleWiFi(0);
            }
        }
    }
    Si.setFreq(0, rto->freqExtClockGen);
    i2c_restore_pins();
}

[cpawliuk]

Someone else already attempted this if I recall, and the results were not great due to stability issues. The best method to fix is the problem with ODVs is to just to break the trace and re-route so SDA and SCL match up between the clock gen and ESP. I've done this myself on one of my ODV units and the fix took under a few minutes to complete. Still works months after without issue.

Having a software solution that is stable and works just as good would be nice, but I don't think it would be practical. Ie. calling Wire.begin() multiple times is also part of the problem.

[trejan]

The issue is the I2C state machines in the scaler and clock end up in bad states due to the incorrect SCL/SDA connections, Doing repeated reads of a register with a known value until the value is correct after each swap made it better but it still isn't 100%. Something still gets messed up. Hooking up a logic analyser was more hassle than just swapping the traces so I didn't investigate any further.