etherkit / Si5351Arduino

Library for the Si5351 clock generator IC in the Arduino environment
GNU General Public License v3.0
229 stars 94 forks source link

Output enable behavior #52

Closed w9ran closed 6 years ago

w9ran commented 6 years ago

In using the output enable function in the library I noticed that the Si5351 output would be re-enabled by a subsequent.si5351_set_freq call after being disabled with si5351.output_enable (SI5351_CLK0,0). Per PM this behavior is intentional, assuming that users would want the output turned on automatically when issuing set_freq() commands.

I understand the reason for this but it took some trial and error to determine that output.enable() does not control the output the way I thought it would. So per PM I'm listing it here for future considereration, even though we agree there is merit in both approaches.

Bob

c5gary2 commented 6 years ago

2nd vote to change this back to the way it was. Why cripple the capabilities. Or, add another argument or call to over-ride it. I was using this to do CW keying for ham radio using your breakout board that has no other hardware way. I don't see this "feature" in the release notes, when did it change so I can use the latest that doesn't have it. I was using 1.1.2 and it worked fine.
Thank you, Gary WB6OGD

NT7S commented 6 years ago

I'm convinced that the behavior should be rolled back, and I will do that in the next update. Thank you both for the feedback.

NT7S commented 6 years ago

Initial code pushed to v2.0.7 branch: https://github.com/etherkit/si5351arduino/tree/v2.0.7

The only time it will turn on the output is on the very first call to _setfreq(). I can't totally break that behavior, since that is what most code is expecting. Any other calls to _setfreq() after the initial one will not change the output enable state.

c5gary2 commented 6 years ago

HI Jason, Thank you for looking in to this. I could be wrong but I see it in your code with no qualification. Happens every call for me.

Look at line 236 in your v2.0.6 .cpp file:

uint8_t Si5351::set_freq(uint64_t freq, enum si5351_clock clk) {     struct Si5351RegSet ms_reg;     uint64_t pll_freq;     uint8_t int_mode = 0;     uint8_t div_by_4 = 0;     uint8_t r_div = 0;

    // Check which Multisynth is being set     if((uint8_t)clk <= (uint8_t)SI5351_CLK5)     {         // MS0 through MS5 logic         // ---------------------

        // Lower bounds check         if(freq > 0 && freq < SI5351_CLKOUT_MIN_FREQ SI5351_FREQ_MULT)         {             freq = SI5351_CLKOUT_MIN_FREQ SI5351_FREQ_MULT;         }

        // Upper bounds check         if(freq > SI5351_MULTISYNTH_MAX_FREQ SI5351_FREQ_MULT)         {             freq = SI5351_MULTISYNTH_MAX_FREQ SI5351_FREQ_MULT;         }

        // If requested freq >100 MHz and no other outputs are already

100 MHz,         // we need to recalculate PLLA and then recalculate all other CLK outputs         // on same PLL         if(freq > (SI5351_MULTISYNTH_SHARE_MAX SI5351_FREQ_MULT))         {             // Check other clocks on same PLL             uint8_t i;             for(i = 0; i < 6; i++)             {                 if(clk_freq[i] > (SI5351_MULTISYNTH_SHARE_MAX SI5351_FREQ_MULT))                 {                     if(i != (uint8_t)clk && pll_assignment[i] == pll_assignment[clk])                     {                         return 1; // won't set if any other clks already >100 MHz                     }                 }             }

            // Enable the output             output_enable(clk, 1); <<<<<<<<<<Line 236<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< always turns on!!!!

            // Set the freq in memory             clk_freq[(uint8_t)clk] = freq;

...

Gary

On 9/9/2017 3:38 PM, Jason Milldrum wrote:

Initial code pushed to v2.0.7 branch: https://github.com/etherkit/si5351arduino/tree/v2.0.7

The only time it will turn on the output is on the very first call to /set_freq()/. I can't totally break that behavior, since that is what most code is expecting. Any other calls to /set_freq()/ after the initial one will not change the output enable state.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328307600, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad39TV3C3qNWugXR1xVc6HmZBQNubnt8ks5sgxNggaJpZM4OuVlx.

c5gary2 commented 6 years ago

Jason, I am sorry.. I guess you are changing it so that it turns on the output only on the first call.

This is not what I need.

I need to change the frequency but not turn on (transmit) until I want to send a message. This is for a ham radio transmitter (CW).    This seems like an obvious app for your breakout board and in fact works great with your v1.0 library.

What you could do is add another optional argument to set_freq(). You could let the default turn the output on but I could over-ride it...  ???

Thank you, 73, Gary WB6OGD

On 9/9/2017 3:38 PM, Jason Milldrum wrote:

Initial code pushed to v2.0.7 branch: https://github.com/etherkit/si5351arduino/tree/v2.0.7

The only time it will turn on the output is on the very first call to /set_freq()/. I can't totally break that behavior, since that is what most code is expecting. Any other calls to /set_freq()/ after the initial one will not change the output enable state.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328307600, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad39TV3C3qNWugXR1xVc6HmZBQNubnt8ks5sgxNggaJpZM4OuVlx.

la3pna commented 6 years ago

What you are suggesting will be a breaking change, requiring changes to all of the current programs out there that uses the library.

For a beacon here, the output is turned off right after the setfreq command, and that seems to work well. I can't detect any output glitch during the start.

FWIW, using the output enable command to key a CW TX isn't realy appropriate as the rise and fall times will in my experience be excessive, and you would end up with a max WPM around 20 or so due to this. A better approach may be to AND or OR the output with a keying signal. Suitable chips for the frequency range the Si5351 works over costs a couple of cents, and the additional buffering would help.

Thomas.

2017-09-10 6:52 GMT+02:00 c5gary2 notifications@github.com:

Jason, I am sorry.. I guess you are changing it so that it turns on the output only on the first call.

This is not what I need.

I need to change the frequency but not turn on (transmit) until I want to send a message. This is for a ham radio transmitter (CW). This seems like an obvious app for your breakout board and in fact works great with your v1.0 library.

What you could do is add another optional argument to set_freq(). You could let the default turn the output on but I could over-ride it... ???

Thank you, 73, Gary WB6OGD

On 9/9/2017 3:38 PM, Jason Milldrum wrote:

Initial code pushed to v2.0.7 branch: https://github.com/etherkit/si5351arduino/tree/v2.0.7

The only time it will turn on the output is on the very first call to /set_freq()/. I can't totally break that behavior, since that is what most code is expecting. Any other calls to /set_freq()/ after the initial one will not change the output enable state.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52# issuecomment-328307600, or mute the thread https://github.com/notifications/unsubscribe-auth/ Ad39TV3C3qNWugXR1xVc6HmZBQNubnt8ks5sgxNggaJpZM4OuVlx.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328319949, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOGvJrPWak-B7lEpPi70RqJa33lXXbuks5sg2spgaJpZM4OuVlx .

-- With Best regards, Thomas S. Knutsen.

Please avoid sending me Word or PowerPoint attachments.

c5gary2 commented 6 years ago

Thank you Thomas and Jason, I have thought about it more, and you are correct, I can live with it.

In my case the old change was the breaking change.  I only upgraded the etherkit library because I updated the Arduino IDE, took me a while to figure out why my good sketches broke.  Seems to happen to me every time, I hate the library system.

But if I can shut the output off without much of a glitch, I can just put a couple dummy calls in the beginning and I'm good.

And, I realized there was a speed max, thought it was from the 16Mhz Arduino though, and it was fast enough. 73, Gary WB6OGD

On 9/10/2017 5:32 AM, Thomas S. Knutsen wrote:

What you are suggesting will be a breaking change, requiring changes to all of the current programs out there that uses the library.

For a beacon here, the output is turned off right after the setfreq command, and that seems to work well. I can't detect any output glitch during the start.

FWIW, using the output enable command to key a CW TX isn't realy appropriate as the rise and fall times will in my experience be excessive, and you would end up with a max WPM around 20 or so due to this. A better approach may be to AND or OR the output with a keying signal. Suitable chips for the frequency range the Si5351 works over costs a couple of cents, and the additional buffering would help.

Thomas.

2017-09-10 6:52 GMT+02:00 c5gary2 notifications@github.com:

Jason, I am sorry.. I guess you are changing it so that it turns on the output only on the first call.

This is not what I need.

I need to change the frequency but not turn on (transmit) until I want to send a message. This is for a ham radio transmitter (CW). This seems like an obvious app for your breakout board and in fact works great with your v1.0 library.

What you could do is add another optional argument to set_freq(). You could let the default turn the output on but I could over-ride it... ???

Thank you, 73, Gary WB6OGD

On 9/9/2017 3:38 PM, Jason Milldrum wrote:

Initial code pushed to v2.0.7 branch: https://github.com/etherkit/si5351arduino/tree/v2.0.7

The only time it will turn on the output is on the very first call to /set_freq()/. I can't totally break that behavior, since that is what most code is expecting. Any other calls to /set_freq()/ after the initial one will not change the output enable state.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52# issuecomment-328307600, or mute the thread https://github.com/notifications/unsubscribe-auth/ Ad39TV3C3qNWugXR1xVc6HmZBQNubnt8ks5sgxNggaJpZM4OuVlx.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub

https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328319949, or mute the thread

https://github.com/notifications/unsubscribe-auth/AEOGvJrPWak-B7lEpPi70RqJa33lXXbuks5sg2spgaJpZM4OuVlx .

-- With Best regards, Thomas S. Knutsen.

Please avoid sending me Word or PowerPoint attachments.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328339794, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad39TfhOtMuv2VHCJ4IFxZenbQXz7nBGks5sg9bbgaJpZM4OuVlx.

w9ran commented 6 years ago

On 9/10/2017 7:32 AM, Thomas S. Knutsen wrote:

FWIW, using the output enable command to key a CW TX isn't realy appropriate as the rise and fall times will in my experience be excessive,

There are other reasons for wanting to do this.  My original request was to make it possible to remove the injection to a transverter mixer where the signal from the Si5351 is only required for the transmit mode.   It should not be necessary to add hardware to use the output enable capability that is built-into the Si5351.

73, Bob W9RAN

la3pna commented 6 years ago

W9RAN: that usage requires no other hardware, the only limitation is in the Si5351 that it uses some time to enable the output, and has some rise time, making sending CW by enabling and disabling the output, restricted to a given max WPM.

2017-09-10 17:09 GMT+02:00 w9ran notifications@github.com:

On 9/10/2017 7:32 AM, Thomas S. Knutsen wrote:

FWIW, using the output enable command to key a CW TX isn't realy appropriate as the rise and fall times will in my experience be excessive,

There are other reasons for wanting to do this. My original request was to make it possible to remove the injection to a transverter mixer where the signal from the Si5351 is only required for the transmit mode. It should not be necessary to add hardware to use the output enable capability that is built-into the Si5351.

73, Bob W9RAN

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328348854, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOGvAFiK8tcdCuaXeGSH2-Su6lJAEjYks5sg_vGgaJpZM4OuVlx .

-- With Best regards, Thomas S. Knutsen.

Please avoid sending me Word or PowerPoint attachments.

c5gary2 commented 6 years ago

FWIW, using the output enable command to key a CW TX isn't realy appropriate as the rise and fall times will in my experience be excessive, and you would end up with a max WPM around 20 or so due to this.

Spec is 10us Max using the OEB pin that we don't have.

I just tried it using direct I2c writes and using the library. Both are close to identical.

I gave up at > 50WPM.  Keying waveform is digitally square. It makes an excellent CW generator in my opinion! Plenty fast for me  anyway   ;-)

73, Gary WB6OGD

On 9/10/2017 5:32 AM, Thomas S. Knutsen wrote:

FWIW, using the output enable command to key a CW TX isn't realy appropriate as the rise and fall times will in my experience be excessive, and you would end up with a max WPM around 20 or so due to this.

c5gary2 commented 6 years ago

Ok, this library works fine for me now.

Added one dummy call to set_freq in my setup routine.

Thank you Jason, Gary WB6OGD

On 9/9/2017 3:38 PM, Jason Milldrum wrote:

Initial code pushed to v2.0.7 branch: https://github.com/etherkit/si5351arduino/tree/v2.0.7

The only time it will turn on the output is on the very first call to /set_freq()/. I can't totally break that behavior, since that is what most code is expecting. Any other calls to /set_freq()/ after the initial one will not change the output enable state.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/etherkit/Si5351Arduino/issues/52#issuecomment-328307600, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad39TV3C3qNWugXR1xVc6HmZBQNubnt8ks5sgxNggaJpZM4OuVlx.

NT7S commented 6 years ago

Regarding the comments from @w9ran , I've checked the output of the Si5351 and there is an output glitch on initialization even if you don't turn on any of the outputs. Doing a set_freq() and then immediately turning off the output does indeed introduce another ~2 ms glitch on the output, but I don't see how that will matter since there is already a ~10 ms glitch generated just by doing an unavoidable initialization.

So this is the best that I can do under the conditions. The glitch is there regardless, so you'll have to deal with it no matter what I do here. And I can't break the old behavior, so I'm going to commit this branch.

Thanks for all of the feedback everyone.

kvdijken commented 6 years ago

I think this behaviour should be documented in the function header. Koen