Add a more robust clockgen detection algorithm

[smgoldade]

Background

As part of a recent gbs-control build I did, I procured an external glockgen that claimed to be an Si5351. Upon further examination, I believe it actually utilizes a clone chip.

The clone chip does NOT replicate the reset functionality of defaulting to 11xx xxxx for the XTAL_CL register, which existing code assumes and uses for detection.

The library used for small code size for the Si5351 also doesn't write XTAL_CL as part of its init which the other small libraries do (a possible oversight?)

These two combined caused issues with this particular Si5351 board that caused it to fail detection and not run.

Changes

Previous code read the XTAL_CL value, and assumed all non-zero values were valid as on reset it should read 11xx xxxx. This fails as it appears the clone chip ships with XTAL_CL unconfigured.

My changes instead do the following

• echoes the detection method of the main Arduino library (see here)
• adds the XTAL_CL portion to the initialization routine

This method works for both what I assume to be an authentic chip (on an Adafruit version) and a clone chip (on some cheap Amazon listing), and my gbs-control functions with both.

[nyanpasu64]

The code seems to work on my clockgen (though it was recognized before already, without this branch). I do not know if it is correct or not. Though you should fix this warning (IMO amend and force-push):

D:\Shared Documents\gbs-c\gbs-control\gbs-control.ino: In function 'void externalClockGenDetectPresence()':
D:\Shared Documents\gbs-c\gbs-control\gbs-control.ino:531:56: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second: [enabled by default]
         Wire.requestFrom(SIADDR, (uint8_t)1, (uint8_t)0);
                                                        ^
In file included from D:\Shared Documents\gbs-c\gbs-control\gbs-control.ino:18:0:
C:\Users\user\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.6.3\libraries\Wire/Wire.h:71:13: note: candidate 1: uint8_t TwoWire::requestFrom(int, int, int)
     uint8_t requestFrom(int, int, int);
             ^
C:\Users\user\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.6.3\libraries\Wire/Wire.h:69:13: note: candidate 2: uint8_t TwoWire::requestFrom(uint8_t, uint8_t, uint8_t)
     uint8_t requestFrom(uint8_t, uint8_t, uint8_t);
             ^

[smgoldade]

The code seems to work on my clockgen (though it was recognized before already, without this branch). I do not know if it is correct or not. Though you should fix this warning (IMO amend and force-push):

D:\Shared Documents\gbs-c\gbs-control\gbs-control.ino: In function 'void externalClockGenDetectPresence()':
D:\Shared Documents\gbs-c\gbs-control\gbs-control.ino:531:56: warning: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second: [enabled by default]
         Wire.requestFrom(SIADDR, (uint8_t)1, (uint8_t)0);
                                                        ^
In file included from D:\Shared Documents\gbs-c\gbs-control\gbs-control.ino:18:0:
C:\Users\user\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.6.3\libraries\Wire/Wire.h:71:13: note: candidate 1: uint8_t TwoWire::requestFrom(int, int, int)
     uint8_t requestFrom(int, int, int);
             ^
C:\Users\user\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.6.3\libraries\Wire/Wire.h:69:13: note: candidate 2: uint8_t TwoWire::requestFrom(uint8_t, uint8_t, uint8_t)
     uint8_t requestFrom(uint8_t, uint8_t, uint8_t);
             ^

I dont know how I missed this before, but I have fixed this.

[nyanpasu64]

I dont know how I missed this before, but I have fixed this.

Horrifyingly, Arduino disables warnings by default. To fix this on Arduino IDE 1, follow https://stackoverflow.com/a/45422879. I'm not sure if this is fixed on Arduino IDE 2 yet.

[ramapcsx2]

Good catch.. People are working on redoing the project without Arduino, on a different platform (ESP32), however, there is a substantial install base on ESP8266. If the ESP32 port is successful, it would be good to look into backporting the basic app.

[nyanpasu64]

This branch still introduces trailing whitespace on a few lines. My personal preference is to configure your text editor to strip trailing whitespace on save (for this project or globally), especially since this project has been auto-formatted in the past to remove all trailing whitespace, I set my own editor to strip trailing whitespace, and I'd prefer to keep the code neat.

[smgoldade]

This branch still introduces trailing whitespace on a few lines. My personal preference is to configure your text editor to strip trailing whitespace on save (for this project or globally), especially since this project has been auto-formatted in the past to remove all trailing whitespace, I set my own editor to strip trailing whitespace, and I'd prefer to keep the code neat.

Sorry, I did the editing in Arduino IDE when I'm used to much more sophisticated solutions that handle this for me.. I noticed the Arduino IDE doesnt even look at the clang-format or anything.

I opened this in my usual IDE and applied the clang-format to my code additions, hopefully that resolves everything!

[nyanpasu64]

I do my development in VS Code setup with the Arduino plugin (and some tricky method to convince it to autogenerate c_cpp_properties.json), but build/upload from the Arduino v1 IDE. To build the project with changes saved in VS Code, I think you don't need to close and reopen Arduino IDE before building, even if Arduino IDE doesn't see those saved changes in its primitive text editor. (Though if you merely alt-tab to Arduino and build, the changes will not be saved to disk and will not apply.)

Also I don't actually use clang-format myself... 😅

[nyanpasu64]

I haven't merged this change yet because I didn't know the chip or code well enough to review the code. Decided to take a look at this code just now:

• [ ] externalClockGenDetectPresence is always called before externalClockGenInitialize, but is defined after. With my refactoring cap on, I'd reorder the functions the other way around, or perhaps even merge them together.
• [ ] After calling Wire.requestFrom((uint8_t)SIADDR, (size_t)1, false), I think Wire.available() will never exceed 1, even if the device writes more bytes. Your comment "Only 1 byte should be returned, all other situations won't be an Si5351" indicates you thought otherwise. Reading the code, I think TwoWire::available() will only return 1:

Wire.requestFrom((uint8_t)SIADDR, (size_t)1, false);
[size_t TwoWire::requestFrom(uint8_t address, size_t size=1, bool sendStop)] {
    size_t read = (twi_readFrom(address, rxBuffer, size, sendStop) == 0) ? (size=1) : 0;
    rxBufferIndex = 0;
    rxBufferLength = (read=1);
}
int TwoWire::available(void) {
    int result = (rxBufferLength=1) - (rxBufferIndex=0);
}

I'm not sure what you intended to check, and what should be done here.

• It's suspicious that externalClockGenInitialize sets the lower bits to 0b11'010010 when the Si5351 datasheet says "Reserved: Leave as default". But I guess it should be fine, since https://github.com/etherkit/Si5351Arduino/blob/b516084e094309837d8f7db6e7c06eb5f8ef6637/src/si5351.cpp#L68 also unconditionally overwrites the lower bits when writing to SI5351_CRYSTAL_LOAD=183.
• [ ] Have you tested that with no clock generator installed, externalClockGenDetectPresence() properly detects its absence and sets rto->extClockGenDetected = 0?

[smgoldade]

I noticed the Presence/Initialize thing as well, but didn't know if it was the norm to refactor stuff like that for this, I can do that now.

As for the requestFrom, I'm not sure now why I thought available could be higher than requestFrom, perhaps I had looked at a different reference that didn't match the ESP library for it or something, but then I use flush, which is non-standard, so not sure why I came up with that idea 😆

The lower bits thing isn't from the datasheet, its actually from an Application note, see AN619, which states "Bits 5:0 should be written to 010010b."

And lastly, I have tested without a clock gen as well to verify no detection with the existing code.

I will make your suggested refactoring changes and retest, thanks!

[ramapcsx2]

The chip starts up with an empty configuration. I wonder whether it sets anything on "Register 183. Crystal Internal Load Capacitance"? If it does, it'd be best to just keep it as is (short of setting the required load config)

[smgoldade]

The chip starts up with an empty configuration. I wonder whether it sets anything on "Register 183. Crystal Internal Load Capacitance"? If it does, it'd be best to just keep it as is (short of setting the required load config)

According to the AN619 note, and the original datasheet, its supposed to start in the 10pF state, but I noticed most libraries manually set it to 10pF anyways, and I think the reason is the whole reason I started this journey, in that some clone chips dont properly start with the default the datasheet says. So by default, all this change is doing is initializing it to 10pF, which was supposed to be the default (and is on the namebrand chip)

[nyanpasu64]

I've rebased this branch on master (https://github.com/ramapcsx2/gbs-control/tree/robust-clockgen-rebase), to clean up the commit history (merging both commits into one, is that okay with you?). I'm also introducing a previous commit which reorders externalClockGenDetectPresence before externalClockGenInitialize, so I can more easily see the functional changes in your branch. Can you reset this PR's branch to my branch (and amend your commit with a signature if you want), then force-push?

(I accidentally pushed an earlier version of this branch to master, because Git Extensions autofilled the push target with the wrong branch. I force pushed the correct master branch without these commits, hopefully nobody pulled them already.)

• You accidentally added a few whitespaces on empty lines, and indented if ((retVal & 0x80) == 0) etc. by 2 extra spaces. I'm fixing that too.
  • VS Code's autoformatter also decided to replace if() with if () (good for consistency with code), and Wire.write(183); // XTAL_CL with four spaces before the comment. Is that fine with you too?

As for functional correctness... as far as I can tell, your latest commit doesn't seem to have any functional changes (reorders code, but it should have the same observable behavior) to d46688df805864b581c131e0539e95ab022db41c (I again had to create a local branch which reordered the two functions, to better see what your latest commit changed). And you said you verified the previous commit works on clone and real clockgens, and properly handles no clockgen installed. And I checked my rebase detects my official clockgen (do not have a clone to test, did not try disconnecting it). So hopefully that should be fine?

[smgoldade]

Darn it, I thought I caught all the white space. I'm fine with any style based changes, I try to match as best I can but I obviously have missed stuff.

I have tested my last commit on 3 setups: 1) Adafruit Si5351 2) Cloned Si5351 3) Desoldered SDA/SCL to clock gen and verified it properly detects no clockgen correctly.

When I attempt to edit, it looks like I can change the base branch to your new branch, but it doesn't seem to let me edit the branch it's coming from unless I'm missing it. If anyone else has power to modify this (admin/contributor?) and knows how to, feel free, unless you meant changing the base, which I can do.

[nyanpasu64]

git checkout robust-clockgen
git tag robust-clockgen-old1 # pick a name to backup your current branch
git reset --keep upstream/robust-clockgen-rebase # or replace upstream with the right name for this main repo
git push --force-with-lease

IIRC I can only add commits to your branch, not force push (though I didn't test).

[smgoldade]

Thank you! I didn't think about resetting to the upstream like that. I've gotten it correctly shifted over now.

[nyanpasu64]

Thank you for bearing with my formatting and code review comments!