GW8RDI / uSDXOpen

Open firmware for all versions of the uSDX transceiver
GNU General Public License v3.0
69 stars 22 forks source link

Incorrect frequency due to improper initialisation of fxtal member of class SI5351 #10

Open murray-lang opened 1 year ago

murray-lang commented 1 year ago

I had a problem with the frequency generated by my White Buttons rig being incorrect. I was able to track it down to the failed initialisation of the fxtal member of class SI5351. The code intended to do this is on line 1649 of WhiteButtonsUSDX.ino, and looks like this:

volatile uint32_t fxtal = F_XTAL;

This is the incorrect and unreliable way to initialise class members, which are intended to be initialised in the constructor of the class. Clearly it has worked in the past through luck, but the compiler in my version of Arduino is not playing that game. I have made the following changes in my code, which have fixed the problem for me.

  1. I have added a constructor to SI5351 after line 1635 thus: class SI5351 { public: SI5351(uint32_t f_xtal) : fxtal(f_xtal) {} // <--- Here

  2. The instantiation of SI5351 on line 1900 has been changed to: static SI5351 si5351(F_XTAL);

  3. The assignment of F_XTAL to fxtal on line 1649 has been removed leaving simply: volatile uint32_t fxtal;

If you like, I am happy to make the corresponding changes to the other versions of the file and create a pull request. I'm guessing that you'd want to try it for yourself first though anyway.

Cheers, Murray VK6HL

murray-lang commented 1 year ago

Elaboration of "improper":

The class definition of SI5351 starting at line 1634, is a description of an object were it to exist. From a C++ language perspective, it doesn't exist until the instantiation on line 1900. This instantiation, because no arguments are provided, invokes the "default constructor", which should initialise all class members to the default value for for their type.

That the assignment on line 1649 is working in so many cases is a quirk of the compiler, and cannot be trusted into the future. It failed for me.