Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream AFSK modulator #6885

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

Akira25
Copy link
Contributor

@Akira25 Akira25 commented Sep 30, 2023

Adds the AFSK modulator from the DigiTUB Satellite communication project.

Signed-off-by: Martin Hübner martin.hubner@web.de

Description

This PR adds the AFSK modulator, that we've created in our satellite communication project at TU Berlin, to upstream GNU-Radio. We've
written this block, as there was none other available already.

Related Issue

To my knowledge, there are none realted issues.

Which blocks/areas does this affect?

The area digital is affected in adding this block. To my knowledge, there should be no negative impact on the module.

Testing Done

The PR includes Unittests. In addition, we've used the modulator already many times in downstream flowgraphs.

Checklist

@Akira25
Copy link
Contributor Author

Akira25 commented Sep 30, 2023

This is my first self-written block I try to contribute to GNU-Radio. Please feel free to comment, if I missed something out here.

@willcode
Copy link
Member

There is an in-tree clang format configuration, so
clang-format -i <filename>
will format to our spec.

@Akira25 Akira25 force-pushed the add_afsk_mod branch 2 times, most recently from d49025c to 88cc062 Compare September 30, 2023 15:26
Adds the AFSK modulator from the DigiTUB Satellite communication project.

Signed-off-by: Martin Hübner <martin.hubner@web.de>
@willcode
Copy link
Member

Take a look at gnuradio-runtime/include/gnuradio/nco.h to see if it would be useful. Setting the freq and asking for the next N values could be a lot easier than having custom calcs in work().

@Akira25
Copy link
Contributor Author

Akira25 commented Oct 1, 2023

Take a look at gnuradio-runtime/include/gnuradio/nco.h to see if it would be useful. Setting the freq and asking for the next N values could be a lot easier than having custom calcs in work().

Thank you for the hint. Though, I am not sure, which simplification exactly you aim for.

We could set the two different frequencies in two oscillators and then switch between them, which is the most trivial implementation of an AFSK modulator. But the major drawback of that approach is, to have signal with a very broad bandwidth, as we can't ensure, that the two oscillators are synchronous. When switching between them, the signal will eventually not be continuous any more, resulting in unnecessarily wide bandwidth.

There is a way better and clearer explanation for this in this blockpost with some graphics demonstrating the difference: https://notblackmagic.com/bitsnpieces/afsk//#digital-modulation-implementation

Was it that, what you want to address?

I must admit, that you are right, saying, that the implementation is a bit difficult to read. It was also difficult to write. Otherwise we really aimed for the low-bandwidth properties of the Continous AFSK modulator.

Did I maybe misunderstood you?

@daniestevez
Copy link
Member

daniestevez commented Oct 2, 2023

Not opposed to having an in-tree block dedicated to AFSK modulation, but just wanted to point out that it already can be achieved with a combination of several in-tree blocks. See example: https://github.com/daniestevez/gr-satellites/blob/main/examples/ax25/afsk.grc

Edit: In fact, looking at the code in this PR, I get the impression that the same can be achieved by the VCO block, preceded by whichever blocks are needed to unpack bits (Unpack K Bits or Packed to Unpacked), and convert from unipolar to VCO voltages (Char To Float + Add Const, or Constellation Encoder).

@Akira25
Copy link
Contributor Author

Akira25 commented Oct 2, 2023

Hello to all,

thank you very much for the comments so far.

Edit: In fact, looking at the code in this PR, I get the impression that the same can be achieved by the VCO block, preceded by whichever blocks are needed to unpack bits (Unpack K Bits or Packed to Unpacked), and convert from unipolar to VCO voltages (Char To Float + Add Const, or Constellation Encoder).

From your comment I made up this flowgraph and wanted to compare the output signals further:
grafik

Is that a correct representation of your approach? Unfortunately I'm not sure, how to set the scale options in char-to-float and add-constant blocks and the sensitivity for the VCO exactly. Help with that would be really appreciated.
I would love to see the outputs in comparison and do further analysis.

Take a look at gnuradio-runtime/include/gnuradio/nco.h to see if it would be useful. Setting the freq and asking for the next N values could be a lot easier than having custom calcs in work().

@willcode I would really like to hear some more information on that approach. Could you give me some additional explanation, please?

@Akira25
Copy link
Contributor Author

Akira25 commented Oct 2, 2023

Here is the file of the flowgraph, if you want to modify it:

modular_AFSK.tar.gz

@daniestevez
Copy link
Member

I think the following would work:

Char To Float Scale = 1 / (mark_freq_hz - space_freq_hz)
Add Const Constant = space_freq_hz
VCO sensitivity = 2 * pi

With this normalization, the output of Add Const is in Hz units, so it's easy to check for debugging.

You're also missing a Repeat block (with samp_rate / bit_rate repetitions). I forgot to mention this in my previous post.

Comment on lines +170 to +182
for (int i = 1; i < noutput_items; i++) {
// calculate the source bits indices in bit-vector on basis of
// output-buffer-size. These are needed for the integral
int curr_i = i / steps;
int prev_i = (i - 1) / steps;

// add up integrals using the trapezoidal rule
this->m += (sliced_bits[prev_i] + sliced_bits[curr_i] / 2.0);

// write data point of audio curve to output-buffer
out[i] = cos(2 * M_PI * this->t * fc_sr - 2 * M_PI * m * fd_sr);
this->t++;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things that seem problematic about this DSP:

  • It keeps integrating forever in this->m. I wonder if errors tend to build up.
  • It seems that out[0] is never written to. This is bad. Since the algorithm seems to need the previous bit, the block probably needs to use history.
  • I'm not sure that (sliced_bits[prev_i] + sliced_bits[curr_i] / 2.0) is right. It seems that (sliced_bits[prev_i] + sliced_bits[curr_i]) / 2.0 was intended instead.
  • The phase of the cos grows without bounds (due to the this->t++). This is bad because evaluation of cos() loses accuracy for large phases. The phase should be reduced modulo 2*PI or similar. In fact, there is no need to have separate variables this->m and this->t. We can have a single d_phase where we add GR_M_PI * (2.0 * fc_sr - (sliced_bits[prev_i] + sliced_bits[curr_i])) in each loop iteration, and keep wrapping this d_phase modulo 2*PI.

As an additional remark, the only fundamental difference between this algorithm and the one in fxpt_vco.h which is what is used by the VCO block is that the phase increment of the VCO is proportional to (sliced_bits[prev_i] + sliced_bits[curr_i]) / 2.0 instead of simply sliced_bits[curr_i]. This only makes a difference at symbol boundaries (and I'm not sure which choice is better; it probably depends on the definition of "better"). In any case, this new block could use fxpt_vco.h (which is good, because it's a fixed-point implementation), in the same way that the VCO block does.

Copy link
Contributor Author

@Akira25 Akira25 Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for that sharp analysis. I appreciate your thoughts on this and the hint to the fxpt_vco. I will have a look on that block.

For this->m, the formular implemented there (and mentioned in the linked blockpost), asks for an integral over the whole signal. The function of m is mainly, to "bend" the signal smoothly, when switching the frequencies. As the signal oscillates between 1 and -1, it shouldn't build up errors, as the influence of the first samples decreases with the increasing amount of new samples, right? (But this is more like a gut feeling on my side)

For the history, I partly agree. It would be much more elegant to use history here. I faced the problem, that I can only get the last N input-items. But for aligning the output signals without a break, I need the last N output items, which is not supported by the runtime. So my workaround was to keep the state in the class attributes.

For the this->t++ I defnitely agree, that this isn't ideal. Your solution with d_pahse sounds very interesting. Thank you for pointing that out. I will have a look on that.

Edit: The out[0] issue is a relict from the time, where I tried fiddeling around with the set_histrory() somehow. That didn't succeed. It was not obviously harmful on the first sight, but You are right, I should definitely change that.

@willcode
Copy link
Member

willcode commented Oct 2, 2023

It would be great to have a block that makes it so we don't need a bunch of math/translation blocks to do a simple operation. In the process though, let's think about whether it could be more general, e.g., add complex output, filtering, etc.

We do have a Continuous Phase Modulation hier block (cpmmod_bc), which can generate a complex FSK signal, and includes filtering of various sorts. It could be modified to generate float output, with a frequency offset to make the output AFSK. Just throwing that out there as another direction.

@Akira25
Copy link
Contributor Author

Akira25 commented Oct 2, 2023

I think the following would work:

Char To Float Scale = 1 / (mark_freq_hz - space_freq_hz)
Add Const Constant = space_freq_hz
VCO sensitivity = 2 * pi

With this normalization, the output of Add Const is in Hz units, so it's easy to check for debugging.

You're also missing a Repeat block (with samp_rate / bit_rate repetitions). I forgot to mention this in my previous post.

Thank you for providing that details. Probably I miss something quite substantial here, I'm sorry for that. But the Signals look quite different:
grafik

With this flowgraph:
grafik

modular means the AFSK-Modulator built from the other blocks.

modular_AFSK.tar.gz

@daniestevez
Copy link
Member

The repeat I proposed only works when the samples per bit is an integer. Try a bitrate of 1200, for instance. With a non-integer samples per bit, some fractional resampling needs to be done (and I'd also need to read your code again to think how it handles that case).

@daniestevez
Copy link
Member

We do have a Continuous Phase Modulation hier block (cpmmod_bc), which can generate a complex FSK signal, and includes filtering of various sorts. It could be modified to generate float output, with a frequency offset to make the output AFSK. Just throwing that out there as another direction.

Taking a glance at that hier block, I like this idea. The hier block is like the approach I suggested, but with a flexible way to define pulse shape filtering.

By the way, I noticed something odd when looking at this code:

      h_numerator: numerator of modulation index (integer)
      h_denominator: denominator of modulation index (numerator and denominator must be relative primes) (integer)

However, only the h_numerator / h_denominator quotient is used. My understanding is that the modulation index can be any (positive) real number, and there's no practical advantage if it's a rational number, so I wonder why the parameters were defined like this in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants